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

dockerTools: add environment variables inheritance #37760

Closed
wants to merge 2 commits into from

Conversation

srghma
Copy link
Contributor

@srghma srghma commented Mar 25, 2018

Motivation for this change

Fixes #37172

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

Tested with

export NIX_PATH="nixpkgs=https://github.com/NixOS/nixpkgs-channels/archive/nixos-18.03.tar.gz"
nix-shell
google-chrome-stable $(nix-build -K nixos/tests/docker-tools.nix)/log.html

@srghma
Copy link
Contributor Author

srghma commented Mar 25, 2018

@GrahamcOfBorg test docker-tools

@roberth
Copy link
Member

roberth commented Mar 26, 2018

Thank you for your pull request. I appreciate your effort to make dockerTools more docker-like.
I do have these two concerns:

  • This is a breaking change. Some users of dockerTools probably depend on the current behavior. This can be resolved either by adding an option to include the parent environment or by splitting the container functions into two 'layers': a low-level interface and one that simulates docker build. Perhaps such a split is similar to OCI image spec / the rest of Docker?
  • ENV is not the only docker command that works in an additive way. By moving away from the current low-level model toward a model that resembles the docker UI even more, we create expectations that need to be managed using documentation.

@roberth
Copy link
Member

roberth commented Mar 26, 2018

@moretea, @LnL7, should dockerTools simulate docker build, remain low-level or try to be both?

@moretea
Copy link
Contributor

moretea commented Mar 26, 2018

IMHO think that we should have two types of tools.

Unfortunately, they would be incompatible with the current dockerTools too.

  1. Tool that is a fixed-output derivation and a bit like runCommand, which will use docker build and a Dockerfile. This can be used to build/package existing projects.
  2. A few Nix expressions that would turn a Nix closure into a list of layers, like I'm doing here, and provide a way to load that in a docker daemon, push to a remote registry.

@LnL7
Copy link
Member

LnL7 commented Mar 26, 2018

I don't really have a strong opinion on this, but I think this makes sense since the structure resembles a dockerfile very closely.

@roberth While this is indeed a breaking change, I'm not sure if it's a problem in practice.

@roberth
Copy link
Member

roberth commented Mar 27, 2018

Well it does say # WARNING: this API is unstable and may be subject to backwards-incompatible changes in the future. near the top. I'm fine with it when it's mentioned in the release notes. (You can be the first! ;) )

@srghma
Copy link
Contributor Author

srghma commented Mar 27, 2018

@roberth awesome, I vote to make this default behavior too

ENV is not the only docker command that works in an additive way.

good point, maybe I could add support for other config variables (reference), like Volumes, Labels.
Doing this manually (like I do with env now) is hard, and I'm wondering, isn't there really no tool that could generate the new schema, that inherits some other schema. I have tried to find something in open collective, but no luck.
I'll try again tomorrow

@mmahut
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@srghma
Copy link
Contributor Author

srghma commented Aug 3, 2019

@mmahut , the inheritance of envs is working (was), the tests are implemented

I didn't find tool that is specialized for merging two configs and could merge also volumes, labels, etc. (now I think its not that hard to implement it)

but I've lost interest

@goosetherumfoodle
Copy link

@roberth how can we move this forward? Is it possible to merge just this functionality (or do it in a new PR if this is too out-of-date), without the additional merging of volumes and labels?

@roberth
Copy link
Member

roberth commented Apr 6, 2020

@goosetherumfoodle It has two conflicting files now so those have to be fixed at least. If it's easy someone can add them, but env only is also ok.

@srghma
Copy link
Contributor Author

srghma commented Jun 5, 2020

@roberth I have fixed conflicts and tests have passed, I think it can be merged

@srghma
Copy link
Contributor Author

srghma commented Jun 5, 2020

Oh, I see #87253 was merged and have the same functionality.

There is no one to blame except me that this pr wasn't merged in due time.

@srghma srghma closed this Jun 5, 2020
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.

preserve environment variables using dockerTools
9 participants