Skip to content
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

Merged
merged 18 commits into from
Aug 11, 2023
Merged

Common utils: Add config to remove zsh rc files from #614

merged 18 commits into from
Aug 11, 2023

Conversation

naturedamends
Copy link
Contributor

@naturedamends naturedamends commented Jul 16, 2023

My dotfiles adds .zshrc itself via the postCreateCommand.

plz squash on merge.

Proposal

Add flag that disables overwriting the user(s) $HOME/.zshrc with a file containing the devcontainers default zsh theme

Copy link
Member

@samruddhikhandale samruddhikhandale left a 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 -

  1. Adding this ability in the form of Feature option (eg. installOhMyZshConfig: bool)
  2. README.md is auto-generated from devcontainer-feature.json & NOTES.md. Hence, you won't need to update the README file
  3. Adding test scenarios
  4. Bumping the minor version - as this PR is adding in a new Feature option

src/common-utils/main.sh Outdated Show resolved Hide resolved
@samruddhikhandale samruddhikhandale mentioned this pull request Jul 27, 2023
@naturedamends naturedamends marked this pull request as draft July 28, 2023 01:49
@naturedamends
Copy link
Contributor Author

@microsoft-github-policy-service agree

@naturedamends naturedamends changed the title Add config to remove zsh rc files from common-utils. common-utils: Add config to remove zsh rc files from Jul 28, 2023
@naturedamends naturedamends changed the title common-utils: Add config to remove zsh rc files from Common utils: Add config to remove zsh rc files from Jul 28, 2023
@naturedamends naturedamends marked this pull request as ready for review July 28, 2023 20:54
Copy link
Member

@samruddhikhandale samruddhikhandale left a 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.

src/common-utils/main.sh Outdated Show resolved Hide resolved
Copy link
Member

@samruddhikhandale samruddhikhandale left a 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!

src/common-utils/devcontainer-feature.json Outdated Show resolved Hide resolved
src/common-utils/devcontainer-feature.json Outdated Show resolved Hide resolved
src/common-utils/main.sh Outdated Show resolved Hide resolved
src/common-utils/main.sh Outdated Show resolved Hide resolved
Copy link
Member

@samruddhikhandale samruddhikhandale left a 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!

image

https://github.com/devcontainers/features/actions/runs/5805829101/job/15753628087?pr=614#step:4:17389

@naturedamends
Copy link
Contributor Author

@naturedamends Looks like some of the zsh tests are failing, mind taking a look? Thanks!

image

https://github.com/devcontainers/features/actions/runs/5805829101/job/15753628087?pr=614#step:4:17389

Done

@samruddhikhandale
Copy link
Member

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.

https://github.com/devcontainers/features/actions/runs/5811584487/job/15755869319?pr=614#step:4:17303

@naturedamends
Copy link
Contributor Author

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.

https://github.com/devcontainers/features/actions/runs/5811584487/job/15755869319?pr=614#step:4:17303

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

@naturedamends naturedamends changed the title Common utils: Add config to remove zsh rc files from WIP: Common utils: Add config to remove zsh rc files from Aug 9, 2023
* Account for mark file in testing.

* Remove some debugging and tests back

* Add back tests?
@naturedamends naturedamends changed the title WIP: Common utils: Add config to remove zsh rc files from Common utils: Add config to remove zsh rc files from Aug 11, 2023
@naturedamends
Copy link
Contributor Author

@samruddhikhandale Tests now pass for me on my fork. Can I have some free build mins haha.

Defo worth you looking over tho.

Copy link
Member

@samruddhikhandale samruddhikhandale left a 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! 🙏

@samruddhikhandale samruddhikhandale merged commit be082b0 into devcontainers:main Aug 11, 2023
12 checks passed
@naturedamends naturedamends deleted the feature/common-utils-add-config-remove-env branch August 11, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants