-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Common utils: Add config to remove zsh rc files from #614
Common utils: Add config to remove zsh rc files from #614
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of adding the ability to control the config for oh-my-zsh 👍 (// cc @joshspicer @Chuxel for any other thoughts)
Thanks for opening the PR and being willing to iterate based on feedbacks.
I will update the README etc or anything else if you give me an approval nod. Need this to complete setup
Thank you, few things which should be added to this PR -
- Adding this ability in the form of Feature option (eg.
installOhMyZshConfig: bool
) - README.md is auto-generated from devcontainer-feature.json & NOTES.md. Hence, you won't need to update the README file
- Adding test scenarios
- Bumping the minor version - as this PR is adding in a new Feature option
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Appreciate the changes ✨
Can we add test scenarios to validate this change with installOhMyZshConfig
set to true
and false
? Let me know if you need any help with it.
See https://github.com/devcontainers/cli/blob/main/docs/features/test.md for reference.
and default to overriding .zshrc with dev-container default template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, left some minor comments. Thanks!
Co-authored-by: Samruddhi Khandale <[email protected]>
Co-authored-by: Samruddhi Khandale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naturedamends Looks like some of the zsh tests are failing, mind taking a look? Thanks!
Done |
Hmm, looks like they are still failing. @naturedamends do they pass locally for you? Let me know, I'd be happy to take a look. |
I'll hopefully get the act cli working with docker outside next time. Then should be able run tests on my local machine for any repository without having 5 docker daemons running. Feel free to merge your own or, fork this... if you want it faster. Was sure that would work, but not read all the code yet. root user home path is not ~, is my first though on issue. I'll mark work in process |
* Account for mark file in testing. * Remove some debugging and tests back * Add back tests?
@samruddhikhandale Tests now pass for me on my fork. Can I have some free build mins haha. Defo worth you looking over tho. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing~ 🚢 🎉
Thank you for reiterating over all the feedbacks! 🙏
My dotfiles adds
.zshrc
itself via thepostCreateCommand
.plz squash on merge.
Proposal
Add flag that disables overwriting the user(s)
$HOME/.zshrc
with a file containing the devcontainers default zsh theme