Conversation
Partially borrowed from Nixpkgs Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Icae2926727255621cc1178488f7b65f06a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I4fc35496a845d9a16179a20b831a22526a6a6964
WalkthroughThe changes add remote build and deployment testing infrastructure by introducing a comprehensive NixOS VM test that validates SSH-based remote building across multiple nodes. The CI workflow is updated to include the new test, test SSH keys are defined, and flake.nix integrates the test derivation into the checks attribute. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@flake.nix`:
- Around line 29-37: The current checks assignment uses the shallow merge
operator (//) which causes self.packages.${system} to be overwritten by the
genAttrs result and drops Linux package checks; update the merge to perform a
deep/recursive merge so nested keys are preserved — for example use
nixpkgs.lib.recursiveUpdate (or nixpkgs.lib.recursiveUpdateAttrs) to combine
self.packages and the genAttrs result that defines nh-remote-test, or refactor
to forAllSystems with conditional logic that adds nh-remote-test only for
non-Darwin systems; locate the checks assignment and replace the top-level //
merge of self.packages and the genAttrs block (the nh-remote-test definition)
with a recursiveUpdate-based merge or a forAllSystems conditional so nh and
default entries in self.packages.${system} are not lost.
In `@test/vm/test-remote.nix`:
- Around line 209-224: The remote subtests call deployer.succeed("nh os switch
... -f '<nixpkgs/nixos>'") but never create /root/configuration.nix on the
deployer; copy the appropriate prepared file to /root/configuration.nix on the
deployer before each remote deployer.succeed invocation (e.g. copy
configuration-2.nix before the subtest that expects config-2-deployed and
configuration-3.nix before the subtest that expects config-3-deployed). Use
deployer.succeed to perform the copy on the deployer host immediately prior to
the deployer.succeed("nh os switch ...") calls so the switch command finds
/root/configuration.nix.
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
6-24: Workflow paths filter may be incomplete.The paths filter references
.github/workflows/build.yaml(line 13, 24) but this file istest.yaml. Additionally, changes to test files (test/**) won't trigger this workflow, which means modifications totest/vm/test-remote.nixortest/ssh-keys.nixwon't automatically run the tests.Consider updating the paths filter:
Suggested paths additions
paths: - "Cargo.toml" - "Cargo.lock" - "xtask/Cargo.toml" - "src/**" - "xtask/src/**" - ".cargo/**" - - ".github/workflows/build.yaml" + - ".github/workflows/test.yaml" + - "test/**"Apply to both
pull_requestandpushsections.test/vm/test-remote.nix (1)
13-16: Consider documenting the security tradeoff in the SSH config.The SSH config disables host key verification, which is appropriate for ephemeral test VMs. However, the test also populates
known_hosts(lines 176-177), making these settings partially redundant. Consider either:
- Removing
known_hostspopulation since verification is disabled, or- Removing the
StrictHostKeyChecking=nosinceknown_hostsis populatedThe current approach works but is slightly inconsistent.
| checks = | ||
| self.packages | ||
| // self.devShells | ||
| // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | ||
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | ||
| inherit (self.packages.${system}) nh; | ||
| inherit nixpkgs; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Incorrect attribute set merge overwrites checks for Linux systems.
The // operator performs shallow merging at the top level (system keys), not a deep merge of the inner attributes. This means:
self.packages.x86_64-linux(containingnh,default) gets overwritten by thegenAttrsresult{ nh-remote-test = ...; }- Linux systems lose their package checks entirely
- Darwin systems won't have
nh-remote-test(correct) but the merge still has issues
To properly merge nested attrsets, use lib.recursiveUpdate or restructure:
Proposed fix using recursiveUpdate
checks =
- self.packages
- // self.devShells
- // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: {
- nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix {
- inherit (self.packages.${system}) nh;
- inherit nixpkgs;
- };
- });
+ nixpkgs.lib.recursiveUpdate
+ (nixpkgs.lib.recursiveUpdate self.packages self.devShells)
+ (nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: {
+ nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix {
+ inherit (self.packages.${system}) nh;
+ inherit nixpkgs;
+ };
+ }));Alternatively, use forAllSystems with conditional logic for a cleaner approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| checks = | |
| self.packages | |
| // self.devShells | |
| // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | |
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | |
| inherit (self.packages.${system}) nh; | |
| inherit nixpkgs; | |
| }; | |
| }); | |
| checks = | |
| nixpkgs.lib.recursiveUpdate | |
| (nixpkgs.lib.recursiveUpdate self.packages self.devShells) | |
| (nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | |
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | |
| inherit (self.packages.${system}) nh; | |
| inherit nixpkgs; | |
| }; | |
| })); |
🤖 Prompt for AI Agents
In `@flake.nix` around lines 29 - 37, The current checks assignment uses the
shallow merge operator (//) which causes self.packages.${system} to be
overwritten by the genAttrs result and drops Linux package checks; update the
merge to perform a deep/recursive merge so nested keys are preserved — for
example use nixpkgs.lib.recursiveUpdate (or nixpkgs.lib.recursiveUpdateAttrs) to
combine self.packages and the genAttrs result that defines nh-remote-test, or
refactor to forAllSystems with conditional logic that adds nh-remote-test only
for non-Darwin systems; locate the checks assignment and replace the top-level
// merge of self.packages and the genAttrs block (the nh-remote-test definition)
with a recursiveUpdate-based merge or a forAllSystems conditional so nh and
default entries in self.packages.${system} are not lost.
| with subtest("Remote build on deployer, deploy to target with --target-host"): | ||
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | ||
|
|
||
| # Verify hostname changed | ||
| target_hostname = target.succeed("cat /etc/hostname").strip() | ||
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | ||
|
|
||
| # Build on buildHost, activate on target (both different from deployer) | ||
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | ||
| deployer.succeed( | ||
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | ||
| ) | ||
|
|
||
| # Verify hostname changed | ||
| target_hostname = target.succeed("cat /etc/hostname").strip() | ||
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" |
There was a problem hiding this comment.
Missing configuration file setup for remote subtests.
The remote build subtests (lines 209-248) run nh os switch with -f '<nixpkgs/nixos>' on the deployer, which expects /root/configuration.nix to exist. However, only /root/configuration-{1,2,3}.nix files are copied to the deployer (lines 189-191), and /root/configuration.nix is never created on the deployer.
Each remote subtest should copy the appropriate configuration file to /root/configuration.nix on the deployer before running the switch command:
Proposed fix for the second subtest
# Build on deployer, activate on target
with subtest("Remote build on deployer, deploy to target with --target-host"):
+ deployer.succeed("cp /root/configuration-2.nix /root/configuration.nix")
deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}")Similar changes needed for the other remote subtests with the appropriate configuration file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with subtest("Remote build on deployer, deploy to target with --target-host"): | |
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | |
| # Build on buildHost, activate on target (both different from deployer) | |
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | |
| deployer.succeed( | |
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | |
| ) | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" | |
| with subtest("Remote build on deployer, deploy to target with --target-host"): | |
| deployer.succeed("cp /root/configuration-2.nix /root/configuration.nix") | |
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | |
| # Build on buildHost, activate on target (both different from deployer) | |
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | |
| deployer.succeed( | |
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | |
| ) | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" |
🤖 Prompt for AI Agents
In `@test/vm/test-remote.nix` around lines 209 - 224, The remote subtests call
deployer.succeed("nh os switch ... -f '<nixpkgs/nixos>'") but never create
/root/configuration.nix on the deployer; copy the appropriate prepared file to
/root/configuration.nix on the deployer before each remote deployer.succeed
invocation (e.g. copy configuration-2.nix before the subtest that expects
config-2-deployed and configuration-3.nix before the subtest that expects
config-3-deployed). Use deployer.succeed to perform the copy on the deployer
host immediately prior to the deployer.succeed("nh os switch ...") calls so the
switch command finds /root/configuration.nix.
1 participant
Supersedes #516
we test those
Summary by CodeRabbit
Release Notes
Tests
Chores