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 base image in Docker buildImage #50690

Closed
wants to merge 1 commit into from

Conversation

thomasjm
Copy link
Contributor

Motivation for this change

Normal Dockerfiles preserve the config.Env setting from the image they're based upon. This change makes the dockerTools buildImage command work the same way: if building on a fromImage, extract the environment variables setting and prepend it to the new image's setting.

For reference: the Docker image spec

Things done

I've tested building images with this on my local machine. Most of the steps below don't seem applicable and I'm not sure how dockerTools is tested.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@thomasjm
Copy link
Contributor Author

Friendly ping, does anyone know who would be best to review this?

If this is received positively I'd also like to open another PR to preserve the "history" fields from the previous image (in a pure way of course).

@Mic92 Mic92 requested a review from grahamc November 29, 2018 14:09
@mmahut
Copy link
Member

mmahut commented Aug 12, 2019

Are there any updates on this pull request, please?

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 3, 2020
@nrdxp
Copy link
Contributor

nrdxp commented Mar 24, 2021

@thomasjm, if you have time to fix the merge conflicts, I'd be glad to properly review this and see if we can't get it merged in time for 21.05. Seems like a really good idea to me.

@thomasjm
Copy link
Contributor Author

thomasjm commented Mar 25, 2021

Hi @nrdxp -- actually, after making this PR I ended up doing a more extensive refactor of the Docker stuff. I've been using my fork ever since but I always meant to rebase it on Nixpkgs and see if the community was interested.

You can see what it looks like here. Highlights include moving large shell scripts into their own files, adding a test suite, fixing various bugs, adding a couple new options, and improving support for multi-layered images (including what this PR does for all of the config fields).

EDIT: oh and it also stores images as unzipped by default, similar to how the streamLayeredImage function which was added later works -- this makes it much more performant to do multi-layer stuff because layers can refer to other layers with symlinks rather than doing big copies and zips.

I know there's been some divergence from the main Nixpkgs, but would you be interested in some of this stuff?

@nrdxp
Copy link
Contributor

nrdxp commented Mar 26, 2021

I believe their are many users who could benefit from your fork, if you don't mind doing the heavy work to factor it into a larger PR, I'd be glad to review and attempt to get some eyes on it.

@louisblin
Copy link

FYI, I believe this particular feature has already been implemented in #87253 since then. It would be nice to merge any other feature you've been adding to your fork though !

@thomasjm
Copy link
Contributor Author

Oh nice -- I'll close this then, and put it on my todo list to investigate what parts of my fork would make sense to merge.

@thomasjm thomasjm closed this Mar 27, 2021
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

6 participants