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

om health: Shell dotfiles are Nix-managed #306

Merged
merged 46 commits into from
Nov 1, 2024
Merged

om health: Shell dotfiles are Nix-managed #306

merged 46 commits into from
Nov 1, 2024

Conversation

rsrohitsingh682
Copy link
Member

resolves #299

Copy link
Member

@srid srid left a 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?

crates/nix_health/CHANGELOG.md Outdated Show resolved Hide resolved
crates/nix_health/src/check/shell_configurations.rs Outdated Show resolved Hide resolved
crates/nix_health/src/check/shell_configurations.rs Outdated Show resolved Hide resolved
crates/nix_health/src/check/shell_configurations.rs Outdated Show resolved Hide resolved
@srid srid changed the title nix_health: Check Shell configurations are Nix-Managed om health: Shell dotfiles are Nix-managed Oct 8, 2024
@rsrohitsingh682
Copy link
Member Author

@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?

MacOS where zsh is managed by nix.

Screenshot 2024-10-10 at 2 35 34 PM

MacOS where zsh is not managed by nix.

macOS

Linux (Ubuntu) where bash is not managed by nix.

Ubuntu

Copy link
Member

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

crates/nix_health/src/check/shell_configurations.rs Outdated Show resolved Hide resolved
@rsrohitsingh682
Copy link
Member Author

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 false and say it is not managed by Nix which would be wrong for the second part. Now when I think about it I am checking only dotfiles but for shells like fish the configurations are present in ~/.config/fish/config.fish. So ideally I should maintain a list where I can refer to the correct place be it .dotfiles or config files.

@rsrohitsingh682 rsrohitsingh682 marked this pull request as draft October 13, 2024 18:39
@rsrohitsingh682
Copy link
Member Author

@srid The CI seems to be failing for aarch64-darwin while running tests.

For checking the shell configuration it is looking into

/var/lib/github-runners/sambar-juspay-08/.bashrc

Any thoughts on this ?

Screenshot 2024-10-24 at 3 01 47 PM

@srid
Copy link
Member

srid commented Oct 24, 2024

That's because in CI $HOME is /var/lib/github-runners/sambar-juspay-08/.

How are you handling the case of a dotfile not even existing in the first place?

@rsrohitsingh682
Copy link
Member Author

That's because in CI $HOME is /var/lib/github-runners/sambar-juspay-08/.

How are you handling the case of a dotfile not even existing in the first place?

https://github.com/juspay/omnix/pull/306/files#diff-f499d0e179c61c3e80fd0d383c0d1574dd411dba6cb707c91bde716d75308709R129

@srid
Copy link
Member

srid commented Oct 24, 2024

Should a missing dotfile cause omnix to panic, or should it cause health check to fail?

@rsrohitsingh682
Copy link
Member Author

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,
if user's shell is not supported then I can cause the health check to fail and for other reasons like $HOME or $SHELL is not set then it should panic. And I think not able to find dotfile is could also be the reason to panic. I mean this is my understanding, I could be wrong though.

@srid
Copy link
Member

srid commented Oct 24, 2024

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.

@rsrohitsingh682
Copy link
Member Author

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.

@rsrohitsingh682 rsrohitsingh682 marked this pull request as ready for review October 24, 2024 17:59
@srid
Copy link
Member

srid commented Oct 29, 2024

@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?

Answer: behaviour when using fish:

image

@rsrohitsingh682
Copy link
Member Author

@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?

Answer: behaviour when using fish:

image

This was because of the below commit.

5aaa25e#diff-f499d0e179c61c3e80fd0d383c0d1574dd411dba6cb707c91bde716d75308709R95-R97

@srid
Copy link
Member

srid commented Oct 30, 2024

@rsrohitsingh682 Yes, this is how we want it to behave.

@srid srid merged commit efeb067 into main Nov 1, 2024
5 checks passed
@srid srid deleted the shell-configurations branch November 1, 2024 18:16
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.

om health: Check that user's shell is configured in Nix
2 participants