Add support for exclusion rules in dockerignore #12245

buddhamagnet · 2015-04-09T19:08:04Z

Parallel pull request to #11823 (since closed).

buddhamagnet · 2015-04-09T19:11:47Z

Amended last commit, now signed.

icecrime · 2015-04-09T20:25:06Z

@buddhamagnet Thanks! There's a formatting problem to one of your files:

These files are not properly gofmt'd:
 - pkg/fileutils/fileutils_test.go

Please reformat the above files using "gofmt -s -w" and commit the result.

Can you please fix it?

buddhamagnet · 2015-04-10T12:52:16Z

Done, thanks. Was a trailing newline.

cpuguy83 · 2015-04-10T15:12:56Z

Can you squash and make sure the commit is signed? Tests aren't running because your last commit isn't signed. These should be 1 commit anyway.
Lmk know if you need any help with this!

Thanks!

buddhamagnet · 2015-04-10T16:11:53Z

Ah sorry I committed at work and hadn't updated my local git config. On it.

buddhamagnet · 2015-04-10T16:16:09Z

Done that, apologies. Don't have time to do the squash now will do later, but the commits are now both signed.

thaJeztah · 2015-04-11T00:26:24Z

I see the tests failed

buddhamagnet · 2015-04-11T07:27:00Z

My bad, had added a failing test for the single exclamation point case. Will complete that locally, apologies.

buddhamagnet · 2015-04-11T08:10:12Z

Now rebased against master and squashed.

thaJeztah · 2015-04-11T09:23:30Z

Sorry, @buddhamagnet looks like it still failed; https://jenkins.dockerproject.com/job/Docker-PRs/5544/console

(The Windows fail doesn't look like your fault)

duglin · 2015-04-11T12:10:26Z

pkg/fileutils/fileutils.go

+				matched = false
+				continue
+			}
+
 			if filepath.Clean(relFilePath) == "." {


Should this check for "." apply to the ! case too?
If so, you can move this up before the "if negative" line and then the line below becomes:

matched = !negative

and you can remove the "if negative" check.

Yep - done.

duglin · 2015-04-11T12:23:31Z

integration-cli/docker_cli_build_test.go

+	dockerfile := `
+        FROM busybox
+        ADD . /bla
+               RUN [[ -f /bla/src/x.go ]]


why are these indented more?
edit: weird the test before has that too, must be tabs mucking with it.

buddhamagnet · 2015-04-11T13:29:41Z

@thaJeztah apologies I didn't write the integration part. I was on this with @omeid but can't seem to reach him (@omeid hope all is well) so I created a fresh PR from master to move things forward. Will work on all this early next week and hopefully get it into shape.

Out of interest @duglin @cpuguy83 why the insistence on squashing all commits during the review process. My common process is to rebase and squash on approval before merge. Just curious.

thaJeztah · 2015-04-11T13:32:22Z

@buddhamagnet no problem; on the squashing: all tests are run for each individual commit, so this can result in longer test-times and failing tests if commits are not complete

duglin · 2015-04-11T13:38:17Z

@buddhamagnet I don't care much about having one vs many commits during the review cycle, as long as its done before its merged. The thing I worry about is that reviewers may jump the gun and merge before you do your final "clean-up" and squash them down to one so I prefer to have the PR in as "final" a state as possible.

buddhamagnet · 2015-04-11T13:39:10Z

@thaJeztah so can I just commit --amend on each iteration and force push? Is that an acceptable workflow?

duglin · 2015-04-11T13:39:28Z

@buddhamagnet yup - that's what I do.

calavera · 2015-04-27T16:35:33Z

Code and design LGTM. Moving to docs review 🎉

moxiegirl · 2015-04-27T17:20:18Z

docs/sources/reference/builder.md

@@ -46,6 +46,8 @@ the files needed for building that Dockerfile to that directory. To further spee
 build, you can exclude files and directories by adding a `.dockerignore` file to the same
 directory.

+The `.dockerignore` file also supports exclusion patterns (as per `.gitignore` files).


Combine this with the paragraph above. Also, there is no need to talk about the patterns in this paragraph, there is a section further down the page with an example .dockerignore --- refer to that instead

Change ## The .dockeringore File in this document to Ignore files and directories

Then

---> https://gist.github.com/moxiegirl/2a8f455f0e7836377457

In most cases, it's best to put each Dockerfile in an empty directory. Then,
only add the files needed for building the Dockerfile to the directory. To
increase the build's performance, you can exclude files and directories by
adding a .dockerignore file to the directory. This file supports the same exclusion patterns as .gitignore files. See the information about how to ignore files and directories on this page.

buddhamagnet · 2015-04-27T17:49:47Z

@moxiegirl amended and pushed.

moxiegirl · 2015-04-27T18:19:13Z

@buddhamagnet You are super quick. I noticed a few problems in the way we document this file. Unfortunately, after my initial comments. My apologies. I'll sort this out and post a Gist with the changes -- if you are willing to make them.

buddhamagnet · 2015-04-27T18:56:34Z

@moxiegirl go for it.

moxiegirl · 2015-04-28T13:58:51Z

Hi @buddhamagnet -- I ended up creating a Gist containing a patch here: https://gist.github.com/moxiegirl/d0ec84384df2763b82fd

Just download it and apply it to your branch.

buddhamagnet · 2015-04-28T14:55:10Z

@moxiegirl output from dummy run:

git apply --stat moxie.patch
 docs/sources/articles/dockerfile_best-practices.md |   12 +-
 docs/sources/reference/builder.md                  |   97 +++++++++++++-------
 docs/sources/reference/commandline/cli.md          |   68 +++++---------

output from dummy run:

git apply --check moxie.patch
error: patch failed: docs/sources/articles/dockerfile_best-practices.md:32
error: docs/sources/articles/dockerfile_best-practices.md: patch does not apply
error: patch failed: docs/sources/reference/builder.md:41
error: docs/sources/reference/builder.md: patch does not apply
error: patch failed: docs/sources/reference/commandline/cli.md:674
error: docs/sources/reference/commandline/cli.md: patch does not apply

Looks like that patch won't apply cleanly?

buddhamagnet · 2015-04-28T15:09:38Z

@moxiegirl also, it looks like a lot of these changes pertain to the documentation of the dockerignore file, rather than changes introduced by this feature. Could this not be added in a separate pull request (just asking).

camilonova · 2015-04-28T15:20:43Z

Man, is so hard to make this happen, should be easier. @buddhamagnet thank you for keeping pushing forward.

buddhamagnet · 2015-04-28T15:22:15Z

@camilonova @moxiegirl it's not hard at all, I'm happy ;)

moxiegirl · 2015-04-28T16:01:58Z

@buddhamagnet Normally, I wouldn't ask for this many changes which is why I checked with you yesterday to see if you are willing. Since all your documentation changes were around the .dockerignore file, I think it is appropriate to clean up the whole slew with the same PR.

BTW, thank you for being so patient. I've been tracking you through the process on this PR. You've been great.

buddhamagnet · 2015-04-28T16:57:34Z

@moxiegirl no worries at all. Easier for me to give you remote access and you issue a PR?

buddhamagnet · 2015-04-28T17:25:41Z

@moxiegirl may well be because I rebased against master and force pushed just before I got the patch. We can either try a new patch or your can issue a PR against my branch, whichever is easiest and spreads more rainbows and unicorns.

moxiegirl · 2015-04-28T17:38:32Z

Sure thing. Here you go: https://github.com/buddhamagnet/docker/pull/2 @buddhamagnet

Signed-off-by: Dave Goodchild <buddhamagnet@gmail.com>

buddhamagnet · 2015-04-28T17:59:25Z

@moxiegirl yeah some conflicts there, resolved them, merged and rebased my own branch to combine it all into the original commit etc. Hope it's good.

buddhamagnet · 2015-04-28T18:22:29Z

@moxiegirl all incorporated, all tests green, one single shining commit.

Add support for exclusion rules in dockerignore

calavera · 2015-04-28T18:32:33Z

🎉

thaJeztah · 2015-04-28T18:34:48Z

\o/ awesome! Thanks for the contribution!

buddhamagnet · 2015-04-28T18:59:44Z

HOLY COW PARTY TIME!!!

buddhamagnet · 2015-04-28T19:04:15Z

@duglin you needed some credit on that commit.

duglin · 2015-04-29T00:43:39Z

@buddhamagnet just glad to help move it along

GordonTheTurtle added status/0-triage dco/no labels Apr 9, 2015

buddhamagnet mentioned this pull request Apr 9, 2015

.dockerignore ignore support. Fixes #9171 #11823

Closed

icecrime added status/1-design-review and removed dco/no status/0-triage labels Apr 9, 2015

duglin reviewed Apr 11, 2015
View reviewed changes

duglin added status/2-code-review and removed status/1-design-review labels Apr 11, 2015

duglin reviewed Apr 11, 2015
View reviewed changes

calavera added status/3-docs-review and removed status/2-code-review labels Apr 27, 2015

moxiegirl reviewed Apr 27, 2015
View reviewed changes

duglin mentioned this pull request Apr 28, 2015

Add coverage on pkg/fileutils #12840

Merged

add support for exclusion rules in dockerignore

6fd8e48

Signed-off-by: Dave Goodchild <buddhamagnet@gmail.com>

jessfraz pushed a commit that referenced this pull request Apr 28, 2015

Merge pull request #12245 from buddhamagnet/dockerignore-ignores

4676ff3

Add support for exclusion rules in dockerignore

jessfraz merged commit 4676ff3 into moby:master Apr 28, 2015

buddhamagnet deleted the dockerignore-ignores branch April 28, 2015 19:02

duglin mentioned this pull request May 18, 2015

.dockerignore should a support including ignored content #9171

Closed

thaJeztah mentioned this pull request Aug 20, 2015

Ignore file extension in all folders with .dockerignore #13113

Closed

Add support for exclusion rules in dockerignore #12245

Add support for exclusion rules in dockerignore #12245

Conversation

buddhamagnet commented Apr 9, 2015

buddhamagnet commented Apr 9, 2015

icecrime commented Apr 9, 2015

buddhamagnet commented Apr 10, 2015

cpuguy83 commented Apr 10, 2015

buddhamagnet commented Apr 10, 2015

buddhamagnet commented Apr 10, 2015

thaJeztah commented Apr 11, 2015

buddhamagnet commented Apr 11, 2015

buddhamagnet commented Apr 11, 2015

thaJeztah commented Apr 11, 2015

duglin Apr 11, 2015

Choose a reason for hiding this comment

buddhamagnet Apr 12, 2015

Choose a reason for hiding this comment

duglin Apr 11, 2015

Choose a reason for hiding this comment

buddhamagnet commented Apr 11, 2015

thaJeztah commented Apr 11, 2015

duglin commented Apr 11, 2015

buddhamagnet commented Apr 11, 2015

duglin commented Apr 11, 2015

calavera commented Apr 27, 2015

moxiegirl Apr 27, 2015

Choose a reason for hiding this comment

buddhamagnet commented Apr 27, 2015

moxiegirl commented Apr 27, 2015

buddhamagnet commented Apr 27, 2015

moxiegirl commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

camilonova commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

moxiegirl commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

moxiegirl commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

calavera commented Apr 28, 2015

thaJeztah commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

buddhamagnet commented Apr 28, 2015

duglin commented Apr 29, 2015