-
Notifications
You must be signed in to change notification settings - Fork 7
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
om health
: Shell dotfiles are Nix-managed
#306
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.
@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?
nix_health
: Check Shell configurations are Nix-Managedom health
: Shell dotfiles are Nix-managed
f098426
to
5cc216c
Compare
MacOS where zsh is managed by nix. MacOS where zsh is not managed by nix. Linux (Ubuntu) where bash is not managed by nix. |
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 think the struct encapsulation can still be improved (for one thing, you only need one struct not two). But the best way to understand why is to answer this question: How does this behave under the following situations?
- User's shell is neither bash nor zsh. Its dotfiles are not managed by Nix.
- User's shell is neither bash nor zsh. Its dotfiles are managed by Nix.
For both the cases it would return |
@srid The CI seems to be failing for For checking the shell configuration it is looking into
Any thoughts on this ? |
That's because in CI How are you handling the case of a dotfile not even existing in the first place? |
|
Should a missing dotfile cause omnix to panic, or should it cause health check to fail? |
If I am able to determine user's default shell, in this case I am and after that if I am not able to find dotfile (in this case because of permission issue I guess), omnix should panic. From our last discussion, |
A missing dotfile is a legitimate use case. It should fail the health check, rather than panic. Then, if this check is required=false, the CI would not fail. |
Got it. I will make those changes then. |
…t* omnix to support all shells used by user
this otherwise has *serious* ramifications to the user, as they otherwise cannot turn off the check and get rid of the meaningless Default type on $SHELL.
Write least code possible
Answer: behaviour when using |
This was because of the below commit. 5aaa25e#diff-f499d0e179c61c3e80fd0d383c0d1574dd411dba6cb707c91bde716d75308709R95-R97 |
@rsrohitsingh682 Yes, this is how we want it to behave. |
resolves #299