nixos-rebuild-ng: allow deploying closures#482430
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
thiagokokada
left a comment
There was a problem hiding this comment.
This is really similar to github.com//pull/464490, but since this seems more complete (additional checks, tests) and I like --store-path more than --use-prebuilt-closure, I am more inclined to merge this one instead of the previous one.
| Instead of building a new configuration as specified by | ||
| _/etc/nixos/configuration.nix_, roll back to the previous configuration. | ||
| (The previous configuration is defined as the one before the “current” | ||
| (The previous configuration is defined as the one before the "current" |
There was a problem hiding this comment.
Any reason for changing the quotes here? Quasi-quotes is generally used consistently in this document.
There was a problem hiding this comment.
force of habit, reverted :)
| # Disable flake auto-detection since we're using a pre-built store path | ||
| args.flake = False |
There was a problem hiding this comment.
I don't like this. If --flake is incompatible with --store-path I would prefer to raise an error here.
There was a problem hiding this comment.
The check is there as well, so this only has an effect if flake == None. The intent was to disable the flake auto-detection.
I changed the code to be a bit more explicit.
| target_host=target_host, | ||
| profile=profile, | ||
| ) | ||
| elif args.store_path: |
There was a problem hiding this comment.
Not that this matters too much because of the flags parsing, but I would prefer to check this condition first and them check for --rollout next.
| # Copy closure to target host if needed | ||
| if target_host: | ||
| nix.copy_closure( | ||
| path_to_config, | ||
| to_host=target_host, | ||
| copy_flags=grouped_nix_args.copy_flags, | ||
| ) |
There was a problem hiding this comment.
No need to check target_host here. nix.copy_closure() should do the right thing already.
Review dismissed automatically
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Tools that want to deploy NixOS all re-implement the same core logic available in `nixos-rebuild-ng` (bin/switch-to-configuration + nix-env -p) because they want to be able to deploy specific system closures. By adding a `--store-path` flag, tools can now directly invoke nixos-rebuild-ng instead. Bringing the interface to a higher level allows us to incorporate ideas such as health checks and auto-rollbacks that are harder to achieve on the lower layer.
c1366c7 to
978a08c
Compare
Review dismissed automatically
|
@thiagokokada back to you :) |
d8fe9f4
|
I think we should've named this https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/ni/nixos-install/nixos-install.sh#L39-L41 |
If you want to open a PR to fix this we can merge ASAP (before people start relying on the flag). |
|
Yeah, it's still in That being said, I think both
Arguably, EDIT: |
|
Oh if it's not in the nixos-install man page perhaps I'll rest my bike shedding case. I do think it'd be nice to be consistent though. Perhaps we add the new name to nixos-install instead |
as yet another alias for `--system/--closure`, but it's at least consistent with the recently added `--store-path` flag in `nixos-rebuild` (NixOS#482430)
as yet another alias for `--system/--closure`, but it's at least consistent with the recently added `--store-path` flag in `nixos-rebuild` (NixOS#482430)
Tools that want to deploy NixOS all re-implement the same core logic
available in
nixos-rebuild-ng(bin/switch-to-configuration + nix-env-p) because they want to be able to deploy specific system closures.
By adding a
--store-pathflag, tools can now directly invokenixos-rebuild-ng instead.
Bringing the interface to a higher level allows us to incorporate ideas
such as health checks and auto-rollbacks that are harder to achieve on
the lower layer.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.