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

Jump to bottom

Preserve environment variables from the parent image on dockerTools.buildImage #87253

Merged
merged 1 commit into from May 15, 2020
Merged

Preserve environment variables from the parent image on dockerTools.buildImage #87253

merged 1 commit into from May 15, 2020

Conversation

utdemir
Copy link
Member

@utdemir utdemir commented May 8, 2020

Motivation for this change

When building the Docker images with docker build, environment variables from the parent is inherited to the child images. This was not the case with dockerTools.buildImage, since it discards the parent configuration when building the child image. This behaviour is a bit inconvenient since that pattern is widely used.

This PR fetches the environment variables from the parent image and injects to the childs configuration. I also extended the NixOS tests to test this behaviour.

Still this is less than ideal, since the environment variables is not the only thing which should be inherited from the parents. Cmd, entrypoint, user, working directory, labels, volumes are also inherited, but unfortunately all in slightly different ways; and I think trying to replicate all that behavior will be costly. So, we can currently only handle environment variables and implement the others only when necessary.

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

@utdemir utdemir changed the title <!-- To help with the large amounts of pull requests, we would appreciate your reviews of other pull requests, especially simple package updates. Just leave a comment describing what you have tested in the relevant package/service. Reviewing helps to reduce the average time-to-merge for everyone. Thanks a lot if you do! List of open PRs: https://github.com/NixOS/nixpkgs/pulls Reviewing guidelines: https://hydra.nixos.org/job/nixpkgs/trunk/manual/latest/download/1/nixpkgs/manual.html#chap-reviewing-contributions --> Preserve environment variables from the parent image on dockerTools.buildImage May 8, 2020
@utdemir utdemir requested a review from nlewo May 8, 2020 10:06
@andir
Copy link
Member

andir commented May 8, 2020

@grahamc might also be qualified to review this

@purcell
Copy link
Member

purcell commented May 8, 2020

Nice! Agree re. handling Cmd, Entrypoint etc. ultimate: this PR is definitely a worthwhile improvement in the meantime.

@bjornfor
Copy link
Contributor

I looked at old PRs and found this from 2018! #37760.

@utdemir
Copy link
Member Author

utdemir commented May 10, 2020

I looked at old PRs and found this from 2018! #37760.

Sorry @bjornfor , I missed that. You are right, it's almost the same with this one.

I read the discussion there, and I still think merging this PR is worthwhile. I guess that was the final decision on that PR anyway, just the author has lost interest.

Also, one thing worth mentioning is that the inheritance of other variables (volumes, labels etc) is not that simple, because as far as I can see there are subtle nuances between the way they are inherited (eg. moby/moby#5147). So, I think the proper solution would either:

  • Figure out the inheritence rules per variable basis and implement them. This might take a lot of effort and significantly increase the code complexity.
  • Re-use docker as a library (as we do with tarsum) to create the manifest of the child layers. I do not know if it is possible, and it'd probably still be at least as complex as the current solution.

So, in short I still think this PR is useful alone (as the original one).

nlewo
nlewo approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@nlewo nlewo left a comment

Choose a reason for hiding this comment

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

@GrahamcOfBorg test docker-tools

@nlewo
Copy link
Member

nlewo commented May 15, 2020 •
edited

Tests locally passed.
Thanks!

@nlewo nlewo merged commit 98a723e into NixOS:master May 15, 2020
@srghma srghma mentioned this pull request Jun 5, 2020
Closed
8 tasks
@thefloweringash thefloweringash mentioned this pull request Jun 8, 2020
Closed
@louisblin louisblin mentioned this pull request Mar 27, 2021
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewo nlewo nlewo approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@utdemir @andir @purcell @bjornfor @nlewo