Repository navigation
Conversation
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
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
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
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
force-pushed
the
api-phase0-contract
branch
from
October 28, 2026 15:36
e0dc379 to
bd2ffd2
Compare
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This was referenced Oct 28, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does, in one sentence
Makes the contract of
pkg/apieasier 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/apiis what a program embedding Compose reads to know what an operation does. Four things in it mislead today:CreateOptions.Inherit:falserecreates the anonymous volumes (up --renew-anon-volumes), while the CLI default istrue.Index0 andAllalso mean different things depending on the operation, and nothing says so;RecreateandRecreateDependenciesare strings, and any value other thanforceorneversilently behaves likediverged, so a typo such as"forced"is accepted and does the opposite of what the caller meant;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:
Index0 is resolved for each operation, and whatAllmeans in each options struct. The fields carry the same information. No code change.CreateOptions.Validatereturns anInvalidOptionError(matching the newErrInvalidOption), called byCreateand, throughUpOptions.Validate, byUp, before anything is touched. The empty string still meansdivergedand the constants are unchanged, so the CLI and existing callers are not affected.ServiceNotRunningErrormatches the newErrNotRunningand carries the service and the replica index. The message is unchanged, so the e2e tests that match it still do.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
Recreatea typed field, renamingInheritso 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