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.

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

nixos/test-driver: minor fixes for nixos-build-vms(8) #133675

Merged
merged 3 commits into from
Aug 16, 2021

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Aug 12, 2021

Motivation for this change

Follow-up for #125979. While I agree with the overall approach from said PR, I don't really like the current implications for nixos-build-vms(8). This PR aims to fix that.

Further details for the actual changes can be found in the commit-messages.

cc @blaggacao

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 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.

Originally removed in 926fb93. This
one is actually quite useful for `nixos-build-vms(8)`.
This is relevant for `nixos-build-vms(8)` which doesn't have a
test-script. In that case it's more intuitive to directly go into the
interactive mode which is IMHO more intuitive.
@@ -1061,7 +1062,8 @@ def clean_up() -> None:
process.terminate()
log.close()

interactive = args.interactive or (not bool(testscript))
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@tfc
Copy link
Contributor

tfc commented Aug 13, 2021

Looks great, let's run some tests to be sure:

@GrahamcOfBorg test bittorrent
@GrahamcOfBorg test boot.biosCdrom
@GrahamcOfBorg test boot.biosNetboot
@GrahamcOfBorg test boot.biosUsb
@GrahamcOfBorg test boot.uefiCdrom
@GrahamcOfBorg test boot.uefiNetboot
@GrahamcOfBorg test boot.uefiUsb
@GrahamcOfBorg test installer.bcache
@GrahamcOfBorg test installer.btrfsSimple
@GrahamcOfBorg test installer.btrfsSubvolDefault
@GrahamcOfBorg test installer.btrfsSubvols
@GrahamcOfBorg test installer.encryptedFSWithKeyfile
@GrahamcOfBorg test installer.grub1
@GrahamcOfBorg test installer.luksroot
@GrahamcOfBorg test installer.lvm
@GrahamcOfBorg test installer.separateBoot
@GrahamcOfBorg test installer.simple
@GrahamcOfBorg test installer.swraid
@GrahamcOfBorg test installer.fsroot

This is the case when the test-script is empty. `nixos-build-vms(8)` is
primarily supposed to be used as tool to test changes or to reproduce
bugs (IMHO) where "just spinning up a few VMs" is the primary use-case.

In the ongoing discussion about these changes[1] it was suggested to
only expose it when needed (i.e. in the case I described above) to keep
the API surface as slim as possible.

[1] NixOS#133675 (comment)
@Ma27
Copy link
Member Author

Ma27 commented Aug 13, 2021

cc @tfc anything to add? :)

@Ma27 Ma27 merged commit 0362d57 into NixOS:master Aug 16, 2021
@Ma27 Ma27 deleted the test-driver-compat branch August 16, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants