Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The public error message reverses the detached-option relationship, and the validation documentation overstates its scope.
2 open findings
What changed in this PR
Adds API-level validation for incompatible Up options before side effects occur.
Changes:
- Introduces a typed incompatible-options error and sentinel.
- Validates detached, attached, wait, watch, and exit-policy combinations.
- Adds API and service-level unit tests.
| File | Description |
|---|---|
pkg/api/errors.go |
Defines the error sentinel and helper. |
pkg/api/up_options.go |
Implements typed errors and validation. |
pkg/api/up_options_test.go |
Tests validation combinations and error behavior. |
pkg/compose/up.go |
Validates options before executing Up. |
pkg/compose/up_options_test.go |
Verifies rejection occurs before side effects. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
7837eea to
90ae075
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The validation behavior is consistent with the documented API semantics and is thoroughly covered by focused tests.
0 open findings
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The CLI refuses combinations such as --wait with --attach, or --detach with --abort-on-container-exit, --exit-code-from or --watch. The library did not: an integrator setting Start.Wait together with Start.Attach got an interactive session in which the wait was silently dropped, and the session-only options were silently ignored when Attach is nil. Add UpOptions.Validate, called first thing by Up, which returns an *IncompatibleOptionsError matching api.ErrIncompatibleOptions. The CLI is unchanged. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
90ae075 to
04fe4bc
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The validation behavior is well-scoped, documented, and comprehensively tested.
0 open findings
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
What this PR does, in one sentence
Upnow refuses, with a dedicatedapi.IncompatibleOptionsError, the option combinations the CLI already refuses, instead of leaving a library caller with silently ignored options.Context
The CLI validates its flags (
validateFlags):--waitcannot be combined with--attach/--attach-dependencies, and--detachcannot be combined with--abort-on-container-exit,--abort-on-container-failure,--exit-code-fromor--watch. None of this exists at the API level: a program using the library can setStart.Waittogether withStart.Attach, get an interactive session, and never learn that the wait was dropped; likewiseOnExit,ExitCodeFromandWatchare session-only options that are silently ignored whenAttachis nil. The first case is theAttach+Waitgap noticed while working on #14081.What the PR brings
UpOptions.Validate(), a pure function, called first thing byUp(before any call to the engine or any progress event). It returns an*api.IncompatibleOptionsErrorcarrying the conflicting fields and the reason;errors.Is(err, api.ErrIncompatibleOptions)anderrors.Asboth work. Integrators can also call it themselves to fail early.Start.Attach == nil):WaitwithAttachset: CLI--waitimplies detached and is incompatible with--attach/--attach-dependencies.OnExitother thanCascadeIgnore: CLI--detachvs--abort-on-container-exit/--abort-on-container-failure.ExitCodeFrom: CLI--detachvs--exit-code-from(which implies--abort-on-container-exit).Watch: CLI--detach(and--wait) vs--watch.Upalready passes the new validation, and the existingup/cascade/wait e2e tests pass.--no-recreatevs--renew-anon-volumes(the zero value ofCreate.Inheritis false, so existing callers leaving it unset would be rejected),--no-recreatevs--always-recreate-depsand--force-recreate(RecreateandRecreateDependenciesare separate strategies at the API level),--buildvs--no-build(not representable),--no-buildvs--watch(a nilCreate.Buildstill allows sync-only watch, and rebuild triggers already fail on their own), and a negative--wait-timeout(not a combination of options).AttachTowith a nilAttachis also left alone, sinceStartusesAttachToto rebuild a project from labels.This does change behavior for integrators: a program that passed one of the above combinations used to get an undefined (usually silently degraded) behavior and now gets an error from
Up. That is the point, but it is worth a release note.Why this is the right next brick
It is independent of the switchover work: the logic lives in its own file and
Uponly gains a single guard call, so it can land before or after #14305 without touching the engine path. Related to #14081.🤖 Generated with Claude Code