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

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

Merged
merged 1 commit into from May 15, 2020

Conversation

@utdemir
Copy link
Member

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.
…t image
@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
@andir
Copy link
Member

andir commented May 8, 2020

@grahamc might also be qualified to review this

@purcell
Copy link
Contributor

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

bjornfor commented May 10, 2020

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
Copy link
Member

nlewo left a comment

@GrahamcOfBorg test docker-tools

@nlewo
Copy link
Member

nlewo commented May 15, 2020

Tests locally passed.
Thanks!

@nlewo nlewo merged commit 98a723e into NixOS:master May 15, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="f5a90a7"; rev="f5a90a7aab126857e9cac4f048930ddabc720c55"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@srghma srghma mentioned this pull request Jun 5, 2020
4 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.