Honor dockerignore - #1883
Conversation
There was a problem hiding this comment.
4 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/job.ts">
<violation number="1" location="src/job.ts:734">
P2: Duplicate rsync/sed command logic for honoring .dockerignore increases maintenance risk</violation>
<violation number="2" location="src/job.ts:734">
P1: `copyIn` applies `.dockerignore` filtering to cache/artifact restores, which can silently drop files. The `copyIn` method is used as a general-purpose copy for both initial source checkout (where `.dockerignore` filtering is desired) and cache/artifact restores (where it is not). When restoring caches or artifacts, if the source folder contains a `.dockerignore`, matching files will be excluded by rsync, breaking cache/artifact integrity. The previous `container cp` behavior copied all files verbatim.</violation>
</file>
<file name="src/parser-includes.ts">
<violation number="1" location="src/parser-includes.ts:283">
P2: `git ls-remote` stdout is parsed without guarding against empty output, causing a crash-prone TypeError path</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.refreshLongRunningSilentTimeout(writeStreams); | ||
| } | ||
| await Utils.spawn([this.argv.containerExecutable, "cp", `${argv.stateDir}/builds/.docker/.`, `${containerId}:${this.ciProjectDir}`], argv.cwd); | ||
| const cmd = "sed -E -e 's,^[.]?/,,' -e '/^[.]git/d' /data/src/.dockerignore >/data/ignore 2>/dev/null; rsync -avh --exclude-from /data/ignore /data/src/. /data/dest"; |
There was a problem hiding this comment.
P1: copyIn applies .dockerignore filtering to cache/artifact restores, which can silently drop files. The copyIn method is used as a general-purpose copy for both initial source checkout (where .dockerignore filtering is desired) and cache/artifact restores (where it is not). When restoring caches or artifacts, if the source folder contains a .dockerignore, matching files will be excluded by rsync, breaking cache/artifact integrity. The previous container cp behavior copied all files verbatim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/job.ts, line 734:
<comment>`copyIn` applies `.dockerignore` filtering to cache/artifact restores, which can silently drop files. The `copyIn` method is used as a general-purpose copy for both initial source checkout (where `.dockerignore` filtering is desired) and cache/artifact restores (where it is not). When restoring caches or artifacts, if the source folder contains a `.dockerignore`, matching files will be excluded by rsync, breaking cache/artifact integrity. The previous `container cp` behavior copied all files verbatim.</comment>
<file context>
@@ -731,7 +731,14 @@ If you know what you're doing and would like to suppress this warning, use one o
this.refreshLongRunningSilentTimeout(writeStreams);
}
- await Utils.spawn([this.argv.containerExecutable, "cp", `${argv.stateDir}/builds/.docker/.`, `${containerId}:${this.ciProjectDir}`], argv.cwd);
+ const cmd = "sed -E -e 's,^[.]?/,,' -e '/^[.]git/d' /data/src/.dockerignore >/data/ignore 2>/dev/null; rsync -avh --exclude-from /data/ignore /data/src/. /data/dest";
+ await Utils.spawn([
+ this.argv.containerExecutable, "run", "--rm",
</file context>
There was a problem hiding this comment.
Hum, another option would be to let people provide a ignore file specific to gitlab-ci-local.
My main problem is with big files in LFS. git lfs ls-files -n can output lfs-files and be used to compute a gcl-ignore file. But that would require an extra file for each such repository.
@firecow Any ideas?
| this.refreshLongRunningSilentTimeout(writeStreams); | ||
| } | ||
| await Utils.spawn([this.argv.containerExecutable, "cp", `${argv.stateDir}/builds/.docker/.`, `${containerId}:${this.ciProjectDir}`], argv.cwd); | ||
| const cmd = "sed -E -e 's,^[.]?/,,' -e '/^[.]git/d' /data/src/.dockerignore >/data/ignore 2>/dev/null; rsync -avh --exclude-from /data/ignore /data/src/. /data/dest"; |
There was a problem hiding this comment.
P2: Duplicate rsync/sed command logic for honoring .dockerignore increases maintenance risk
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/job.ts, line 734:
<comment>Duplicate rsync/sed command logic for honoring .dockerignore increases maintenance risk</comment>
<file context>
@@ -731,7 +731,14 @@ If you know what you're doing and would like to suppress this warning, use one o
this.refreshLongRunningSilentTimeout(writeStreams);
}
- await Utils.spawn([this.argv.containerExecutable, "cp", `${argv.stateDir}/builds/.docker/.`, `${containerId}:${this.ciProjectDir}`], argv.cwd);
+ const cmd = "sed -E -e 's,^[.]?/,,' -e '/^[.]git/d' /data/src/.dockerignore >/data/ignore 2>/dev/null; rsync -avh --exclude-from /data/ignore /data/src/. /data/dest";
+ await Utils.spawn([
+ this.argv.containerExecutable, "run", "--rm",
</file context>
|
I close this in favor of #1884 which I think is a better idea/implementation. |
Try to address the issue #1879 by using
rsyncwith a temporary container.Modify the
.dockerignorefound to be compatible to rsync ignore file (quite the same format)..gitdirectory is removed from ignore as it is needed for some functions.Summary by cubic
Honors
.dockerignorewhen copying sources into job containers by replacingdocker cpwith helper-imagersync, and adds component context variables for templates. Also fixes array input validation and improves local component includes.New Features
docker cpwith helper-imagersyncfor copy-in and Docker context; honor.dockerignore(normalize patterns) and always include.git.$[[ component.name ]],$[[ component.reference ]],$[[ component.version ]],$[[ component.sha ]]with lazy resolution (handles semantic ranges and annotated tags).Bug Fixes
optionsforarrayinput types element-by-element, rejecting values not in the allowed list.Written for commit 283b112. Summary will update on new commits.