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.
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>
…o the test Points compose-go at ndeloof/compose-go@a626c70 (branch 14224-working-dir, PR pending) so TestOciRemoteProjectDirectory has something to pass against. This commit exists only to let CI/reviewers see the new test go green with the fix applied -- to drop once the compose-go PR merges and this repo's go.mod bumps to the released pseudo-version normally. 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>
fa69d9b to
cd6a2ef
Compare
2 participants
Draft: adds the regression test for #14224, plus a temporary
go.modreplace pointing at the compose-go fix (compose-spec/compose-go#930) so the test can actually be seen passing before that PR merges.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 is in compose-go (see the issue comment) —LoadConfigFilescan't tell an explicit working dir from a defaulted one once it reaches the loader as a plain string, so a remote resource loader (git, oci) overrides it the same way it would a default.What this 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.go.mod/go.sumcommit replacing compose-go with fix: an explicit working dir must survive a remote resource loader compose-spec/compose-go#930's branch, purely so this test is green in this draft. To be squashed away once that PR merges and this repo's compose-go pin bumps normally — not meant to land as-is.Depends on compose-spec/compose-go#930.
🤖 Generated with Claude Code