Skip to content

nixos: Add dry activation for users/groups#136605

Merged
Lassulus merged 2 commits intoNixOS:masterfrom
helsinki-systems:feat/dry-activation-scripts
Sep 8, 2021
Merged

nixos: Add dry activation for users/groups#136605
Lassulus merged 2 commits intoNixOS:masterfrom
helsinki-systems:feat/dry-activation-scripts

Conversation

@dasJ
Copy link
Member

@dasJ dasJ commented Sep 3, 2021

Motivation for this change

This can be extended to any degree with activation scripts offering dry-activation modes.
The users-groups activation script seemed like a good place to start because the removal of users/groups is something that's nice to know before it actually happens.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 3, 2021
@dasJ dasJ changed the title Add dry activation for users/groups nixos_ Add dry activation for users/groups Sep 3, 2021
@dasJ dasJ changed the title nixos_ Add dry activation for users/groups nixos: Add dry activation for users/groups Sep 3, 2021
@dasJ
Copy link
Member Author

dasJ commented Sep 3, 2021

@GrahamcOfBorg test simple

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 3, 2021
Copy link
Member

@Mic92 Mic92 Sep 3, 2021

Choose a reason for hiding this comment

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

I think this is a useful feature but could this be implemented without introducing another option?
system.activationScripts is a module, where we could add a .dryActivate option or so.
Than I would set an environment variable i.e. DRY_ACTIVATE instead of a flag to mark an activation script as dry-able. Does this make sense? I think this way we would duplicate less code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This idea sounds better than mine, thank you for the suggestion. Going to force-push soon-ish

Copy link
Member

Choose a reason for hiding this comment

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

in the original system.activationScripts this function could be moved out and take the also a set parameter.
Then this function could be applied twice for script and dryScript, where the latter one would receive the filtered set.

@dasJ dasJ force-pushed the feat/dry-activation-scripts branch from bd61bc7 to db95206 Compare September 3, 2021 16:05
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Diff LGTM

Got some minor suggestions.

@dasJ dasJ force-pushed the feat/dry-activation-scripts branch 3 times, most recently from d51a6dc to 3ec871b Compare September 4, 2021 17:30
@oxij
Copy link
Member

oxij commented Sep 6, 2021 via email

@dasJ dasJ requested review from rnhmjoj and stigtsp September 6, 2021 12:59
@ajs124 ajs124 requested a review from grahamc September 6, 2021 13:00
@dasJ
Copy link
Member Author

dasJ commented Sep 6, 2021

@grahamc since you know a bit of Perl ;)

@ajs124 ajs124 removed the request for review from rnhmjoj September 6, 2021 13:00
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

It looks okay to me. It looks like it could be a bit cleaner by taking the is_dry checks from the parse functions, and before any of those lines print output lines like "The plan is:" vs. "Applying:" and then changing the call sites to only call updateFile if we're not in dry run. What would you think about that?

EDIT: I think about this because it looks like it is sort of doing a "plan" phase already, so adding a dry run, I think, would not be so tricky. If I'm missing something that makes this hard ... understood :).

@dasJ dasJ force-pushed the feat/dry-activation-scripts branch from 3ec871b to a851b4d Compare September 7, 2021 08:31
@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2021

Should all be done now. I also enabled runtime warnings

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2021

@GrahamcOfBorg test mutableUsers

@asymmetric
Copy link
Contributor

Should this be added to the (future) release notes?

@dasJ dasJ deleted the feat/dry-activation-scripts branch September 9, 2021 19:16
@dasJ
Copy link
Member Author

dasJ commented Sep 9, 2021

@asymmetric The option or the fact that users/groups uses the feature?

@ryantm
Copy link
Member

ryantm commented Sep 10, 2021

Re: ryantm/agenix#55

@dasJ it seems to me like this is maybe a bug with this that it considers an activation script to support dry-activation when one of its deps does not support it.

@dasJ
Copy link
Member Author

dasJ commented Sep 10, 2021

Oh I'm so sorry, it didn't strike me that third-party repos might add dependencies to existing activation scripts.
Do you have an idea how to work around that problem? I can come up with something myself (that's not "just add dry activation to agenix") but I'm not sure I'll have enough time this week

@ryantm
Copy link
Member

ryantm commented Sep 10, 2021

@dasJ We're working on adding it. We also need to add it for specialfs in nixpkgs too:

https://github.com/ryantm/agenix/pull/56/files#diff-4d1123b4dc58254162148eaa8c5f7b32deaca05d636527a9f2f50b6566d85af3R128

@ryantm
Copy link
Member

ryantm commented Sep 11, 2021

@dasJ I pushed a workaround to agenix that just disables dry activation on users and groups for now. So no worries!

@Mic92
Copy link
Member

Mic92 commented Sep 11, 2021

@dasJ
Copy link
Member Author

dasJ commented Sep 12, 2021

@Mic92 It's not relevant since the problem only occurs when adding dependencies to an activation script that does support dry mode to depend on scripts that don't.

Last night I had an idea how to properly fix this and I'll try to implement it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants