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

cleanup host environment passed to VM for image building #639

Merged

Conversation

pedohorse
Copy link
Contributor

@pedohorse pedohorse commented May 17, 2024

keeping only origBuilder env from host

should resolve #622

@Mic92
Copy link
Member

Mic92 commented May 17, 2024

Looks good. Does it work?

@pedohorse
Copy link
Contributor Author

i've mentioned in the issue #622 that it does seem to work fine, but i only manually tested a few configs for config.system.build.diskoImagesScript, and visually confirmed that result boots and seems good, nothing else.

the change should affect only VM environment, created early in stage-2 script, the only change in environment in which the script that starts VM is executed is the absence of origBuilder var there, as it's set through preVM instead

i basically know two features in this project so far :)
are there any automated tests to ensure nothing else broke from this change?

@Lassulus
Copy link
Collaborator

there are tests in the tests folder, they should be nix-build able or alternatively with nix build .#tests.make-disk-image

@pedohorse
Copy link
Contributor Author

i've ran some tests so far (I assume - the process of building IS running the test), it seem to work fine.
interestingly, the tests run with nix build will run in clean environment, so they won't catch issues as with running the result of image building script in tainted user enviornment. (unless there is such special test case that imitates that problem somewhere, but I doubt that since it seems that this was not noticed to be something noticeable before)

Oh wait, (i'm blind) I've just noticed that there are github actions that run all the tests, and all the tests pass

@pedohorse pedohorse marked this pull request as ready for review May 20, 2024 22:43
@Mic92
Copy link
Member

Mic92 commented Jun 3, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Jun 3, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c194451

@mergify mergify bot merged commit c194451 into nix-community:master Jun 3, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

diskoImagesScript creates host's user home dir with some garbage leftovers
3 participants