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

[dockerfile] Allow ARG in FROM #31352

Merged
merged 3 commits into from Apr 10, 2017

Conversation

@dnephin
Member

dnephin commented Feb 24, 2017

Fixes #18119

This has been a major pain point for building from common base images. There's no way to build a base image with a unique tag (git sha is a common one), and then build app specific images from that tag.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin
Contributor

duglin commented Feb 24, 2017

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Feb 24, 2017

Contributor

Definitely +1 to the idea, but we'll need lots of tests :-)

Contributor

duglin commented Feb 24, 2017

Definitely +1 to the idea, but we'll need lots of tests :-)

@griwes

This comment has been minimized.

Show comment
Hide comment
@griwes

griwes Feb 24, 2017

As the author of #18119, +100.

griwes commented Feb 24, 2017

As the author of #18119, +100.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Feb 25, 2017

Member

Design LGTM

Member

tonistiigi commented Feb 25, 2017

Design LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 25, 2017

Member

Haven't checked exactly what's possible with this PR yet, but adding some points that came up in previous discussions;

  • Dockerfile should always be buildable without specifying --build-arg, this probably requires that the ARG must have a default. Empty values could be an option, but what happens with FROM foo:$tag, which leads to FROM foo: (trailing colon)?
  • Do we allow both name / registry, and tag to be set? FROM $registry:$port/$repo:$tag, or just tag?
  • Having a dedicated option for this, instead of ARG came up at some point. This would give us more flexibility, and control over what's possible / acceptable, but TBD if that's needed/desirable
Member

thaJeztah commented Feb 25, 2017

Haven't checked exactly what's possible with this PR yet, but adding some points that came up in previous discussions;

  • Dockerfile should always be buildable without specifying --build-arg, this probably requires that the ARG must have a default. Empty values could be an option, but what happens with FROM foo:$tag, which leads to FROM foo: (trailing colon)?
  • Do we allow both name / registry, and tag to be set? FROM $registry:$port/$repo:$tag, or just tag?
  • Having a dedicated option for this, instead of ARG came up at some point. This would give us more flexibility, and control over what's possible / acceptable, but TBD if that's needed/desirable
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 25, 2017

Member

With docker 1.13+ doing API version negotiation, we may have to throw an error if this feature is tried against an older daemon, but that's orthogonal to overall design

Member

thaJeztah commented Feb 25, 2017

With docker 1.13+ doing API version negotiation, we may have to throw an error if this feature is tried against an older daemon, but that's orthogonal to overall design

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Feb 25, 2017

Contributor

I think it will anyway since it'll flag a pre-FROM cmd

Contributor

duglin commented Feb 25, 2017

I think it will anyway since it'll flag a pre-FROM cmd

@StefanScherer

This comment has been minimized.

Show comment
Hide comment
@StefanScherer

StefanScherer Feb 25, 2017

Contributor

That is good for creating images for different architectures. So I don't need two Dockerfiles, one with FROM node:7.4.0-slim and another with hypriot/rpi-node:7.4.0-slim or an Windows image if the rest is identical.

Contributor

StefanScherer commented Feb 25, 2017

That is good for creating images for different architectures. So I don't need two Dockerfiles, one with FROM node:7.4.0-slim and another with hypriot/rpi-node:7.4.0-slim or an Windows image if the rest is identical.

@friism

This comment has been minimized.

Show comment
Hide comment
@friism

friism Feb 25, 2017

Contributor

cc @tt #18585

Contributor

friism commented Feb 25, 2017

cc @tt #18585

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Feb 28, 2017

Contributor

@thaJeztah your points only seem to work if you have a docker build --from alpine:3.4 . option say, which completely overrides the specified FROM. That seems a little unintuitive, as the Dockerfile is specifying one thing but the build does another, but maybe it is ok, it is not really much different from ARG.

Contributor

justincormack commented Feb 28, 2017

@thaJeztah your points only seem to work if you have a docker build --from alpine:3.4 . option say, which completely overrides the specified FROM. That seems a little unintuitive, as the Dockerfile is specifying one thing but the build does another, but maybe it is ok, it is not really much different from ARG.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 7, 2017

Contributor

Yeah, originally I was thinking a specific --from flag personally... but having it parameterized with what we have now is not awful.

Contributor

cpuguy83 commented Mar 7, 2017

Yeah, originally I was thinking a specific --from flag personally... but having it parameterized with what we have now is not awful.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 15, 2017

Member

@thaJeztah Can we move this to code review?

Member

tonistiigi commented Mar 15, 2017

@thaJeztah Can we move this to code review?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2017

Member

Yes, let's move if forward

Member

thaJeztah commented Mar 16, 2017

Yes, let's move if forward

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 16, 2017

Member

Why does this use a different variable replacer than other commands?

Member

tonistiigi commented Mar 16, 2017

Why does this use a different variable replacer than other commands?

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 16, 2017

Contributor

can we get some tests added?

Contributor

duglin commented Mar 16, 2017

can we get some tests added?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Mar 16, 2017

Member

Warning seems weird

suda@ws01:/tmp/a> cat Dockerfile 
FROM busybox:$foo

suda@ws01:/tmp/a> docker build --build-arg foo=latest .
Sending build context to Docker daemon  2.048kB
Step 1/1 : FROM busybox:$foo
 ---> 7968321274dc
[Warning] One or more build-args [foo] were not consumed
Successfully built 7968321274dc
Member

AkihiroSuda commented Mar 16, 2017

Warning seems weird

suda@ws01:/tmp/a> cat Dockerfile 
FROM busybox:$foo

suda@ws01:/tmp/a> docker build --build-arg foo=latest .
Sending build context to Docker daemon  2.048kB
Step 1/1 : FROM busybox:$foo
 ---> 7968321274dc
[Warning] One or more build-args [foo] were not consumed
Successfully built 7968321274dc
@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 16, 2017

Contributor

@AkihiroSuda the error you're seeing is correct since there is no ARG defined for "version"

Contributor

duglin commented Mar 16, 2017

@AkihiroSuda the error you're seeing is correct since there is no ARG defined for "version"

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 16, 2017

Member

Why does this use a different variable replacer than other commands?

That's a good question. I'm pretty sure I had a reason, but let me dig into it again.

can we get some tests added?

Yup, I was just waiting for this to get into code-review before investing time in tests (mentioned as a TODO in the description). I'll get to adding those now.

Warning seems weird

The error is correct. You still have to declare the arg with ARG foo before using it in the FROM

Member

dnephin commented Mar 16, 2017

Why does this use a different variable replacer than other commands?

That's a good question. I'm pretty sure I had a reason, but let me dig into it again.

can we get some tests added?

Yup, I was just waiting for this to get into code-review before investing time in tests (mentioned as a TODO in the description). I'll get to adding those now.

Warning seems weird

The error is correct. You still have to declare the arg with ARG foo before using it in the FROM

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 5, 2017

Member

It's in 17.05 and 17.06

Member

dnephin commented Jul 5, 2017

It's in 17.05 and 17.06

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 5, 2017

Member

@hillct if you want to use it for automated builds; Docker Hub, nor Docker Cloud are updated yet to 17.06 (they generally follow the EE releases); in Docker Cloud, you can configure a custom engine version though

Member

thaJeztah commented Jul 5, 2017

@hillct if you want to use it for automated builds; Docker Hub, nor Docker Cloud are updated yet to 17.06 (they generally follow the EE releases); in Docker Cloud, you can configure a custom engine version though

@hillct

This comment has been minimized.

Show comment
Hide comment
@hillct

hillct Jul 5, 2017

@dnephin I checked my version and find I'm running
Version 17.06.0-ce-mac18 (18433)
Channel: stable
d9b66511e0

The error I receive is as follows:

Sending build context to Docker daemon 89.6kB
Step 1/16 : ARG linux_versiom=latest
--->
Step 2/16 : FROM alpine:${linux_version}
invalid reference format

hillct commented Jul 5, 2017

@dnephin I checked my version and find I'm running
Version 17.06.0-ce-mac18 (18433)
Channel: stable
d9b66511e0

The error I receive is as follows:

Sending build context to Docker daemon 89.6kB
Step 1/16 : ARG linux_versiom=latest
--->
Step 2/16 : FROM alpine:${linux_version}
invalid reference format

@Foxlik

This comment has been minimized.

Show comment
Hide comment
@Foxlik

Foxlik Jul 5, 2017

@hillct there is a typo in your first step ... replace m with n and try again ;-)

Foxlik commented Jul 5, 2017

@hillct there is a typo in your first step ... replace m with n and try again ;-)

@hillct

This comment has been minimized.

Show comment
Hide comment
@hillct

hillct Jul 5, 2017

@fortinux It's always the silliest things. I had several earlier iterations absent the typos but there, my test strategy was flawed. The vagueness of the error message didn't help.

Thanks all. I'm all set.

hillct commented Jul 5, 2017

@fortinux It's always the silliest things. I had several earlier iterations absent the typos but there, my test strategy was flawed. The vagueness of the error message didn't help.

Thanks all. I'm all set.

@rogaha

This comment has been minimized.

Show comment
Hide comment
@rogaha

rogaha Jul 6, 2017

Contributor

thanks a lot for adding this @dnephin! It's awesome!

Contributor

rogaha commented Jul 6, 2017

thanks a lot for adding this @dnephin! It's awesome!

@junghans junghans referenced this pull request Jul 10, 2017

Closed

travis: remove DISTRO hack #7

@olkgrab

This comment has been minimized.

Show comment
Hide comment
@olkgrab

olkgrab Jul 13, 2017

Hello,

Looks like don't work for me. Here is my Dockerfile:

FROM node:4.4.7
ARG VERSION_DAS=4.4.7
RUN echo $VERSION_DAS

run success

Sending build context to Docker daemon  68.57MB
Step 1/3 : FROM node:4.4.7
 ---> 93b396996a16
Step 2/3 : ARG VERSION_DAS=4.4.7
 ---> Running in a088e6265995
 ---> a916acc1199a
Removing intermediate container a088e6265995
Step 3/3 : RUN echo $VERSION_DAS
 ---> Running in 487025b76de9
4.4.7
 ---> a98702da95b7
Removing intermediate container 487025b76de9
Successfully built a98702da95b7

But when I move ARG before FROM, it is didn't work

ARG VERSION_DAS=4.4.7
FROM node:4.4.7
RUN echo $VERSION_DAS

Result

docker build -f Dockerfile .
Sending build context to Docker daemon  68.57MB
Step 1/3 : ARG VERSION_DAS=4.4.7
 ---> 
Step 2/3 : FROM node:4.4.7
 ---> 93b396996a16
Step 3/3 : RUN echo $VERSION_DAS
 ---> Using cache
 ---> 2b731d6f09c2
Successfully built 2b731d6f09c2

Docker version

Client:
 Version:      17.06.0-ce
 API version:  1.30
 Go version:   go1.8.3
 Git commit:   02c1d87
 Built:        Fri Jun 23 21:20:36 2017
 OS/Arch:      linux/amd64

Server:
 Version:      17.06.0-ce
 API version:  1.30 (minimum version 1.12)
 Go version:   go1.8.3
 Git commit:   02c1d87
 Built:        Fri Jun 23 21:21:56 2017
 OS/Arch:      linux/amd64
 Experimental: false
uname -a
Linux default-centos-73.vagrantup.com 3.10.0-514.el7.x86_64 #1 SMP Tue Nov 22 16:42:41 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Could you please advice, am I doing something wrong?
@thaJeztah @Foxlik

olkgrab commented Jul 13, 2017

Hello,

Looks like don't work for me. Here is my Dockerfile:

FROM node:4.4.7
ARG VERSION_DAS=4.4.7
RUN echo $VERSION_DAS

run success

Sending build context to Docker daemon  68.57MB
Step 1/3 : FROM node:4.4.7
 ---> 93b396996a16
Step 2/3 : ARG VERSION_DAS=4.4.7
 ---> Running in a088e6265995
 ---> a916acc1199a
Removing intermediate container a088e6265995
Step 3/3 : RUN echo $VERSION_DAS
 ---> Running in 487025b76de9
4.4.7
 ---> a98702da95b7
Removing intermediate container 487025b76de9
Successfully built a98702da95b7

But when I move ARG before FROM, it is didn't work

ARG VERSION_DAS=4.4.7
FROM node:4.4.7
RUN echo $VERSION_DAS

Result

docker build -f Dockerfile .
Sending build context to Docker daemon  68.57MB
Step 1/3 : ARG VERSION_DAS=4.4.7
 ---> 
Step 2/3 : FROM node:4.4.7
 ---> 93b396996a16
Step 3/3 : RUN echo $VERSION_DAS
 ---> Using cache
 ---> 2b731d6f09c2
Successfully built 2b731d6f09c2

Docker version

Client:
 Version:      17.06.0-ce
 API version:  1.30
 Go version:   go1.8.3
 Git commit:   02c1d87
 Built:        Fri Jun 23 21:20:36 2017
 OS/Arch:      linux/amd64

Server:
 Version:      17.06.0-ce
 API version:  1.30 (minimum version 1.12)
 Go version:   go1.8.3
 Git commit:   02c1d87
 Built:        Fri Jun 23 21:21:56 2017
 OS/Arch:      linux/amd64
 Experimental: false
uname -a
Linux default-centos-73.vagrantup.com 3.10.0-514.el7.x86_64 #1 SMP Tue Nov 22 16:42:41 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Could you please advice, am I doing something wrong?
@thaJeztah @Foxlik

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Jul 13, 2017

Member

@olkgrab Every build stage has to define its own args. If no default has been set for a stage it inherits it from the "global FROM" args.

ARG VERSION_DAS=4.4.7
FROM node:4.4.7
ARG VERSION_DAS
RUN echo $VERSION_DAS
Member

tonistiigi commented Jul 13, 2017

@olkgrab Every build stage has to define its own args. If no default has been set for a stage it inherits it from the "global FROM" args.

ARG VERSION_DAS=4.4.7
FROM node:4.4.7
ARG VERSION_DAS
RUN echo $VERSION_DAS
@rogaha

This comment has been minimized.

Show comment
Hide comment
@rogaha

rogaha Jul 13, 2017

Contributor

@tonistiigi this is the second time I see people misunderstanding this behavior (e.g. #34008), we need to improve the docs to make sure these details are well explained.

Contributor

rogaha commented Jul 13, 2017

@tonistiigi this is the second time I see people misunderstanding this behavior (e.g. #34008), we need to improve the docs to make sure these details are well explained.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 13, 2017

Member

This seems pretty clear to me: https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact

FROM instructions support variables that are declared by any ARG instructions that occur before the first FROM.

Edit: I guess this doesn't say "but the ARG before FROM can't be used anywhere else"

Member

dnephin commented Jul 13, 2017

This seems pretty clear to me: https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact

FROM instructions support variables that are declared by any ARG instructions that occur before the first FROM.

Edit: I guess this doesn't say "but the ARG before FROM can't be used anywhere else"

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 13, 2017

Member

How's this look docker/cli#333 ?

Member

dnephin commented Jul 13, 2017

How's this look docker/cli#333 ?

@thehesiod

This comment has been minimized.

Show comment
Hide comment
@thehesiod

thehesiod Jul 21, 2017

it would be nice if there was a warning, the behavior was really strange w/o any warnings

thehesiod commented Jul 21, 2017

it would be nice if there was a warning, the behavior was really strange w/o any warnings

childnode added a commit to childnode/gradle-docker-plugin that referenced this pull request Aug 10, 2017

PR for bmuschko#435 - FROM is now only mandatory and yelled to be mis…
…sing if there are other instructions than ARG before. Else, we will ignore validation if installed Docker client is capable of this functionality introduced in 17.05 https://github.com/moby/moby/releases/tag/v17.05.0-ce by moby/moby#31352

travi referenced this pull request in greenkeeper-keeper/reference-instance Sep 21, 2017

nelg referenced this pull request in deviantony/docker-elk Nov 20, 2017

@drummerwolli drummerwolli referenced this pull request Jan 9, 2018

Closed

update docker #480

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment