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

Proposal: Add support for build-time environment variables to the 'build' API #9176

Closed
wants to merge 1 commit into from

Conversation

mapuri
Copy link
Contributor

@mapuri mapuri commented Nov 14, 2014

A build-time environment variable is passed to the builder (as part of build API) and made available to the Dockerfile primitives for use in variable expansion and setting up the environment for the RUN primitive (without modifying the Dockerfile and persisting them as environment in the final image).

Following simple example illustrates the feature:

docker build --build-env usr=foo --build-env http_proxy="my.proxy.url" <<EOF
From busybox
USER ${usr:-bar}
RUN git clone <my.private.repo.behind.a.proxy>
EOF

Some of the use cases this PR enables are listed below (captured from comments in the PR's thread).

[Edit: 05/22/2015] A build-time environment variable gets used only while processing the 'RUN' primitive of a DockerFile. Such a variable is only accessible during 'RUN' and is 'not' persisted with the intermediate and final docker images, thereby addressing the portability concerns of the images generated with 'build'.

This addresses issue #4962

+++++++++
Edit: 05/21/2015, 06/26/2015

This PR discussion thread has grown, bringing out use cases that this PR serves and doesn't serves well. Below I consolidate a list of those use cases that have emerged from the comments for ease of reference.

There are two broad usecases that this feature enables:

The following use-case is not served well by this feature and hence not recommended to be used such:

Following use-cases might still be suitable with caching turned off:
#9176 (comment)
#9176 (comment)

@mapuri mapuri force-pushed the master branch 3 times, most recently from d041e52 to ea496b4 Compare Nov 15, 2014
@mapuri mapuri force-pushed the master branch 2 times, most recently from 5658c13 to 83ac17d Compare Nov 20, 2014
@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Dec 4, 2014

the cmd() function has been removed from the test utils so the tests will need to be updated

@mapuri mapuri force-pushed the master branch 5 times, most recently from 6b648af to cdb2f78 Compare Dec 4, 2014
@mapuri
Copy link
Contributor Author

@mapuri mapuri commented Dec 4, 2014

@jfrazelle, thanks for the heads up. I have updated the tests.

@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Dec 4, 2014

I presume that ENV BANANA asd will over-ride the build env value, and that I can persist the build-env values by doing something like RUN export BANANA=$BUILDVAR

All up, nice! - that would allow me to do ...

docker build -e HTTP_PROXY=http://10.10.10.2:4342 -t build ., and for others to add the passwords to access their closed binaries.

which would be useful to add to the apt-cacher-ng article.

@chriskilding
Copy link

@chriskilding chriskilding commented Dec 4, 2014

In our case we are looking at using Docker to build images of our Web services, which need to obtain JARs from a private (but Internet-accessible) Maven (Artifactory) repository at build time. This PR would allow us to set our auth credentials for that Maven repo at build time, using environment variables which only exist at build time, which would be very useful to us!

Additionally, if the Docker Hub Web UI was extended with a little GUI to set these build-time variables, that would allow us to convert manual builds which our devs must currently run on their boxes to automated builds.

@mapuri
Copy link
Contributor Author

@mapuri mapuri commented Dec 4, 2014

@SvenDowideit

I presume that ENV BANANA asd will over-ride the build env value,

Yes, this PR will allow build env value for BANANA to be overriden by the value provided by ENV statement ('asd' in this case).

and that I can persist the build-env values by doing something like RUN export
BANANA=$BUILDVAR

I am not sure if I understand this presumption. Orthogonal to changes in this PR, I think issuing 'export' during a RUN will not persist the exported environment in the final container itself as it will just be run as a shell command in the context of that RUN statement. And this PR doesn't change anything around that behavior. Am I missing something?

that would allow me to do ...
docker build -e HTTP_PROXY=http://10.10.10.2:4342 -t build ., and for others to add the passwords to access their closed binaries.
which would be useful to add to the apt-cacher-ng article.

Yes, I think this should be addressed by this PR. Were you planning to use persisted environment with 'RUN export' to achieve this?

@themasterchef
Yes, this PR shall address the requirement of passing 'auth' credentials at build time without persisting them.

I have mostly worked with cli interface till now and haven't gotten to familiarizing myself with Docker Hub Web UI. So I just enhanced the cli to start making use of this feature. But I could definitely look into enhancing the web UI as well. Is it ok, if I look into it as a separate issue/PR?

@mapuri mapuri force-pushed the master branch 2 times, most recently from d5fb15c to 3816882 Compare Dec 4, 2014
@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Dec 5, 2014

@mapuri I was wondering mostly about the intended function - and I like it - it would be worth adding a little more of what you've answered here into the documentation.

please also note that the cli.md is the primary docs for commands, and the man pages are shorter summaries.

wrt Hub UI - its not open source but your work will enable things that @themasterchef, I and others would like :)

next up - @shykes @crosbymichael ? design review

@mapuri
Copy link
Contributor Author

@mapuri mapuri commented Dec 5, 2014

@SvenDowideit thanks for clarifying and pointers on documentation!

I have updated the cli.md to document more details on the changes. I have also updated docker_remote_api.md and docker_remote_api_v1.16.md to document the optional header, introduced to pass the variables to builder. The hub UI and other remote clients will form and pass this header in order to use this feature. I wasn't sure if this change demands bumping up the API version though, so I just updated v1.16 doc file for now.

@SvenDowideit
Copy link
Contributor

@SvenDowideit SvenDowideit commented Dec 15, 2014

Docs LGTM - @fredlf @jamtur01

@SvenDowideit SvenDowideit changed the title Add support for build-time environment variables to the 'build' API Proposal: Add support for build-time environment variables to the 'build' API Dec 15, 2014
@chriskilding
Copy link

@chriskilding chriskilding commented Dec 16, 2014

Looking good, do you have an estimate of when this will be making its way into master? We've just hit the point today where this is feature a really big deal for us.

`POST /build`

**New!**
This endpoint now optinally takes a serialized array of build-time environment

Choose a reason for hiding this comment

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

Spelling mistake here.

Copy link
Contributor Author

@mapuri mapuri Jan 1, 2015

Choose a reason for hiding this comment

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

Thanks for reviewing @EronWright. I have fixed the spelling and updated the diff

@mapuri mapuri force-pushed the master branch 2 times, most recently from 402fca3 to 5c1bb40 Compare Jan 1, 2015
@stuarthannig
Copy link

@stuarthannig stuarthannig commented Jul 18, 2015

+1

I'm in much need of this feature to support privately forked repositories.

@fortitudecloud
Copy link

@fortitudecloud fortitudecloud commented Jul 21, 2015

+1
This would greatly simplify my already generic build process

@vlad-belogrudov
Copy link

@vlad-belogrudov vlad-belogrudov commented Jul 21, 2015

+1
long awaited options!

@mapuri mapuri force-pushed the master branch 2 times, most recently from e840408 to 24a8bcf Compare Jul 22, 2015
@brk3
Copy link

@brk3 brk3 commented Jul 23, 2015

+1

1 similar comment
@theothermattm
Copy link

@theothermattm theothermattm commented Jul 23, 2015

+1

…text

- The build-time environment variables are passed as environment-context for command(s)
run as part of the RUN primitve. These variables are not persisted in environment of
intermediate and final images when passed as context for RUN. The build environment
is prepended to the intermediate continer's command string for aiding cache lookups.
It also helps with build traceability. But this also makes the feature less secure from
point of view of passing build time secrets.

- The build-time environment variables also get used to expand the symbols used in certain
Dockerfile primitves like ADD, COPY, USER etc, without an explicit prior definiton using a
ENV primitive. These variables get persisted in the intermediate and final images
whenever they are expanded.

Signed-off-by: Madhav Puri <madhav.puri@gmail.com>
@nathanboktae
Copy link

@nathanboktae nathanboktae commented Jul 23, 2015

+1 the Docker build is totally dependent on the state of the files it's working with, so if those files weren't placed predictably then the build will result in a different output. That argument is a strawman.

Many, many users including myself are using hacks to pipe in information into the build process... we need this PR.

@sebastiangraf
Copy link

@sebastiangraf sebastiangraf commented Jul 24, 2015

+1

3 similar comments
@imasen
Copy link

@imasen imasen commented Jul 24, 2015

+1

@AndyBerman
Copy link

@AndyBerman AndyBerman commented Jul 24, 2015

+1

@baio
Copy link

@baio baio commented Jul 24, 2015

+1

@icecrime
Copy link
Contributor

@icecrime icecrime commented Jul 24, 2015

@mapuri The signal to noise ratio in this thread has reach a point where I just cannot keep up. Please come ping me on IRC or send me a mail, and we can discuss the way to move forward into a solution that we will merge. Thank you for your work and time.

@mapuri
Copy link
Contributor Author

@mapuri mapuri commented Jul 24, 2015

Closing this in favor of #14634 .

I will work on adapting the changes in this PR to the spec detailed in #14634. Hoping to address the use cases that this thread brought up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/needs-attention Calls for a collective discussion during a review session status/1-design-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet