Conversation
docker-agent
left a comment
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
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
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>
cd6a2ef to
578b2ef
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
docker-agent
left a comment
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.
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>
3cfcee2 to
c2a8650
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
BindMountSource kept iterating after locating the bind mount at the expected destination, so a hypothetical second bind entry at the same target would trip a spurious mismatch even though the first entry already confirmed the source. Break out of the loop once a match is found, per docker-agent review feedback on docker#14234. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since c2a8650.
Assessment: 🟢 APPROVE
The single added line — break after a successful bind-mount match in BindMountSource — is correct. It is placed after found = true and after the source-mismatch error return, so it fires only on a successful match. The post-loop !found check remains accurate because found is already true when break is reached. No issues introduced.
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
Just one nit, not a blocker at all
wantAbs was just wantSource under a different name, left over from an earlier revision. Use wantSource directly. (glours review on docker#14234) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since 7d133b1.
Assessment: 🟢 APPROVE
The single change in this incremental diff — removing the dead-code alias wantAbs := wantSource in BindMountSource and replacing both uses with the original parameter directly — is a clean, correct simplification. The alias became dead once a previous commit added the filepath.IsAbs guard (which returns an error for non-absolute paths and leaves wantSource unchanged), so both sides of the comparison already held the same value. The removal introduces no behavioural change.
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>
.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>
BindMountSource kept iterating after locating the bind mount at the expected destination, so a hypothetical second bind entry at the same target would trip a spurious mismatch even though the first entry already confirmed the source. Break out of the loop once a match is found, per docker-agent review feedback on docker#14234. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
wantAbs was just wantSource under a different name, left over from an earlier revision. Use wantSource directly. (glours review on docker#14234) Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
20f7eda to
fdee36d
Compare
…wed load error, strict service validation projectOrName and toProjectName resolved the project with opposite precedences, and projectOrName silently swallowed any load error when COMPOSE_PROJECT_NAME was set: a broken compose file sent stop, down, ps... into label-based reconstruction without a word — even when the file was named explicitly with --file. One precedence now, documented on both resolvers and applied identically by compose-go while loading: --project-name, then COMPOSE_PROJECT_NAME, then the model's name. The failure policy becomes explicit: an unreadable explicit --file is a hard error; no file around with COMPOSE_PROJECT_NAME set stays the silent file-less workflow; a present-but-broken implicit file falls back to label-based mode with a warning. Service-name validation follows one rule — strict whenever a model is available: restart and wait no longer silently no-op on a typo (validateServiceNames, profile-disabled services remain legitimate targets), and the hand-rolled checks in ps and volumes are removed as dead code, the load-time selection already rejecting unknown names (pinned by test). Epic docker#14074, F.4. Rebased onto main, which since merged the jobs work (docker#14093, docker#14234): projectOrName's job-target detection (jobTargetErr) is restored ahead of the new explicit-file hard-error branch -- the file loaded fine here, only the target's selection failed -- and validateServiceNames now checks project.AllJobs() too, since restart/wait route their service arguments through it instead of projectOrName's own selection. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
This branch has not been deployed
3 participants
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