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/lib/make-disk-image.nix: support content mode and ownership #104292

Merged
merged 2 commits into from Dec 21, 2020

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Nov 19, 2020

Motivation for this change

Annoyance 1 in #23052

Things done
  • Tested by building a custom image with additional contents
  • 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 pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@fgaz
Copy link
Member Author

fgaz commented Nov 22, 2020

/marvin opt-in
/status awaiting_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Nov 22, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 22, 2020

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.

@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 26, 2020

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 needs_reviewer queue in one day.

2 similar comments
@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 29, 2020

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 needs_reviewer queue in one day.

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 2, 2020

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 needs_reviewer queue in one day.

@fgaz
Copy link
Member Author

fgaz commented Dec 2, 2020

@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

Copy link
Member

@Ekleog Ekleog left a 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?

nixos/lib/make-disk-image.nix Show resolved Hide resolved
nixos/lib/make-disk-image.nix Show resolved Hide resolved
@Ekleog
Copy link
Member

Ekleog commented Dec 3, 2020

Oh and also, wrt.

I think I broke it by assigning manually the first reviewer...

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 :)

@timokau
Copy link
Member

timokau commented Dec 3, 2020

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.

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 7, 2020

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 needs_reviewer queue in one day.

@Ekleog
Copy link
Member

Ekleog commented Dec 8, 2020

/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)

@timokau
Copy link
Member

timokau commented Dec 8, 2020

I think further comments (either from you or me) after a rewiew reset the status to awaiting_reviewer

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 awaiting_reviewer, since nagging only happens in that status. awaiting_changes is kind of the "abandoned PR sink", so I want to avoid putting PRs there on accident. The status doesn't have to be entirely correct, it only really matters if there are three days without activity. Still, there is room for improvement.

@timokau
Copy link
Member

timokau commented Dec 8, 2020

Although it looks like it kind of worked as intended here, since it re-ignited the previously stale discussion :)

@Ekleog
Copy link
Member

Ekleog commented Dec 8, 2020

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 ^^')

@timokau
Copy link
Member

timokau commented Dec 8, 2020

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 :)

@fgaz
Copy link
Member Author

fgaz commented Dec 14, 2020

@Ekleog

  • documented
  • added the assertion
  • fixed the bug in the loop

I also removed the possibility to use the uid and gid attributes, to keep it simple.

@fgaz
Copy link
Member Author

fgaz commented Dec 14, 2020

Also, do you know of an easy way to test this PR, ideally something directly nix-buildable like a NixOS test?

Hmm, no, sorry, my use case is external: https://git.sr.ht/~sircmpwn/builds.sr.ht/tree/master/images/nixos

EDIT:
Oops, I think I misunderstood.
Yes, I think there's some vm test stuff already in nixpkgs, let me take a look

@Ekleog
Copy link
Member

Ekleog commented Dec 14, 2020

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

@fgaz
Copy link
Member Author

fgaz commented Dec 20, 2020

@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.

@fgaz
Copy link
Member Author

fgaz commented Dec 20, 2020

@GrahamcOfBorg build nixosTests.image-contents

Copy link
Member

@Ekleog Ekleog left a 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

@Ekleog
Copy link
Member

Ekleog commented Dec 21, 2020

@GrahamcOfBorg build nixosTests.image-contents

Hmm… maybe asking ofborg directly from a review doesn't work?

@Ekleog
Copy link
Member

Ekleog commented Dec 21, 2020

@GrahamcOfBorg build nixosTests.ec2-config

Let's see whether other tests made with makeEc2Test build on aarch64

@Ekleog
Copy link
Member

Ekleog commented Dec 21, 2020

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! :)

@Ekleog Ekleog merged commit a5a819e into NixOS:master Dec 21, 2020
@fgaz
Copy link
Member Author

fgaz commented Dec 21, 2020

And thanks to you for the review!

Would it be OK to open a backport PR for this?

@fgaz fgaz deleted the image-contents branch December 22, 2020 15:39
@Ekleog
Copy link
Member

Ekleog commented Dec 23, 2020

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! :/

@fgaz
Copy link
Member Author

fgaz commented Dec 27, 2020

I understand, I'll just vendor the file for now :)

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