Add support for comments in .dockerignore #23111

yongtang · 2016-05-31T01:28:12Z

- What I did

This fix tries to address the issue raised in #20083 where comment is not supported in .dockerignore.

- How I did it

This fix updated the processing of .dockerignore so that any lines starting with # are ignored, which is similiar to the behavior of .gitignore.

Related documentation has been updated.

- How to verify it

Additional tests have been added to cover the changes.

- Description for the changelog

Add support for comments (lines starting with #) in .dockerignore.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #20083.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

yongtang · 2016-05-31T01:31:08Z

cc @duglin @thaJeztah.

LK4D4 · 2016-05-31T05:28:35Z

Probably people will expect inline comments to work as well, but I'm not sure.

vdemeester · 2016-05-31T06:03:15Z

@LK4D4 yeah I guess, but git didn't support inline coment for a while, depending on the diffculty, I think having only lines comment is a good start.

thaJeztah · 2016-05-31T09:11:18Z

builder/dockerignore/dockerignore.go

@@ -21,7 +21,8 @@ func ReadAll(reader io.ReadCloser) ([]string, error) {

 	for scanner.Scan() {
 		pattern := strings.TrimSpace(scanner.Text())
-		if pattern == "" {
+		// Blank lines or lines starting with # are ignored.
+		if pattern == "" || strings.HasPrefix(pattern, "#") {


the strings.HasPrefix(.. should be before we trim spaces, otherwise we're not consistent with the description;

# this is a comment # this is now also considered a comment

I'm ok with either. I always thought it was odd that Dockerfiles didn't support spaces before comments.

Thanks @thaJeztah @duglin I updated the pull request. Now only lines starting with # (at position 1) are considered as comments.

thaJeztah · 2016-05-31T09:26:31Z

I'm fine with having just this as a start; Dockerfile's also only support comments if they start at position 1. Supporting inline comments doesn't look complicated if we accept that paths cannot contain #;

pattern = strings.SplitN(pattern, "#", 2)[0]

@vdemeester @LK4D4 wdyt?

justincormack · 2016-05-31T10:54:15Z

Supporting inline comments seems a bad thing for something as simple as this, let's just do the simplest thing and accept comments only if the first character is #.

thaJeztah · 2016-05-31T11:00:29Z

Ok, let's go with that option for now (as mentioned, it's also consistent with the Dockerfile syntax).

Should we address #23111 (comment)? I'm a bit in doubt as it's inconsistent with the way Dockerfiles are parsed, although the documentation mentions;

A preprocessing step removes leading and trailing whitespace

So I'm a bit in doubt as to what the most sensible approach is

justincormack · 2016-05-31T11:04:35Z

I think the preprocessing step should only take place on non comment lines, so the description should be first comments are removed then the current processing takes place.

duglin · 2016-05-31T11:12:35Z

integration-cli/docker_cli_build_test.go

+# because we have git ;-)
+# The above comment is from #20083
+./foo
+#dir1//foo


why the double / ?
Let's keep the test simple and just make it #dir1/foo to make sure its really ignored.
And if you do end up supporting spaces before "#" then please include that in the test too.
And you should probably add one where "#" is in the middle of the line - to make sure we don't read it as a comment.

Thanks @duglin. The pull request has been updated with simplified filename. Also two files starting with # (one #1 and another #2) have been added to test # in the middle case.

yongtang · 2016-05-31T14:26:37Z

Thanks @LK4D4 @vdemeester @duglin @thaJeztah @justincormack for the review. The pull request has been updated.

Now only lines starting with # (at position 1) are considered as comments and are ignored.

In addition, I updated the test by adding a couple of files with file name #1 and #2. This is to test the case of # in the middle of the line (which is not considered as comment and is not ignored therefore).

Please let me know if there are any issues.

duglin · 2016-05-31T14:35:26Z

builder/dockerignore/dockerignore.go

@@ -20,6 +20,10 @@ func ReadAll(reader io.ReadCloser) ([]string, error) {
 	var excludes []string

 	for scanner.Scan() {
+		// Lines starting with # (comments) are ignored before processing
+		if strings.HasPrefix(scanner.Text(), "#") {


its a minor performance (stylistic) thing, but can you change this so that its:

pattern := scanner.Text() if strings.HasPrefix(pattern, "#") { ... } pattern = strings.TrimSpace(pattern)

So that we're not calling scanner.Text() more than once?

Thanks @duglin the pull request has been updated.

yongtang · 2016-06-01T16:22:16Z

The failure in windowsTP5 seems to be real. I would guess it is related to the (ls /tmp/#1) and Windows may not recognize #.

I don't have a setup in windowsTP5 locally yet. But I will try to do some investigation on this issue.

15:29:05 FAIL: docker_cli_build_test.go:6745: DockerSuite.TestBuildDockerignoreComment
15:29:05 
15:29:05 docker_cli_build_test.go:6774:
15:29:05     c.Fatal(err)
15:29:05 ... Error: failed to build the image: Sending build context to Docker daemon 5.632 kB

15:29:05 Step 1 : FROM busybox
15:29:05  ---> 70c337fe05f8
15:29:05 Step 2 : ADD . /tmp/
15:29:05  ---> b47bf1e14ad7
15:29:05 Removing intermediate container bf8b6e903af4
15:29:05 Step 3 : RUN sh -c "(! ls /tmp/foo) && (! ls /tmp/foo2) && (ls /tmp/dir1/foo) && (ls /tmp/#1) && (! ls /tmp/#2)"
15:29:05  ---> Running in 0277780112c5
15:29:05 ls: /tmp/foo2: No such file or directory
15:29:05 The command 'cmd /S /C sh -c "(! ls /tmp/foo) && (! ls /tmp/foo2) && (ls /tmp/dir1/foo) && (ls /tmp/#1) && (! ls /tmp/#2)"' returned a non-zero code: 255
15:29:05 
15:29:05

yongtang · 2016-06-02T13:39:35Z

The issue in TP5 should have been fixed.

duglin · 2016-06-02T13:41:47Z

Where did janky go?

thaJeztah · 2016-06-02T13:46:23Z

Took a day off, possibly Leeroy (@GordonTheTurtle) didn't get a call. I'll rebuild

duglin · 2016-06-02T13:47:35Z

LGTM if janky reappears and is ok.

yongtang · 2016-06-02T13:53:35Z

Thanks @thaJeztah @duglin.

I also noticed that documentation always fails, not just for this PR but possibly for any PR related to documentation change:

#23192
#23195
#23107

And the documentation error always points to

ERROR: 2016/06/02 05:39:15 content_renderer.go:56: LinkResolver: No page found for "/docker-trusted-registry/overview.md" on page "registry/index.md".

I am not familiar with how Docker's Jenkins build for documentation works so not sure how to fix it.

thaJeztah · 2016-06-02T13:56:07Z

@yongtang yes, It looks like the CI settings changed and now fail instead of "warn" for some errors. That link error looks to be in another repository, so should probably be fixed in the "distribution" repository.

This fix tries to address the issue raised in moby#20083 where comment is not supported in `.dockerignore`. This fix updated the processing of `.dockerignore` so that any lines starting with `#` are ignored, which is similiar to the behavior of `.gitignore`. Related documentation has been updated. Additional tests have been added to cover the changes. This fix fixes moby#20083. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-06-03T02:30:30Z

Thanks @thaJeztah @duglin. The pull request has been rebased and the documentation issue seems to have been fixed. Please let me know if there are any other issues.

vdemeester · 2016-06-03T06:14:29Z

LGTM 🐮

vdemeester · 2016-06-03T06:16:19Z

Moving to doc review /cc @thaJeztah

thaJeztah · 2016-06-03T12:21:42Z

docs LGTM

gccgo timed out

vdemeester · 2016-06-03T12:23:03Z

docs LGTM it is, merging 👼

GordonTheTurtle added area/distribution status/0-triage labels May 31, 2016

yongtang mentioned this pull request May 31, 2016

.dockerignore does not fully ignore comments #20083

Closed

LK4D4 added status/2-code-review and removed status/0-triage labels May 31, 2016

thaJeztah reviewed May 31, 2016
View reviewed changes

thaJeztah added this to the 1.12.0 milestone May 31, 2016

thaJeztah added the impact/changelog label May 31, 2016

duglin reviewed May 31, 2016
View reviewed changes

yongtang force-pushed the 20083-dockerignore-comment branch from 33cfd76 to 1088c92 Compare May 31, 2016 14:18

duglin reviewed May 31, 2016
View reviewed changes

yongtang force-pushed the 20083-dockerignore-comment branch from 1088c92 to 414d175 Compare May 31, 2016 15:01

yongtang force-pushed the 20083-dockerignore-comment branch 6 times, most recently from 4e83bc2 to 162aecb Compare June 2, 2016 04:25

yongtang force-pushed the 20083-dockerignore-comment branch 3 times, most recently from 1f60786 to babf71a Compare June 2, 2016 13:37

yongtang force-pushed the 20083-dockerignore-comment branch from babf71a to 8913dac Compare June 3, 2016 02:11

vdemeester added status/3-docs-review and removed status/2-code-review labels Jun 3, 2016

vdemeester merged commit 9c1278b into moby:master Jun 3, 2016

yongtang deleted the 20083-dockerignore-comment branch June 3, 2016 12:37

thaJeztah mentioned this pull request Jun 3, 2016

Flaky test: TestBuildDockerignoreComment #23246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for comments in .dockerignore #23111

Add support for comments in .dockerignore #23111

yongtang commented May 31, 2016

yongtang commented May 31, 2016

LK4D4 commented May 31, 2016

vdemeester commented May 31, 2016

thaJeztah May 31, 2016

duglin May 31, 2016

yongtang May 31, 2016

thaJeztah commented May 31, 2016

justincormack commented May 31, 2016

thaJeztah commented May 31, 2016

justincormack commented May 31, 2016

duglin May 31, 2016

yongtang May 31, 2016

yongtang commented May 31, 2016

duglin May 31, 2016

yongtang May 31, 2016

yongtang commented Jun 1, 2016

yongtang commented Jun 2, 2016

duglin commented Jun 2, 2016

thaJeztah commented Jun 2, 2016

duglin commented Jun 2, 2016

yongtang commented Jun 2, 2016

thaJeztah commented Jun 2, 2016

yongtang commented Jun 3, 2016

vdemeester commented Jun 3, 2016

vdemeester commented Jun 3, 2016

thaJeztah commented Jun 3, 2016

vdemeester commented Jun 3, 2016

Add support for comments in .dockerignore #23111

Add support for comments in .dockerignore #23111

Conversation

yongtang commented May 31, 2016

yongtang commented May 31, 2016

LK4D4 commented May 31, 2016

vdemeester commented May 31, 2016

thaJeztah May 31, 2016

Choose a reason for hiding this comment

duglin May 31, 2016

Choose a reason for hiding this comment

yongtang May 31, 2016

Choose a reason for hiding this comment

thaJeztah commented May 31, 2016

justincormack commented May 31, 2016

thaJeztah commented May 31, 2016

justincormack commented May 31, 2016

duglin May 31, 2016

Choose a reason for hiding this comment

yongtang May 31, 2016

Choose a reason for hiding this comment

yongtang commented May 31, 2016

duglin May 31, 2016

Choose a reason for hiding this comment

yongtang May 31, 2016

Choose a reason for hiding this comment

yongtang commented Jun 1, 2016

yongtang commented Jun 2, 2016

duglin commented Jun 2, 2016

thaJeztah commented Jun 2, 2016

duglin commented Jun 2, 2016

yongtang commented Jun 2, 2016

thaJeztah commented Jun 2, 2016

yongtang commented Jun 3, 2016

vdemeester commented Jun 3, 2016

vdemeester commented Jun 3, 2016

thaJeztah commented Jun 3, 2016

vdemeester commented Jun 3, 2016