Fix .dockerignore resolution when using multiple Dockerfiles #863
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
| func TestDockerignoreNoMappingYAML(t *testing.T) { | ||
| cwd, err := os.Getwd() | ||
| if !assert.NoError(t, err) { | ||
| t.FailNow() | ||
| } | ||
| // we expect this test to succeed, as we test that the ignore.txt file does in fact _not_ get ignored | ||
| // the ignore.txt file does not get ignored, as .dockerignore does not map to Mockerfile. | ||
| // The RUN command in Mockerfile therefore succeeds. | ||
| integration.ProgramTest(t, &integration.ProgramTestOptions{ | ||
| Dir: path.Join(cwd, "test-dockerfile", "dockerignore-no-mapping"), | ||
| Quick: true, | ||
| SkipRefresh: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
This was testing the bug, so I removed it.
| func TestDockerignoreDefaultFailYAML(t *testing.T) { | ||
| cwd, err := os.Getwd() | ||
| if !assert.NoError(t, err) { | ||
| t.FailNow() | ||
| } | ||
|
|
||
| integration.ProgramTest(t, &integration.ProgramTestOptions{ | ||
| Dir: path.Join(cwd, "test-dockerfile", "dockerignore-default-fail"), | ||
| Quick: true, | ||
| SkipRefresh: true, | ||
| ExpectFailure: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
This was effectively the same as the dockerignore-default test. I removed it and consolidated its assertion with the other test by adding explicit existence/non-existence checks in the Dockerfile.
| RUN [ -f "app.txt" ] | ||
|
|
||
| RUN [ ! -f "ignore.txt" ] |
There was a problem hiding this comment.
Added these here and elsewhere to ensure we always build the image with the expected files.
There was a problem hiding this comment.
This looks good!
Can you add a bit more detail to the pull request description for posterity? A quick description of the situation where a codebase is "using multiple Dockerfiles" and how those map to .dockerignores, and a summary of how this PR fixes detection and fallback (the code comment is great but having it in the commit message is also valuable IMO)
| } | ||
| defer f.Close() | ||
|
|
||
| ignorePatterns, err := dockerignore.ReadAll(f) |
There was a problem hiding this comment.
I presume we return on the first found .dockerignore file because we're relying on the order in which the particular .dockerignores get appended to paths (specific first, root-level second) - is there a way to make that precedence more explicit?
There was a problem hiding this comment.
Yes, exactly. I did some spelunking and confirmed we should always check for the presence of <Dockerfile>.dockerignore regardless of the file's name. So that makes this logic a little more explicit -- there are always 2 things to check and in a particular order.
This pushes
.dockerignorepath resolution down intogetIgnorePatternsso it can juggle the multiple candidate ignore-files correctly.Resolution follows the behavior described here:
In other words, we look for a
<Dockerfile>.dockerignorefile sitting next to<Dockerfile>, and if that doesn't exist we fall back on the.dockerignoreat the root of our build context.Fixes #675