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.

Sign up for GitHub

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

Jump to bottom

Salt minion improvements #41977

Closed
wants to merge 3 commits into from
Closed

Salt minion improvements #41977

wants to merge 3 commits into from

Conversation

aneeshusa
Copy link
Contributor

Motivation for this change

Review commit by commit.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@aneeshusa
nixos/salt-minion: Remove trailing whitespace
8691dcd
@aneeshusa
nixos/salt-minion: Make system packages available
07b6b00 
This makes system-wide packages available to the minion,
which makes remote execution through Salt much more ergonomic.
@aneeshusa
nixos/salt-minion: Fix salt-call without -c
7cafb0f
infinisil
infinisil reviewed Jun 24, 2018
View reviewed changes
nixos/modules/services/admin/salt/minion.nix
];
path = [
(builtins.dirOf config.security.wrapperDir)
] ++ config.environment.profiles;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen environment.profiles being used like this, but it seems like a good solution

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing:
You might want to consider reversing the list. The way this list seems to work is that the most specific thing comes last. Thats why reversing it when creating a PATH might be a good idea.

See: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/programs/environment.nix#L58

@infinisil
Copy link
Member

@aneeshusa Can you do the thing mentioned by @andir? I can then merge it

@aneeshusa
Copy link
Contributor Author

Hmm, to me it looks like the list is already ordered from specific to less specific:

nixpkgs/nixos/modules/programs/environment.nix

Lines 26 to 30 in f1fbf81

environment.profiles =
[ "$HOME/.nix-profile"
"/nix/var/nix/profiles/default"
"/run/current-system/sw"
];

The way I decided to use environment.profiles was by trying to use the exact same $PATH that nixpkgs sets up, which doesn't reverse the list:

nixpkgs/nixos/modules/config/shells-environment.nix

Lines 17 to 20 in f1fbf81

suffixedVariables =
flip mapAttrs cfg.profileRelativeEnvVars (envVar: listSuffixes:
concatMap (profile: map (suffix: "${profile}${suffix}") listSuffixes) cfg.profiles
);

That code is invoked for $PATH by this code:

nixpkgs/nixos/modules/programs/environment.nix

Lines 33 to 34 in f1fbf81

environment.profileRelativeEnvVars =
{ PATH = [ "/bin" ];

@infinisil
Copy link
Member

Hmm, I'm questioning this way of adding to PATH now. A user could also just set systemd.services.salt-minion.path = config.environment.profiles in their own module to get access to all installed program. It seems to be a source of potential non-reproducibility. Adding something to environment.systemPackages (which happens a lot in my system) could potentially alter binaries and stuff, which can have an effect on salt-minion, which seems weird. What's your motivation for doing it this way?

Maybe it would be better to just mention in the option description how users can add some packages to PATH for salt-minion (via systemd.services.salt-minion.path).

@aneeshusa
Copy link
Contributor Author

Those are reasonable concerns, so going to close this. The other two useful commits are now included in #48499.

@aneeshusa aneeshusa closed this Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andir andir andir left review comments

@infinisil infinisil infinisil approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aneeshusa @infinisil @andir @GrahamcOfBorg