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/lib/make-disk-image.nix: support content mode and ownership #104292
Conversation
/marvin opt-in |
Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here. |
Reminder: Please review! This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the |
2 similar comments
Reminder: Please review! This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the |
Reminder: Please review! This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the |
@timokau I think I broke it by assigning manually the first reviewer... I don't see a way to stop it, so I'll just: /status needs_reviewer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I can't directly comment on a non-modified line, but this probably deserves documentation, at least around the contents ? []
line, and maybe at other places if we already have documentation about make-disk-image.
Also, do you know of an easy way to test this PR, ideally something directly nix-buildable like a NixOS test?
Oh and also, wrt.
AFAIU the bot currently doesn't ever actually assign a new reviewer (because of a current bug in the implementation), so you probably didn't break it, it's just that it's a reason why marvin is still beta :) |
Yeah, it's a bug. Unfortunately it might take a while longer until I get to it. If the bot is spamming you, just reset it manually for now. Sorry about that. |
Reminder: Please review! This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the |
/status awaiting_changes (anyone understands why marvin changed the status back to awaiting_reviewer? I can't see any new commit nor marvin command since the review, so this looks weird to me) |
Only comments from the PR author (implementation). Probably this comment. Its maybe a bit of an over-eager heuristic. The intent is to err on the side of |
Although it looks like it kind of worked as intended here, since it re-ignited the previously stale discussion :) |
Makes sense! (FWIW, I've been sick the last few days and didn't open the computer. I only noticed the nagging after having read the answers… that said even though it wasn't required it did work as intended here I guess, as it had been more than 3 days waiting for my feedback ^^') |
Yeah, 3 days might be a bit on the shorter side in general. I hope you feel better already and will feel better still soon :) |
I also removed the possibility to use the uid and gid attributes, to keep it simple. |
Hmm, no, sorry, my use case is external: https://git.sr.ht/~sircmpwn/builds.sr.ht/tree/master/images/nixos EDIT: |
Thank you! This looks good to me otherwise, the only missing thing is ideally a test to ensure non-regression, though we could probably land as is anyway provided the current VM tests (eg. the openstack-image-* tests) pass, as they already test some previous parts of make-disk-image. Feel free to let me know if you want to give up on adding a test to ensure non-regression, though that will mean this feature will be less stable than it could be :) /status awaiting_changes |
@Ekleog I added the test. It's based on the ec2 stuff because it makes it fairly easy to test an actual image instead of a configuration, not sure if that's ok. |
@GrahamcOfBorg build nixosTests.image-contents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basing the test on the ec2 test infrastructure is perfectly good! Going to restart ofborg as it looks like the aarch64 test failed due to a machine being rebooted or similar
@GrahamcOfBorg build nixosTests.image-contents
@GrahamcOfBorg build nixosTests.image-contents Hmm… maybe asking ofborg directly from a review doesn't work? |
@GrahamcOfBorg build nixosTests.ec2-config Let's see whether other tests made with makeEc2Test build on aarch64 |
Wellll, looks like nixosTests.ec2-config worked on aarch64 but failed on x86_64. Let's just say makeEc2Test is probably wobbly and land this :) If you have spare time, feel free to have a look as to why these tests don't consistently work on all architectures, but for now I'm calling this good enough and don't want to push more work on you. Thank you for all your work on this PR! :) |
And thanks to you for the review! Would it be OK to open a backport PR for this? |
Given it's a new feature and not a bugfix, in my opinion it's something that cannot be backported, especially seeing as it's risky given the fact it touches a quite core part of our system. Sorry! :/ |
I understand, I'll just vendor the file for now :) |
Motivation for this change
Annoyance 1 in #23052
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)