Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The regression test and BindMountSource check are well-constructed. The go.mod replace directive is correctly flagged in the PR description as a temporary draft measure to be squashed before merging — no action needed there. One low-severity latent issue in the new check function is noted inline.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
filepath.Abs resolves a relative wantSource against the test process cwd, not any project directory -- silently comparing against the wrong base for a future caller passing a relative path, even though the doc comment frames this as a general check. Reject a relative wantSource explicitly instead. (docker-agent review on docker#14234) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Incremental review — this review covers only the commits pushed since
01f9a21b1402f13f6b96210ad957a18f6864ec95.
The incremental diff implements exactly the fix suggested in the prior review thread (now outdated): replacing the silent filepath.Abs(wantSource) call with an explicit filepath.IsAbs guard that returns an early, informative error when a relative path is passed. wantAbs then holds the already-absolute value directly. The doc-comment update accurately explains the rationale. No new bugs were found in the changed lines.
filepath.Abs resolves a relative wantSource against the test process cwd, not any project directory -- silently comparing against the wrong base for a future caller passing a relative path, even though the doc comment frames this as a general check. Reject a relative wantSource explicitly instead. (docker-agent review on docker#14234) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
fa69d9b to
cd6a2ef
Compare
TestOciRemoteProjectDirectory publishes a project whose only service mounts a relative volume, then runs `up` on the oci:// artifact with an explicit --project-directory pointing elsewhere. The relative volume must resolve against that directory. It currently fails against the pinned compose-go: LoadConfigFiles defaults the working dir to the downloaded artifact's own cache directory whenever a remote resource loader is involved, silently overriding the explicit --project-directory the same way it would a mere default -- docker#14224. On Docker Desktop this surfaces as a hard failure (the cache directory isn't a shared mount); on Linux it silently mounts the wrong directory, matching the original report. New BindMountSource check (pkg/e2e/checks.go) pins a service's bind mount source to an exact expected path, for tests that need to verify which working directory a relative volume path resolved against. Needs the compose-go fix (github.com/ndeloof/compose-go@a626c70, branch 14224-working-dir) to pass -- not yet reflected in go.mod here. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
filepath.Abs resolves a relative wantSource against the test process cwd, not any project directory -- silently comparing against the wrong base for a future caller passing a relative path, even though the doc comment frames this as a general check. Reject a relative wantSource explicitly instead. (docker-agent review on docker#14234) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Replaces the previous commit's temporary fork replace: compose-spec/compose-go#930 is merged, so TestOciRemoteProjectDirectory now passes against the real, released compose-go pin instead of a personal fork branch. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
cd6a2ef to
578b2ef
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| var mounts []struct { | ||
| Destination string | ||
| Source string | ||
| } |
There was a problem hiding this comment.
[medium] BindMountSource matches all mount types, not just bind mounts
The anonymous struct used to unmarshal {{json .Mounts}} captures only Destination and Source, omitting Docker's Type field. Docker's .Mounts array includes bind mounts ("bind"), named volumes ("volume"), and tmpfs entries ("tmpfs"). If a future caller deploys a container that also has a named volume or tmpfs mounted at the same target path, the check will match it first and either return a confusing mismatch (named volume Source is something like /var/lib/docker/volumes/…) or a silent false-pass (tmpfs Source is empty).
The function name and doc-comment both explicitly say "bind mount", so this is a real gap in the check's type discrimination. The current test only has one bind mount and will not trigger the issue, but the function is positioned as a reusable generic observable.
Add Type string to the struct and skip non-bind entries before comparing destinations:
| var mounts []struct { | |
| Destination string | |
| Source string | |
| } | |
| var mounts []struct { | |
| Type string | |
| Destination string | |
| Source string | |
| } |
Then add if m.Type != "bind" { continue } at the top of the inner for _, m := range mounts loop body (before the Destination check), so only bind-mount entries are considered.
| Confidence | Score |
|---|---|
| 🟢 strong | 97/100 |
…esolved The bumped compose-go pin documents that WithImagesResolved now always treats a `type: image` volume source as a docker image reference -- compose-spec has no notion of a source referencing another service, so it no longer special-cases a source that happens to match a service name. resolveServiceImageDigests built its pseudo-project relying on the old behavior to keep such sources (e.g. a volume mounting another service's locally built image) untouched. Filter them out ourselves before resolution, matching the previous observable behavior and unblocking TestResolveImageDigests, which was hanging the CI unit-test job with an unmocked DistributionInspect call for a synthetic "builder:latest" image reference. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
.Mounts also lists named volumes and tmpfs entries at the same Destination; matching on Destination alone let those shadow the intended bind mount, either failing with a confusing volume Source or silently passing against an empty tmpfs Source. Filter to Type == "bind" before comparing, as the function's name and doc promise. Reported by docker-agent. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since
578b2ef8b2e9.
Assessment: 🟢 APPROVE
The two changes in this incremental diff are both correct:
-
cmd/compose/config.go: The new guardif _, isServiceRef := services[source]; isServiceRef { source = "" }properly blanks volume sources whose name matches a compose service before they reachWithImagesResolved, preventing those local service references from being looked up as registry image names. The existing writeback loop already skips empty sources, so no unintended mutation occurs. -
pkg/e2e/checks.go: AddingType stringto the anonymous mount struct and theif m.Type != "bind" { continue }guard directly fixes the issue raised in the prior open review thread —BindMountSourcenow correctly ignores named-volume and tmpfs entries and only compares bind-mount destinations against the expected source path.
No bugs were found in the introduced lines.
What this PR does, in one sentence
docker compose -f oci://... --project-directory DIRmust resolve a relative volume path againstDIR, not against the local copy compose downloaded the artifact into.Context
Reported in #14224: an OCI artifact whose service declares a relative bind mount ends up with that mount resolved under
~/.cache/docker-compose/<digest>/...instead of the directory the user explicitly passed via--project-directory. Root cause was in compose-go —LoadConfigFilescouldn't tell an explicit working dir from a defaulted one once it reached the loader as a plain string, so a remote resource loader (git, oci) overrode it the same way it would a default.The fix landed in compose-spec/compose-go#930, now merged, with this PR's compose-go pin bumped past it.
What the PR brings
TestOciRemoteProjectDirectory(pkg/e2e/remote_oci_test.go): publishes a project with a relative volume, runsupon theoci://artifact with an explicit--project-directoryelsewhere, and asserts the bind mount resolves there.BindMountSource(pkg/e2e/checks.go): a new, reusable check pinning a service's bind mount source to an exact expected path.🤖 Generated with Claude Code