Skip to content

api: document the zero values, validate the recreate policy, type the not-running error - #14417

Open
ndeloof wants to merge 4 commits into
docker:mainfrom
ndeloof:api-phase0-contract
Open

ndeloof wants to merge 4 commits into
docker:mainfrom
ndeloof:api-phase0-contract

Conversation

@ndeloof

@ndeloof ndeloof commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

What this PR does, in one sentence

Makes the contract of pkg/api easier to read and to program against without breaking any caller: it documents the zero values that differ from the CLI defaults, rejects an unknown recreate policy, gives the "service is not running" failure a type, and marks as deprecated the symbols nothing produces.

Context

pkg/api is what a program embedding Compose reads to know what an operation does. Four things in it mislead today:

  • an options struct left at its zero value does not always behave like the matching command run without flags, because the CLI fills in defaults before calling the API. The sharpest case is CreateOptions.Inherit: false recreates the anonymous volumes (up --renew-anon-volumes), while the CLI default is true. Index 0 and All also mean different things depending on the operation, and nothing says so;
  • Recreate and RecreateDependencies are strings, and any value other than force or never silently behaves like diverged, so a typo such as "forced" is accepted and does the opposite of what the caller meant;
  • the failure of exec, port, export, commit or attach on a service with no running container can only be recognised by matching its message;
  • several exported symbols describe a contract Compose never honors (stack status constants, ConfigOptions, ServiceStatus, some error sentinels, a cloud-login exit code), and a reader cannot tell.

This is the first, non-breaking step of making the package unambiguous, in line with the "Public surface" and API-honesty items of #14074 and the public-surface work of #14396.

What the PR brings

One commit per point, each building and passing tests on its own:

  1. Zero values documented. A package comment lists the options whose zero value differs from the CLI behavior, how Index 0 is resolved for each operation, and what All means in each options struct. The fields carry the same information. No code change.
  2. An unknown recreate policy is rejected. CreateOptions.Validate returns an InvalidOptionError (matching the new ErrInvalidOption), called by Create and, through UpOptions.Validate, by Up, before anything is touched. The empty string still means diverged and the constants are unchanged, so the CLI and existing callers are not affected.
  3. A typed "not running" error. ServiceNotRunningError matches the new ErrNotRunning and carries the service and the replica index. The message is unchanged, so the e2e tests that match it still do.
  4. Dead surface deprecated. The symbols nothing in the repository uses are marked Deprecated, with no change of behavior, so they can go with the next breaking release.

Guardrails: no exported symbol is removed or changes type, and no existing call can start failing except one passing an unknown recreate policy, which was already doing something other than what it said. Making Recreate a typed field, renaming Inherit so its zero value is the CLI default, and removing the deprecated symbols are breaking and are left to a later, deliberate release.

🤖 Generated with Claude Code

@ndeloof
ndeloof requested a review from docker-agent October 28, 2026 15:00

@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

Lower-confidence findings (not posted inline)

  • [low] pkg/api/create_options.go:72 — InvalidOptionError.Allowed omits the empty string, which is also valid (confidence: not scored — low severity, informational only)
  • [low] pkg/api/up_options.go:82 — UpOptions.Validate may produce a doubly-nested errors.Join when CreateOptions has multiple invalid fields (confidence: not scored — low severity, informational only)

@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

@codecov

codecov Bot commented Oct 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ndeloof
ndeloof marked this pull request as ready for review October 28, 2026 15:27
@ndeloof
ndeloof requested review from a team as code owners October 28, 2026 15:27
@ndeloof
ndeloof requested a review from glours October 28, 2026 15:27

@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

…g of All

An options struct left at its zero value does not always behave like the
matching command run without flags, because the CLI fills in some defaults
before calling the API (Inherit is the sharpest case: its zero value is the
opposite of the CLI default). Index 0 and All also mean different things
depending on the operation.

Describe them in a package comment, and on the fields they concern. No code
change.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…s diverged

CreateOptions.Recreate and RecreateDependencies are strings, and any value
other than force or never silently behaved like diverged: a typo such as
"forced" was accepted and did the opposite of what the caller meant.

Add CreateOptions.Validate, which rejects such values with an
InvalidOptionError matching the new ErrInvalidOption, and call it from
Create and, through UpOptions.Validate, from Up, before anything is
touched. The empty string keeps meaning diverged, and the Recreate*
constants are unchanged, so existing callers are not affected.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Exec, port, export, commit and attach fail with "service X is not running"
when they find no container to act on. A program driving Compose as a library
could only recognise that failure by matching the message.

Return a ServiceNotRunningError, which matches the new ErrNotRunning and
carries the service and the replica index. The message is unchanged.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Several exported symbols describe a contract Compose never honors, and a
reader of the package cannot tell: the stack status constants (Stack.Status is
never one of them), ConfigOptions and ServiceStatus (no operation takes or
returns them), the error sentinels ErrAlreadyExists, ErrForbidden, ErrUnknown
and ErrUnsupportedFlag (never returned, with their Is helpers), and the
ExitCodeLoginRequired exit code of the cloud login Compose no longer has.

Mark them Deprecated, with no change of behavior, so that they can be
removed with the next breaking release. Nothing in the repository uses them.

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof
ndeloof force-pushed the api-phase0-contract branch from e0dc379 to bd2ffd2 Compare October 28, 2026 15:36
@ndeloof
ndeloof requested a review from docker-agent October 28, 2026 15:36

@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

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

⌂ Proxy