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

build: accept -f - to read Dockerfile from stdin #31236

Merged
merged 3 commits into from Apr 10, 2017

Conversation

@tonistiigi
Member

tonistiigi commented Feb 21, 2017

fixes #27393

Heavily based on implementation by @dsheets https://github.com/dsheets/docker/commits/build-dockerfile-stdin

Signed-off-by: David Sheets sheets@alum.mit.edu
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Show outdated Hide outdated pkg/archive/archive.go Outdated
Show outdated Hide outdated pkg/archive/archive.go Outdated
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 21, 2017

Contributor

Design LGTM

Contributor

aaronlehmann commented Feb 21, 2017

Design LGTM

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Feb 22, 2017

Contributor

Why are the commits squashed here? It'd be better to have two commits, to preserve history and authorship. Could you rebase to split the commits, @tonistiigi?

Contributor

yallop commented Feb 22, 2017

Why are the commits squashed here? It'd be better to have two commits, to preserve history and authorship. Could you rebase to split the commits, @tonistiigi?

Show outdated Hide outdated pkg/archive/archive.go Outdated
Show outdated Hide outdated pkg/archive/archive.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 23, 2017

Member

We discussed this PR in the maintainers meeting and like this feature; @vieux is gonna look into adding the same for -f /path/to/a/Dockerfile as a follow up

Member

thaJeztah commented Feb 23, 2017

We discussed this PR in the maintainers meeting and like this feature; @vieux is gonna look into adding the same for -f /path/to/a/Dockerfile as a follow up

Show outdated Hide outdated pkg/archive/archive.go Outdated
@@ -235,6 +247,28 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
}
}
// replace Dockerfile if added dynamically

This comment has been minimized.

@duglin

duglin Mar 6, 2017

Contributor

If someone already has a Dockerfile but its not being used for the build, and they use "-" on the -f, won't this replace their Dockerfile and cause incorrect results if their Dockerfile is used for some other purpose?

@duglin

duglin Mar 6, 2017

Contributor

If someone already has a Dockerfile but its not being used for the build, and they use "-" on the -f, won't this replace their Dockerfile and cause incorrect results if their Dockerfile is used for some other purpose?

This comment has been minimized.

@duglin

duglin Mar 10, 2017

Contributor

I think this comment needs to be updated based on the recent change you made to generate a random dockerfile name.

@duglin

duglin Mar 10, 2017

Contributor

I think this comment needs to be updated based on the recent change you made to generate a random dockerfile name.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 6, 2017

Contributor

While I like the idea of this, I'm not comfortable with replacing a user-owned file with the stdin and we should be adding any files we inject into the context to .dockerignore.

My preferred solution would be to make the input of the API a multi-part mime where one section is the tar and another section is the Dockerfile. But, that's a much bigger change. Short of that, can we consider storing the stdin into a new/random file that then gets added to .dockerignore? Our goal should be to have zero impact on existing files in the context. Meaning, a Dockerfile cmd of COPY * ... should not see any new files due to this feature.

Contributor

duglin commented Mar 6, 2017

While I like the idea of this, I'm not comfortable with replacing a user-owned file with the stdin and we should be adding any files we inject into the context to .dockerignore.

My preferred solution would be to make the input of the API a multi-part mime where one section is the tar and another section is the Dockerfile. But, that's a much bigger change. Short of that, can we consider storing the stdin into a new/random file that then gets added to .dockerignore? Our goal should be to have zero impact on existing files in the context. Meaning, a Dockerfile cmd of COPY * ... should not see any new files due to this feature.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 6, 2017

Member

@duglin This is definitely harder but I'll see what I can do. Although for - I don't see much of a problem. Dockerfile is the default name that is always used unless user sets a new name with -f. If they set -f - they don't really specify the new name. But I can see it becoming more complicated when we start accepting absolute paths outside working dir for -f as well.

Member

tonistiigi commented Mar 6, 2017

@duglin This is definitely harder but I'll see what I can do. Although for - I don't see much of a problem. Dockerfile is the default name that is always used unless user sets a new name with -f. If they set -f - they don't really specify the new name. But I can see it becoming more complicated when we start accepting absolute paths outside working dir for -f as well.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 7, 2017

Contributor

Can't we write to some temp file within the context, add to dockerignore, and set the dockerfile path to the temp file when sending to the API? If the user wants it to be part of their context they shouldn't use stdin, I think?

Contributor

cpuguy83 commented Mar 7, 2017

Can't we write to some temp file within the context, add to dockerignore, and set the dockerfile path to the temp file when sending to the API? If the user wants it to be part of their context they shouldn't use stdin, I think?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 7, 2017

Member

@cpuguy83 I guess that's what @duglin was suggesting as well. Note though that even atm we don't make any promises about the contents of Dockerfile. Using content trust with COPY Dockerfile . you would not get the same contents.

Member

tonistiigi commented Mar 7, 2017

@cpuguy83 I guess that's what @duglin was suggesting as well. Note though that even atm we don't make any promises about the contents of Dockerfile. Using content trust with COPY Dockerfile . you would not get the same contents.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 7, 2017

Contributor

@tonistiigi can you elaborate on your statement about the Dockerfile? With docker build -f foo . I would hope that any file in . would be totally untouched, including one named Dockerfile. So the user could put anything they wanted in there an be assured that docker won't touch it.

Contributor

duglin commented Mar 7, 2017

@tonistiigi can you elaborate on your statement about the Dockerfile? With docker build -f foo . I would hope that any file in . would be totally untouched, including one named Dockerfile. So the user could put anything they wanted in there an be assured that docker won't touch it.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 7, 2017

Contributor

To be honest, I'm not thrilled with modifying the user's .dockerignore file either since we never claimed it was "ours". To me its the user's input to us.

Another option would be to add a flag to just the API for now (e.g. &tmpDockerfile) to indicate that the referenced Dockerfile is a temporary one that we created that should be deleted after it is read. Then we know for sure we're not mucking with the user's files at all.

Contributor

duglin commented Mar 7, 2017

To be honest, I'm not thrilled with modifying the user's .dockerignore file either since we never claimed it was "ours". To me its the user's input to us.

Another option would be to add a flag to just the API for now (e.g. &tmpDockerfile) to indicate that the referenced Dockerfile is a temporary one that we created that should be deleted after it is read. Then we know for sure we're not mucking with the user's files at all.

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 7, 2017

Member

@duglin When content trust is used we add a digest to every FROM instruction if needed, before uploading the Dockerfile. We don't rename the file while doing so.

Member

tonistiigi commented Mar 7, 2017

@duglin When content trust is used we add a digest to every FROM instruction if needed, before uploading the Dockerfile. We don't rename the file while doing so.

@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 7, 2017

Contributor

that's interesting - is this during a docker build so that it knows which file is the real Dockerfile?

Contributor

duglin commented Mar 7, 2017

that's interesting - is this during a docker build so that it knows which file is the real Dockerfile?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Mar 9, 2017

Member

@duglin @cpuguy83 I have updated to use the random name and .dockerignore. I generalized the tar converter method as well so the same method can be more easily used for dct and plugin create.

Member

tonistiigi commented Mar 9, 2017

@duglin @cpuguy83 I have updated to use the random name and .dockerignore. I generalized the tar converter method as well so the same method can be more easily used for dct and plugin create.

Show outdated Hide outdated cli/command/image/build.go Outdated
@@ -219,6 +224,8 @@ func getDockerfileRelPath(givenContextDir, givenDockerfile string) (absContextDi
absDockerfile = altPath
}
}
} else if absDockerfile == "-" {

This comment has been minimized.

@duglin

duglin Mar 10, 2017

Contributor

would this ever be true? I'm wondering if we would see the random name instead. And if so, then I think we can remove the "if" on line 243 below (if givenDockerfile != "-" {) since it would always be true.

@duglin

duglin Mar 10, 2017

Contributor

would this ever be true? I'm wondering if we would see the random name instead. And if so, then I think we can remove the "if" on line 243 below (if givenDockerfile != "-" {) since it would always be true.

This comment has been minimized.

@tonistiigi

tonistiigi Mar 13, 2017

Member

This is called before the name replacement. So the return value is replaced anyway, but would be nice not to return anything broken from the func. So it returns the default as it can't determine the relative Dockerfile path.

@tonistiigi

tonistiigi Mar 13, 2017

Member

This is called before the name replacement. So the return value is replaced anyway, but would be nice not to return anything broken from the func. So it returns the default as it can't determine the relative Dockerfile path.

Show outdated Hide outdated cli/command/image/build.go Outdated
@duglin

This comment has been minimized.

Show comment
Hide comment
@duglin

duglin Mar 10, 2017

Contributor
$ docker build -f - . < /dev/null
Sending build context to Docker daemon  5.632kB
Error response from daemon: The Dockerfile (.dockerfile.2052ec928e68) cannot be empty

Would it be possible to catch this on the cli so we can give a better error message that doesn't include a filename that is unknown to the user? For example:

Error: the input stream Dockerfile can not be empty
Contributor

duglin commented Mar 10, 2017

$ docker build -f - . < /dev/null
Sending build context to Docker daemon  5.632kB
Error response from daemon: The Dockerfile (.dockerfile.2052ec928e68) cannot be empty

Would it be possible to catch this on the cli so we can give a better error message that doesn't include a filename that is unknown to the user? For example:

Error: the input stream Dockerfile can not be empty
Show outdated Hide outdated pkg/archive/archive.go Outdated
Show outdated Hide outdated pkg/archive/archive.go Outdated
Show outdated Hide outdated pkg/archive/archive.go Outdated

dsheets and others added some commits Feb 21, 2017

build: accept -f - to read Dockerfile from stdin
Heavily based on implementation by David Sheets

Signed-off-by: David Sheets <sheets@alum.mit.edu>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Factor out adding dockerfile from stdin.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 5, 2017

Member

I've rebased and pushed a couple commits to address these comments.

I updated the tests for ReplaceFileTarWrapper to use table testing, and added a case for appending to a file.

Member

dnephin commented Apr 5, 2017

I've rebased and pushed a couple commits to address these comments.

I updated the tests for ReplaceFileTarWrapper to use table testing, and added a case for appending to a file.

Show outdated Hide outdated cli/command/image/build.go Outdated
Upadte archive.ReplaceFileTarWrapper() to not expect a sorted archive
Improve test coverage of ReplaceFileTarWrapper()

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Apr 6, 2017

Member

Ok, added the comment, and also fixed a failing test. content was being passed as an empty reader instead of nil. This case is now also caught by the unit tests, and the comment for TarModifierFunc mentions that content will be nil when the file doesn't exist.

Member

dnephin commented Apr 6, 2017

Ok, added the comment, and also fixed a failing test. content was being passed as an empty reader instead of nil. This case is now also caught by the unit tests, and the comment for TarModifierFunc mentions that content will be nil when the file doesn't exist.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Apr 6, 2017

LGTM

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Apr 10, 2017

Member

LGTM for the latest updates

Member

tonistiigi commented Apr 10, 2017

LGTM for the latest updates

@vdemeester

LGTM 🦁
Just one small comment 👼

@@ -4,6 +4,7 @@ import (
"archive/tar"
"bytes"
"fmt"
"github.com/docker/docker/pkg/testutil/assert"

This comment has been minimized.

@vdemeester

vdemeester Apr 10, 2017

Member

nit but this import is not in the right place 😛

@vdemeester

vdemeester Apr 10, 2017

Member

nit but this import is not in the right place 😛

@vdemeester vdemeester merged commit 778e32a into moby:master Apr 10, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32617 has succeeded
Details
janky Jenkins build Docker-PRs 41227 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1423 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12344 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1240 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#31236 from tonistiigi/docker-stdin
build: accept -f - to read Dockerfile from stdin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment