Have .dockerignore support Dockerfile/.dockerignore by duglin · Pull Request #8748 · moby/moby

duglin · 2014-10-24T03:41:21Z

If .dockerignore mentions either then the client will send them to the
daemon but the daemon will erase them after the Dockerfile has been parsed
to simulate them never being sent in the first place.

Closes #8330

Signed-off-by: Doug Davis dug@us.ibm.com

cpuguy83 · 2014-10-24T14:15:44Z

This doesn't seem to be working for me.

#=> cat .dockerignore
Dockerfile

#=> cat Dockerfile
FROM busybox
ADD . /foo
# This is a test

If I modify the commented line, it breaks on the ADD.

duglin · 2014-10-24T20:29:58Z

@cpuguy83 good catch. It appears the metadata on the dir was changing and that was part of the hash calculation. Not sure was I wasn't seeing that last night.... fix coming soon....

duglin · 2014-10-24T23:57:22Z

@cpuguy83 can you try it again?

thaJeztah · 2014-10-25T09:50:21Z

Can't comment from a technical point of view, but ❤️ the clear documenting in your code!

erikh · 2014-10-27T09:22:18Z

builder/evaluator.go

this should probably be in its own function

duglin · 2014-10-27T14:41:17Z

@erikh ok - moved it into a func

duglin · 2014-10-27T14:41:59Z

btw - @cpuguy83 did confirm that I fixed the issue he was seeing.

erikh · 2014-10-27T20:14:49Z

Builder changes LGTM, but untested from me (I don't have time right now). If someone could review the tarsum changes (@unclejack?) and review the file handling, I would appreciate it.

tianon · 2014-10-28T07:28:39Z

builder/evaluator.go

Why is this function explicitly only handling Dockerfile and .dockerignore instead of just reusing the same logic as the client directly? (via some refactored new function)

This would then have the benefit of making build more consistent, especially in the case of client implementations that don't yet support .dockerignore.

+1

On Oct 28, 2014, at 12:28 AM, Tianon Gravi notifications@github.com wrote:

In builder/evaluator.go:

@@ -173,6 +179,39 @@ func (b *Builder) Run(context io.Reader) (string, error) {
return b.image, nil
}

+func (b *Builder) cleanupDockerignore() {
Why is this function explicitly only handling Dockerfile and .dockerignore instead of just reusing the same logic as the client directly? (via some refactored new function)

This would then have the benefit of making build more consistent, especially in the case of client implementations that don't yet support .dockerignore.

—
Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/8748/files#r19455996.

I want to make sure I understand what you're looking for....

a common .dockerignore reader util that the client and daemon both use makes sense

but are you suggesting that all of the .dockerignore processing be done on the daemon-side as well and we look to remove all files from the build context and not just Dockerfile & .dockerignore?

My understanding is that the HTTP body of the "POST /build" is the build context, which seems like it should be "post .dockerignore" processing - because .dockerignore is really just a client-side concept. The docs talk about .dockerignore listing the files that are excluded from the context, and the first thing the docker client does is "send the context" - not "send a set of files that are a superset of the context". And its just a Docker client concept, not necessarily a concept that all users of the REST API will support or even care about. Of course, we're tweaking that notion a bit in this case due to the need for the daemon to have the Dockerfile no matter what but we can't avoid that. Too bad the Dockerfile isn't passed to the daemon separately from the build context then we'd avoid all of this and would actually be my preferred way to solve this - but that's a much bigger change.

So, I'm not comfortable with the daemon blindly processing the .dockerignore again when that might not have been what the client wanted. They should have already done all of the "exclusion" processing they wanted by not including the files in the POST /build body in the first place otherwise its like they're saying "here's my build context, but I didn't really mean it".

If we really want to head down this path I think we'd need to add a new flag so the client could explicitly ask for the daemon modify their build context (again). But at that point I wonder if its worth the complexity when all we need (right now) is to remove the Dockerfile. Do you guys think there will be other special cases like we're seeing with Dockerfile? I'm leaning towards thinking this is just a one-time edge-case.

@duglin There is some redundant code indeed. You could create a refactoring function that takes the path to the .dockerignore file, and returns excludes. That way, the client can manage the includes on its own, and the daemon just removes what the files it needs to remove from the context.

This will also allow to use a bufio.NewScanner instead of ioutil.ReadFile the whole file, not that much of a difference i bet, but good practice :)

I moved the reading of .dockerfile into utils/utils.go and used a scanner.
Hopefully we're getting close :-)

I'm not a fan of functions like this because it's not clear when and where it should be called as it accepts no input and produces no output. Maybe this should be part of readContext and that should return the Dockerfile ast?

Sorry, I don't mean to lag more on this, we are just dealing with methods like this in another area of the codebase so it's something that I noticed.

SvenDowideit · 2014-10-29T02:05:29Z

This needs documentation - grep for dockerignore in the docs dir :)

duglin · 2014-10-29T02:16:32Z

Does it? Not sure what to say since it does what they asked for now.

-Doug

Sent from my iPhone

On Oct 28, 2014, at 7:06 PM, "Sven Dowideit" notifications@github.com
wrote:

This needs documentation - grep for dockerignore in the docs dir :)

—
Reply to this email directly or view it on GitHub.

SvenDowideit · 2014-10-29T02:44:51Z

If this PR doesn't change how the .dockerignore file works (ie, that everything works as its currently documented), then er huh?

I'm assuming that the PR makes a change to user functionality (ie that you can newly exclude the Dockerfile from the context from which you ADD) - and that is something the users need to be told about - they're not going to read this PR, nor are they able to wade through the discussion.

thaJeztah · 2014-10-29T06:40:14Z

Think adding it to a changelog / what's new section and describe the slightly different behavior would make sense. Think Fred was working on such section?

duglin · 2014-10-29T17:14:38Z

Yup, this one is hard because I think the current behavior is incorrect and this just fixes it. If anything the docs right now should mention that Dockerfile isn't allowed in there, then I could have removed that text as part of this PR. And I'm not sure we want to add text to say "we used to x but now we do y" when "y" is what we should have been doing all along - plus when do we remove a line like that?

@SvenDowideit not sure how you'd like me to proceed on this one.

thaJeztah · 2014-10-29T18:38:21Z

Well, it's a "feature", when described in a positive note;

The .dockerignore file now allows you to ignore the Dockerfile and .dockerignore.

I agree that it shouldn't be added to the "main" docs, because this limitation was never mentioned there in the first place.

SvenDowideit · 2014-10-30T01:23:33Z

I would in fact add @thaJeztah 's sentence as a note in the main docs for .dockerignore.

duglin · 2014-10-30T02:08:58Z

@SvenDowideit ok I added a note.

abevoelker · 2014-10-30T19:02:19Z

Just wanted to say I'm really looking forward to this, thanks for working on it! It's brutal at the moment having to wait several minutes on asset precompilation (Rails) on every build when I'm just tweaking the Dockerfile.

duglin · 2014-10-30T20:01:25Z

@tianon @erikh any thoughts on my comment above? I'm not comfortable with introducing daemon-side .dockerignore processing in general as I think that's a much bigger issue - in particular for folks who are using the REST API and might be surprised by the deamon suddenly removing files from their build-context. If we really want to do this I would prefer to do it under a different PR so we can have that discussion isolated from, what I consider, a fix to the current .dockerignore support.

SvenDowideit · 2014-10-31T02:08:20Z

Doc LGTM - @fredlf @jamtur01

especially as i didn't even realise that i could use it for .dockerignore! - and as @abevoelker points out, its really painful to watch your entire context cache invalidated due to Dockerfile / .dockerignore!

erikh · 2014-10-31T02:15:05Z

I'm ok with this patch and the parts I maintain LGTM. I'm going to leave it up to the rest of the crew to decide the rest.

/cc @tiborvass @unclejack @jfrazelle

tiborvass · 2014-11-03T20:58:11Z

I like the hacky solution of checking .dockerignore on the server side.
@duglin needs a rebase :)

tianon · 2014-11-03T21:35:49Z

Fair enough, although this makes the same change just with much smaller
scope (so API users are less likely to notice the change).

fredlf · 2014-11-03T22:49:23Z

docs/sources/reference/builder.md

It might be nice to have an example or explanation of when/why you would want to do this. It's kind of counter-intuitive, so an example might help clarify.

Not a requirement, just a suggestion.

fredlf · 2014-11-03T22:51:57Z

LGTM. We can't rewrite the past, but we can at least document the present!

My suggestion is just that, a suggestion. We can merge this without it from the docs POV.

duglin · 2014-12-11T20:47:59Z

@vbatts thanks! Do we need more reviewers?

erikh · 2014-12-11T20:50:42Z

I LGTM'd a while ago but can review again in a bit.

duglin · 2014-12-12T14:44:49Z

Rebased after some changes were made to the tar stuff.

erikh · 2014-12-15T19:53:20Z

daemon/graphdriver/aufs/aufs.go

why is this patch in here? bad rebase?

note, per vbatts suggestion, I renamed Excludes to ExcludesPatterns - which is why this diff shows up.

Ah ok. If @vbatts asked you do this, I won’t get in the way. LGTM.

On Dec 15, 2014, at 11:56 AM, Doug Davis notifications@github.com wrote:

In daemon/graphdriver/aufs/aufs.go #8748 (diff):

@@ -300,8 +300,8 @@ func (a *Driver) Put(id string) {
func (a *Driver) Diff(id, parent string) (archive.Archive, error) {
// AUFS doesn't need the parent layer to produce a diff.
return archive.TarWithOptions(path.Join(a.rootPath(), "diff", id), &archive.TarOptions{

Compression: archive.Uncompressed,

Excludes: []string{".wh..wh.*"},

Compression: archive.Uncompressed,

ExcludePatterns: []string{".wh..wh.*"},
note, per vbatts suggestion, I renamed Excludes to ExcludesPatterns - which is why this diff shows up.

—
Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/8748/files#r21851657.

pinging @crosbymichael @jessfraz for merge

On Dec 15, 2014, at 11:56 AM, Doug Davis notifications@github.com wrote:

In daemon/graphdriver/aufs/aufs.go #8748 (diff):

@@ -300,8 +300,8 @@ func (a *Driver) Put(id string) {
func (a *Driver) Diff(id, parent string) (archive.Archive, error) {
// AUFS doesn't need the parent layer to produce a diff.
return archive.TarWithOptions(path.Join(a.rootPath(), "diff", id), &archive.TarOptions{

Compression: archive.Uncompressed,

Excludes: []string{".wh..wh.*"},

Compression: archive.Uncompressed,

ExcludePatterns: []string{".wh..wh.*"},
note, per vbatts suggestion, I renamed Excludes to ExcludesPatterns - which is why this diff shows up.

—
Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/8748/files#r21851657.

erikh · 2014-12-15T19:54:44Z

LGTM minus the AUFS fixes. They don't belong here.

ewindisch · 2014-12-15T20:25:56Z

The tarsum stuff LGTM. Haven't looked at the AUFS bits or why it's here.

vbatts · 2014-12-15T21:23:58Z

@erikh and @ewindisch the AUFS lines are just a variable name change, for clarification.

duglin · 2014-12-19T11:54:08Z

any more comments on this one?

vbatts · 2015-01-05T20:57:04Z

honestly this looks to have plenty of LGTMs. @jfrazelle care to once over and pull the trigger?

jessfraz · 2015-01-05T21:01:54Z

builder/evaluator.go

should we not be returning errors here?

fileutils.Matches only returns an error from filepath.Matches, which only has an error if the pattern is bad, but we know the pattern is good.

i meant an error on the os.Remove()

Move along, nothing to see here :)

It would be really really really strange to get an error here... so maybe.

I can add it if you guys want but it would be pretty odd.

If .dockerignore mentions either then the client will send them to the daemon but the daemon will erase them after the Dockerfile has been parsed to simulate them never being sent in the first place. an events test kept failing for me so I tried to fix that too Closes moby#8330 Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2015-01-06T18:59:10Z

@crosbymichael I refactored the reading of the dockerfile into its own function and moved the cleanup stuff into there so it shouldn't look so odd to you any more. Have a look and let me know.

crosbymichael · 2015-01-06T21:45:09Z

@duglin awesome, thanks

crosbymichael · 2015-01-06T21:47:39Z

LGTM

Have .dockerignore support Dockerfile/.dockerignore

duglin force-pushed the Issue8330 branch from d6d2f31 to bb984f7 Compare October 24, 2014 04:53

duglin force-pushed the Issue8330 branch from bb984f7 to eb2d50c Compare October 24, 2014 23:41

erikh reviewed Oct 27, 2014
View reviewed changes

builder/evaluator.go Outdated

Copy link

Contributor

erikh Oct 27, 2014

Choose a reason for hiding this comment

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

this should probably be in its own function

duglin force-pushed the Issue8330 branch from eb2d50c to 3db741d Compare October 27, 2014 14:40

tianon reviewed Oct 28, 2014
View reviewed changes

SvenDowideit added the /project/doc label Oct 29, 2014

duglin force-pushed the Issue8330 branch from 3db741d to 62a743f Compare October 30, 2014 02:08

fredlf reviewed Nov 3, 2014
View reviewed changes

duglin force-pushed the Issue8330 branch from fc6ce46 to bf03c4a Compare December 12, 2014 14:28

erikh reviewed Dec 15, 2014
View reviewed changes

crosbymichael added this to the 1.5.0 milestone Dec 17, 2014

duglin mentioned this pull request Dec 17, 2014

Proposal: docker build should allow arbitrary Dockerfile path #7284

Closed

thaJeztah mentioned this pull request Dec 30, 2014

Proposal: Remove dockerfile from context #8418

Closed

jessfraz reviewed Jan 5, 2015
View reviewed changes

duglin force-pushed the Issue8330 branch from bf03c4a to 6d801a3 Compare January 6, 2015 18:58

crosbymichael added a commit that referenced this pull request Jan 6, 2015

Merge pull request #8748 from duglin/Issue8330

6d78013

Have .dockerignore support Dockerfile/.dockerignore

crosbymichael merged commit 6d78013 into moby:master Jan 6, 2015

duglin mentioned this pull request Jan 7, 2015

Allow for Dockerfile to be named something else. #9707

Merged

LK4D4 mentioned this pull request Jan 22, 2015

.dockerignore doesn't used during docker hub automated build #9455

Closed

duglin deleted the Issue8330 branch January 22, 2015 22:26

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

tianon mentioned this pull request Jun 12, 2015

Let "docker build -" respect .dockerignore #13919

Closed

thaJeztah added area/builder Build kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. labels Dec 2, 2019

danvergara mentioned this pull request Feb 29, 2024

Fix Dockerfile not located when added to dockerignore testcontainers/testcontainers-go#2272

Merged

Conversation

duglin commented Oct 24, 2014

Uh oh!

cpuguy83 commented Oct 24, 2014

Uh oh!

duglin commented Oct 24, 2014

Uh oh!

duglin commented Oct 24, 2014

Uh oh!

thaJeztah commented Oct 25, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Oct 27, 2014

Uh oh!

duglin commented Oct 27, 2014

Uh oh!

erikh commented Oct 27, 2014

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvenDowideit commented Oct 29, 2014

Uh oh!

duglin commented Oct 29, 2014

Uh oh!

SvenDowideit commented Oct 29, 2014

Uh oh!

thaJeztah commented Oct 29, 2014

Uh oh!

duglin commented Oct 29, 2014

Uh oh!

thaJeztah commented Oct 29, 2014

Uh oh!

SvenDowideit commented Oct 30, 2014

Uh oh!

duglin commented Oct 30, 2014

Uh oh!

abevoelker commented Oct 30, 2014

Uh oh!

duglin commented Oct 30, 2014

Uh oh!

SvenDowideit commented Oct 31, 2014

Uh oh!

erikh commented Oct 31, 2014

Uh oh!

tiborvass commented Nov 3, 2014

Uh oh!

tianon commented Nov 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredlf commented Nov 3, 2014

Uh oh!

duglin commented Dec 11, 2014

Uh oh!

erikh commented Dec 11, 2014

Uh oh!

duglin commented Dec 12, 2014

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!

erikh commented Dec 15, 2014

Uh oh!

ewindisch commented Dec 15, 2014