nixos/switch-to-configuration: Rework activation script restarts #150859
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
dasJ
force-pushed
the
feat/redo-restart-by-activation-script
branch
from
eeba8b0 to
5cdbf6e
Compare
There was a problem hiding this comment.
The concept/idea sounds good to me (although I should note that I'm not familiar with that interface) and the diff seems to be fine. I really like that this makes that interface less "dangerous" and transfers the decision logic back to the normal NixOS logic handleModifiedUnit. So +1 for this change :) (Release notes entry and tests already included <3)
That said I must admit that I find the commit message really lacking, even for our low(?) Nixpkgs standards (normally that'd be a hard NAK for me). Would you mind explaining the reasoning in a few sentences? It doesn't have to be perfect but otherwise someone looking at the Git history in the future will have no idea why this change was made (or what it even does - without looking at the diff), etc. See also https://cbea.ms/git-commit/#seven-rules.
dasJ
force-pushed
the
feat/redo-restart-by-activation-script
branch
from
5cdbf6e to
2277f82
Compare
|
Done and rebased due to merge conflicts |
|
@GrahamcOfBorg test switchTest |
|
Any more comments? If not I think this is ready to merge |
| } | ||
|
|
||
| my $baseName = $baseUnit; | ||
| $baseName =~ s/\.[a-z]*$//; |
There was a problem hiding this comment.
Maybe File::Basename::basename would be safer. (It's in core)
There was a problem hiding this comment.
I was trying this but this isn't really the basename we both were thinking ;) It's doing /a/b/c.service → /a/b/c and not the classic basename
This removes `/run/nixos/activation-reload-list` (which we will need in the future when reworking the reload logic) and makes `/run/nixos/activation-restart-list` honor `restartIfChanged` and `reloadIfChanged`. This way activation scripts don't have to bother with choosing between reloading and restarting.
dasJ
force-pushed
the
feat/redo-restart-by-activation-script
branch
from
2277f82 to
2cf157c
Compare
Motivation for this change
This was the actual reason I started touching stc.
Instead of expecting the activation scripts to differentiate between restarting and reloading, do this in stc now.
This shouldn't be a too disruptive change since the only user of that interface I know of is sops-nix and that is already using the new interface.
Should also streamline things to make #49528 easier to fix in the future.
Also test the new behaviour.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes