build: accept -f - to read Dockerfile from stdin by tonistiigi · Pull Request #31236 · moby/moby

tonistiigi · 2017-02-21T20:10:56Z

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

Signed-off-by: David Sheets [email protected]
Signed-off-by: Tonis Tiigi [email protected]

aaronlehmann · 2017-02-21T21:31:07Z

pkg/archive/archive.go

The comment is a bit unclear. It makes it sound like the contents of the existing tar is replaced with just one file.

aaronlehmann · 2017-02-21T21:40:18Z

pkg/archive/archive.go

Agreed, this seems very similar to replaceDockerfileTarWrapper.

Also, I would rather avoid having two nested tar stream modifications in the trusted + stdin case.

Yes, it could be like a pipeline of modifications. The same pipeline steps could be also used for tar creation and extraction. Atm there is big list of specific arguments, for example user remapping, filename renaming for cp etc. while all these could be implemented as a conversion step.

aaronlehmann · 2017-02-21T21:42:56Z

Design LGTM

yallop · 2017-02-22T12:31:34Z

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?

dmcgowan · 2017-02-22T22:21:57Z

pkg/archive/archive.go

Not sure how important it is for how this tar stream is used but after reading the tar content there could be remaining data that on the input tar stream, we often pipe that to null to ensure that anything blocking on that read is freed (without needing to rely on the close).

You mean in case of an error? Can tarReader really skip data if it returns io.EOF

dmcgowan · 2017-02-22T22:25:11Z

pkg/archive/archive.go

Instead of always writing at end, write when err == io.EOF or as soon as header.Name <= hdr.Name

I've updated this and added more tests.

thaJeztah · 2017-02-23T19:36:32Z

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

dmcgowan · 2017-03-03T19:17:44Z

pkg/archive/archive.go

What is the point of this assignment instead of just directly passing into io.Copy or assigning here. I see this as unnecessarily shadowing the arg content.

duglin · 2017-03-06T13:49:38Z

cli/command/image/build.go

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?

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

duglin · 2017-03-06T14:45:05Z

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 · 2017-03-06T17:40:20Z

@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 · 2017-03-07T19:20:44Z

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 · 2017-03-07T19:32:33Z

@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 · 2017-03-07T20:35:45Z

@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 · 2017-03-07T20:45:51Z

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 · 2017-03-07T21:02:52Z

@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 · 2017-03-07T21:47:03Z

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

tonistiigi · 2017-03-09T01:49:06Z

@duglin That's in https://github.com/docker/docker/blob/3fe2730ab3d7bba483d7bf4fb1e95b627823ff4d/cli/command/image/build.go#L384-L388

tonistiigi · 2017-03-09T17:52:23Z

@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.

duglin · 2017-03-10T13:17:11Z

cli/command/image/build.go

I know its really really unlikely, but to avoid a flaky situation, can you just check to make sure there isn't a file with this name already in the build context? And if so, try again.

In 99% of the places we don't check things like this and it would make the code really complicated with multiple passes to achieve this. I increased the size if you are actually worried about the collisions.

Heavily based on implementation by David Sheets Signed-off-by: David Sheets <[email protected]> Signed-off-by: Tonis Tiigi <[email protected]>

Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-04-05T23:43:29Z

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.

aaronlehmann · 2017-04-06T00:23:55Z

cli/command/image/build.go

+					return nil, nil, err
+				}
+			} else {
+				b.WriteString("\n.dockerignore")


In this case, content is nil, so this will be the first entry in the file, and we shouldn't need to begin with a \n.

...but we should probably include a \n at the end in case someone tries to look at the file from the context. I think @duglin requested this in a comment on an earlier version.

Outside of this if/else there is a write that adds the trailing newline. So I think that's covered.

I'll remove the first \n, you're right it's not necessary.

Improve test coverage of ReplaceFileTarWrapper() Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-04-06T17:38:36Z

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 · 2017-04-06T17:41:13Z

LGTM

tonistiigi · 2017-04-10T17:49:14Z

LGTM for the latest updates

vdemeester

LGTM 🦁
Just one small comment 👼

vdemeester · 2017-04-10T17:50:49Z

pkg/archive/archive_test.go

 	"archive/tar"
 	"bytes"
 	"fmt"
+	"github.com/docker/docker/pkg/testutil/assert"


nit but this import is not in the right place 😛

build: accept -f - to read Dockerfile from stdin

GordonTheTurtle added the status/0-triage label Feb 21, 2017

aaronlehmann reviewed Feb 21, 2017

View reviewed changes

aaronlehmann added status/2-code-review and removed status/0-triage labels Feb 21, 2017

tonistiigi force-pushed the docker-stdin branch from 2bb02a2 to f9aeef2 Compare February 22, 2017 17:26

tonistiigi mentioned this pull request Feb 22, 2017

build: add multi-stage build support #31257

Merged

dmcgowan reviewed Feb 22, 2017

View reviewed changes

tonistiigi force-pushed the docker-stdin branch 2 times, most recently from d0d3f62 to a5da5b1 Compare February 23, 2017 22:50

dmcgowan reviewed Mar 3, 2017

View reviewed changes

tonistiigi force-pushed the docker-stdin branch from a5da5b1 to 9dfd81c Compare March 4, 2017 00:25

duglin reviewed Mar 6, 2017

View reviewed changes

GordonTheTurtle assigned vdemeester Mar 8, 2017

tonistiigi force-pushed the docker-stdin branch from 9dfd81c to 1f59a90 Compare March 9, 2017 01:46

duglin reviewed Mar 10, 2017

View reviewed changes

dnephin mentioned this pull request Apr 4, 2017

Feature/steps embeds a Dockerfile inside a image resource field dnephin/dobi#55

Closed

dsheets and others added 2 commits April 5, 2017 19:42

build: accept -f - to read Dockerfile from stdin

3f6dc81

Heavily based on implementation by David Sheets Signed-off-by: David Sheets <[email protected]> Signed-off-by: Tonis Tiigi <[email protected]>

Factor out adding dockerfile from stdin.

56bf6de

Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the docker-stdin branch from e686130 to 40b3f11 Compare April 5, 2017 23:43

aaronlehmann reviewed Apr 6, 2017

View reviewed changes

Upadte archive.ReplaceFileTarWrapper() to not expect a sorted archive

8cd6c30

Improve test coverage of ReplaceFileTarWrapper() Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the docker-stdin branch from 40b3f11 to 8cd6c30 Compare April 6, 2017 17:36

dnephin added the rebuild/powerpc label Apr 6, 2017

GordonTheTurtle removed the rebuild/powerpc label Apr 6, 2017

vdemeester approved these changes Apr 10, 2017

View reviewed changes

vdemeester added the docs/revisit label Apr 10, 2017

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

GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

thaJeztah added impact/changelog impact/cli labels Apr 10, 2017

This was referenced Apr 10, 2017

Add docs for reading Dockerfile from stdin #32488

Merged

Small build client cleanup handling - #32493

Closed

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

Merge pull request moby#31236 from tonistiigi/docker-stdin

eddc5b9

build: accept -f - to read Dockerfile from stdin

derimagia mentioned this pull request May 16, 2017

Create an official drupal-vm image on Docker Hub geerlingguy/drupal-vm#1356

Closed

This was referenced Jun 7, 2017

Feature request: Pipe input to docker build #33556

Closed

✨ Add -b flag to docker build to allow building without a Dockerfile docker/cli#165

Closed

docker build read from stdin needs documentation docker/docs#3538

Closed

MilesCranmer mentioned this pull request Jun 11, 2017

Refactor Dockerfiles to use common base layer lwa-project/bifrost#92

Merged

amontalban mentioned this pull request Aug 8, 2017

Allow using temp directory for Dockerfile on modern Docker test-kitchen/kitchen-docker#272

Closed

thaJeztah mentioned this pull request Sep 16, 2017

ISSUE: Can't use ADD when reading Dockerfile from STDIN from docker build (working as intended) #19197

Closed

thaJeztah mentioned this pull request Jun 11, 2023

Docker build doesn't disallow -f with stdin context #45700

Closed

Conversation

tonistiigi commented Feb 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Feb 21, 2017

Uh oh!

yallop commented Feb 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Mar 6, 2017

Uh oh!

tonistiigi commented Mar 6, 2017

Uh oh!

cpuguy83 commented Mar 7, 2017

Uh oh!

tonistiigi commented Mar 7, 2017

Uh oh!

duglin commented Mar 7, 2017

Uh oh!

duglin commented Mar 7, 2017

Uh oh!

tonistiigi commented Mar 7, 2017

Uh oh!

duglin commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonistiigi commented Mar 9, 2017

Uh oh!

tonistiigi commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin commented Apr 6, 2017

Uh oh!

aaronlehmann commented Apr 6, 2017

Uh oh!

tonistiigi commented Apr 10, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

duglin commented Mar 7, 2017 •

edited

Loading

dnephin commented Apr 5, 2017 •

edited

Loading