fix: preserve Dockerfile-specific .dockerignore files#1206
fix: preserve Dockerfile-specific .dockerignore files#1206lawrence3699 wants to merge 1 commit intodevcontainers:mainfrom
Conversation
|
@lawrence3699 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
This PR fixes devcontainer builds so Dockerfile-specific ignore files (e.g. dev.Dockerfile.dockerignore) continue to apply when the CLI rewrites the Dockerfile into a temp folder (e.g. Dockerfile-with-features), aligning behavior with Docker’s documented .dockerignore lookup rules.
Changes:
- Copy
<source Dockerfile>.dockerignorealongside the generatedDockerfile-with-featuresfor direct Dockerfile builds. - Apply the same
.dockerignorepreservation for compose-backed builds when generating the compose build override Dockerfile. - Add a regression test covering a compose build that uses a non-standard Dockerfile name.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/spec-node/dockerignoreUtils.ts | Adds helper to copy Dockerfile-specific .dockerignore to the generated Dockerfile location. |
| src/spec-node/singleContainer.ts | Calls the helper when generating Dockerfile-with-features for direct Dockerfile builds. |
| src/spec-node/dockerCompose.ts | Calls the helper for compose builds after writing Dockerfile-with-features. |
| src/test/dockerignore.test.ts | Adds regression coverage ensuring the generated compose Dockerfile has the sibling .dockerignore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fakeDocker = path.join(root, 'fake-docker'); | ||
| await fs.writeFile(fakeDocker, `#!/bin/sh | ||
| set -eu |
There was a problem hiding this comment.
This test creates a fake docker executable as a #!/bin/sh script and relies on chmod + shebang execution. That approach will fail on Windows when running tests via Node’s child_process.spawn (even under Git Bash), because Windows cannot directly execute shebang scripts without an explicit interpreter. Consider generating a cross-platform fake (e.g. a small Node.js .js script invoked via node, or a .cmd wrapper on win32), or skipping this test on process.platform === 'win32'.
| const { context, dockerfilePath, target } = serviceInfo.build; | ||
| const resolvedDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); | ||
| const originalDockerfile = (await cliHost.readFile(resolvedDockerfilePath)).toString(); | ||
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); |
There was a problem hiding this comment.
sourceDockerfilePath is resolved using Node's path.resolve(...) even though the rest of the compose path handling uses cliHost.path (which can be posix/win32 depending on the host). This can produce incorrect paths when the CLI host path semantics differ from the local Node process (e.g. WSL/remote scenarios), and would prevent reading/copying the Dockerfile-specific .dockerignore. Use cliHost.path.resolve(context, dockerfilePath) (and avoid mixing path and cliHost.path) to keep resolution consistent.
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); | |
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : cliHost.path.resolve(context, dockerfilePath); |
Fixes #969
Summary
.dockerignorefile next to the generatedDockerfile-with-featuresWhy this is correct
When the CLI rewrites a Dockerfile into a temp folder, Docker stops seeing the original
<Dockerfile>.dockerignorefile because it only looks for Dockerfile-specific ignore files next to the Dockerfile it is actually building. Copying that sibling file alongside the generated Dockerfile preserves Docker's existing lookup and precedence rules.Validation
ts-noderepro aroundbuildAndExtendDockerCompose: before the patch the generated temp Dockerfile had no sibling.dockerignore; after the patch it does, with the original contents preserved./node_modules/.bin/tsc -b./node_modules/.bin/eslint src/spec-node/dockerCompose.ts src/spec-node/singleContainer.ts src/spec-node/dockerignoreUtils.ts src/test/dockerignore.test.tsI could not run the Docker-backed CLI test suite in this environment because Docker is not installed here.