Skip to content

cli: one project-resolution story — single name precedence, no swallowed load error, strict service validation - #14151

Open
ndeloof wants to merge 1 commit into
docker:mainfrom
ndeloof:f4-project-resolution
Open

ndeloof wants to merge 1 commit into
docker:mainfrom
ndeloof:f4-project-resolution

Conversation

@ndeloof

@ndeloof ndeloof commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Epic #14074, F.4, per the agreed behavior:

One name precedence (Q1), documented on both resolvers and applied identically by compose-go while loading: --project-name > COMPOSE_PROJECT_NAME > the model's name. projectOrName and toProjectName used to disagree.

Explicit failure policy (Q2.c) — projectOrName used to swallow any load error when COMPOSE_PROJECT_NAME was set, silently sending stop/down/ps… into label-based reconstruction, even for an explicit --file:

  • unreadable explicit --file → hard error;
  • no compose file around + COMPOSE_PROJECT_NAME → the normal file-less workflow, silent;
  • present-but-broken implicit file + COMPOSE_PROJECT_NAME → label-based fallback with a warning.

Strict service validation whenever a model is available (Q3): restart and wait no longer silently no-op on a typo (profile-disabled services remain legitimate targets); the hand-rolled checks in ps/volumes are removed as dead code — load-time selection already rejects unknown names (pinned by test).

Behavioral changes: restart/wait on an unknown service now error; a broken explicit --file now errors instead of silently falling back; a broken implicit file now warns. Unit tests cover the full resolution matrix.

🤖 Generated with Claude Code

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The PR is well-structured and implements the described behavior correctly:

  • Name precedence (--project-name > COMPOSE_PROJECT_NAME > model name) is applied consistently through the refactored projectOrName and toProjectName.
  • Error handling for explicit --file failures (hard error), missing files with env name (silent), and broken implicit files with env name (warning + fallback) is correct and clearly documented.
  • validateServiceNames correctly handles the nil-project case (label-based mode has no manifest to validate against) and covers profile-disabled services as legitimate targets.
  • The removal of hand-rolled service checks in ps.go and volumes.go is safe: ToProject service-selection already rejects unknowns at load time when a model is available.
  • The new test file covers the full resolution matrix with t.Context() and clearly-named subtests.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/compose/ps.go 0.00% 0 Missing and 1 partial ⚠️
cmd/compose/volumes.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@ndeloof
ndeloof force-pushed the f4-project-resolution branch from ba06b79 to 6ccca66 Compare September 22, 2026 13:01
@ndeloof

ndeloof commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Re-checked this PR's legitimacy given everything that's landed since it was opened (Aug 28) — it's still exactly the right fix (Q1/Q2.c/Q3 from #14074 are unaffected by the jobs work), but it needed reconciling with projectOrName's new job-target detection (#14093, #14234, both merged since):

  • jobTargetErr restored ahead of the new explicit-file hard error: this PR's rewrite replaced projectOrName's whole error-handling body, dropping the job check entirely. Without it, docker compose stop migrate -f compose.yaml (a declared job, explicit file) would have regressed from the clear job "migrate" can only be triggered with "docker compose run" back to a raw no such service: migrate — the file loads fine, only the target's selection fails, so the job check must run before the new explicit-file/implicit-file branching, not be skipped by it.
  • validateServiceNames now also checks project.AllJobs(): restart/wait don't route their service arguments through projectOrName's own ToProject selection (they call it with no services, precisely so validateServiceNames validates the full name afterward) — a separate code path from every other projectOrName caller, so restart migrate/wait migrate would otherwise have fallen through to the generic no such service instead of the job-specific message.
  • Added TestRestartRefusesJob/TestWaitRefusesJob (e2e) and a job case in TestValidateServiceNames, since this is genuinely distinct wiring from the other commands' job-refusal coverage.

Rebased onto current main, build/vet/lint clean, full unit suite green, all job-refusal e2e tests (create/start/stop/down/restart/wait) pass.

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟡 NEEDS ATTENTION

Comment thread cmd/compose/compose.go
…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.

docker-agent review: the "compose file found but could not be loaded"
warning's suppression guard only matched errdefs.IsNotFoundError
(compose-go's own ErrNotFound sentinel) -- a raw os.ErrNotExist (e.g. a
nonexistent --project-directory) wasn't recognized and would have
printed a misleading warning. Added errors.Is(err, os.ErrNotExist) as
a fallback, and TestProjectOrNameResolution now asserts the warning's
presence/absence in both directions instead of just the fallback name.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof
ndeloof force-pushed the f4-project-resolution branch from 065922e to a64fdf7 Compare September 22, 2026 17:26

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants