External reviewer noted that the alreadyStopped branch adds createNode
to removeDeps while the !alreadyStopped branch does not — semantically
correct but fragile, since it relies on the implicit invariant that
stopNode.DependsOn contains createNode in the !alreadyStopped path.
Spell out the invariant in a comment so a future maintainer who edits
the stop → create edge in the normal path knows they must also add
createNode unconditionally in the remove deps.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Two coverage gaps surfaced by an external review:
- TestReconcileContainers_DependsOnChain: asserts that a service B with
depends_on: [A] produces a CreateContainer for B that depends on A's
last plan node (the serviceNodes mechanism in infrastructureDeps).
This was the only depends_on-via-plan-DAG behavior untested before.
- TestReconcileContainers_DependsOnScaleDown: companion test that
exercises the scale-down → dependent path specifically, verifying
that the previous commit's lastNode-on-scale-down fix actually wires
the dependency through.
- TestOperationTypeString: adds OpRunProvider to the table; all other
OperationType values were already covered.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Three fixes surfaced by an external code review of the new reconciler:
1. Scale-down now propagates through serviceNodes. When a service is
scaled down (all containers in excess), reconcileService used to
continue without assigning lastNode, leaving r.serviceNodes[svc]
unset. Dependent services then declared no edge on the scale-down
ops and could start before the cleanup finished. Track the
RemoveContainer node as lastNode so depends_on chains pick it up.
2. mustRecreate errors are no longer silently ignored by sortContainers.
The comparator used `obsi, _ := r.mustRecreate(...)`, falling back
to false on any hashing error. Pre-compute obsolescence into a map
keyed by container ID before sorting and propagate the error to
reconcileService.
3. A container is no longer Stopped twice when its network and its
config both diverge. planRecreateNetwork already stops the affected
container as part of the disconnect/remove/recreate dance; the
subsequent planRecreateContainer (triggered via hasNetworkMismatch)
used to add another OpStopContainer against the now-stopped target.
Track stops in r.stoppedByPlan; planRecreateContainer reuses an
existing Stop node when present, and chains its Remove on both that
Stop and the replacement Create.
Two golden tests (TestReconcileNetworks_Diverged*) are updated to
reflect the new, dedupe'd plan shape (one Stop instead of two per
recreated container).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Three improvements identified in the Principal Engineer pass but
deliberately deferred:
1. Test fidelity. Split executePlan into newPlanExecutor (constructs
the executor seeded from observed state) and (*planExecutor).run
(walks the DAG). Production callers go through executePlan
unchanged. TestExecutePlanRemoveContainerDropsFromCache now uses
newPlanExecutor + run, exercising the same errgroup, done-channel
and group-tracker wiring as production instead of a hand-rolled
loop over executeNode.
2. //nolint:unused chain. The three preserved helpers
(reconciler.prompt, planRecreateVolume, servicesUsingVolume) each
carried a separate "kept for future" comment. Consolidate the
rationale on the reconciler.prompt field doc and point the helper
nolint directives there, so a future cleanup is a single grep.
3. Concurrency test. Add TestExecutePlanConcurrentRemovesCacheCoherence
which builds N independent Stop→Remove chains in one plan; the
errgroup fans them out across goroutines that all hit
containersByService under the mutex. Passes under -race. Failure
would expose a missing or incorrect lock.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Two related changes to the executor, plus the small cleanups they
attracted in review:
* Rename node consults a planner-set CreateNodeID instead of walking
ancestors. The old execRenameContainer searched node.DependsOn for a
CreateContainer result and fell back to a recursive walk through the
group chain. That worked only by convention; a future op with cross-
node data needs would have to rediscover or copy the pattern. Now the
rename op carries an explicit CreateNodeID int set at plan time and
the executor reads pctx[CreateNodeID].ContainerID directly. The
recursive findCreatedIDInChain is gone.
* Stop re-listing containers on every create. execCreateContainer used
to call getContainersByService(ctx, projectName) — a fresh
ContainerList per create — to resolve service references at execute
time. The executor now holds a live containersByService view seeded
from ObservedState (via observed.containersByService()) and grown as
OpCreateContainer nodes complete, so service references resolve from
memory. On OpRemoveContainer the removed container is dropped from
the view via slices.DeleteFunc, so a dependent's create that resolves
network_mode: service:x against the just-removed container cannot
pick up a stale ID (Containers.sorted() orders by canonical name and
would otherwise return the removed container).
* Defensive slices.Clone of op.Service.VolumesFrom in execCreateContainer.
resolveServiceReferences mutates VolumesFrom in place, and the
shallow struct copy of *op.Service still shares the backing array.
Single-execution-per-node makes it safe today, but the clone removes
the trap for any future parallel-execution mode.
* Operation gains a CreateNodeID int (not a *PlanNode pointer) to avoid
a structural cycle between Operation and PlanNode. OperationType
values are pinned to explicit integers so adding an op in the middle
cannot silently shift the others.
* execRenameContainer carries two checks so a missing CreateNodeID and
an empty produced ID are distinguishable in logs. Both are programmer
invariants (prefixed "internal:").
* containersByServiceFromObserved moved from a package-level helper to
a method on *ObservedState.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
ensureProjectVolumes already prompts the user when a volume's config hash
diverges from the compose file (create.go:1626) and recreates it on confirm.
The reconciler ran after ensureProjectVolumes and prompted again with the
exact same message — so a user who declined the first prompt was asked the
same question a second time.
Drop the prompt + recreate call from reconcileVolumes(). Recreation of
diverged volumes stays owned by ensureProjectVolumes; the reconciler only
plans the creation of missing volumes. If the user declined recreation,
the existing container's mounts still match the existing volume name and
hasVolumeMismatch correctly returns false, so containers are not falsely
flagged as obsolete.
Keep the supporting infrastructure available for future use, when
divergence detection migrates fully into the reconciler:
- reconciler.prompt field
- prompt parameter on reconcile()
- planRecreateVolume function (//nolint:unused)
- servicesUsingVolume function (//nolint:unused)
- noPrompt test helper
The reframed test (TestReconcileVolumes_DivergedIsIgnored) asserts the
new contract: a diverged volume produces no plan operations from the
reconciler.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
- plan.go: pin OperationType constants to explicit values so adding an op
in the middle doesn't shift the others.
- executor.go: remove the meaningless `var _ = getContainerProgressName`
line — same-package functions are always accessible.
- reconcile.go: fix the stale switch-default comment that contradicted
the case clause above it.
- reconcile.go: drop the local `serviceLabel` const that shadowed
`api.ServiceLabel` and use the shared constant.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
- Fix ContainerReplaceLabel detection: use op.Inherited != nil (not
op.Container) as signal for recreate in execCreateContainer
- Use observed network name (not desired) for DisconnectNetwork and
RemoveNetwork operations, in case the name changed
- Use observed volume name (not desired) for RemoveVolume operations
- Update reconciliation.md with 3 new lessons learned (7.8, 7.9, 7.10)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
- Remove the `convergence` struct and `newConvergence` constructor
- Extract `resolveServiceReferences` as a standalone function taking
`map[string]Containers` instead of a method on convergence
- Add `getContainersByService` helper on composeService
- Update run.go and executor.go to use the new standalone function
- Remove dead code: `getObservedState`, `setObservedState`
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
When a service's build context is a remote git/HTTP URL, the path was
unconditionally added to bake's --allow fs.read= entitlements. On Windows,
bake then tries to evaluate the URL as a local filesystem path and fails
because `https:` is invalid path syntax (colon is reserved for drive
letters).
Apply the same gitutil.ParseGitRef + "://" check already used for
additional_contexts so that remote contexts are skipped from the fs.read
allowlist.
Fixes#13815
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Replace the "flag any literal env var" check with a key-name heuristic
backed by the upstream DefangLabs keyword detector (password, secret,
token, api_key, …), and convert the hard error into a prompt matching
the existing checkForBindMount / checkForSensitiveData UX. --with-env
silences the env prompt; literal config.content gets its own prompt.
The previous check flagged benign vars like LOG_LEVEL=info, blocking
the 99% case, while still missing low-entropy real secrets the
existing secret-detector skips (MYSQL_ROOT_PASSWORD=toto slips through
on entropy ~1.5).
Refs: docker/compose#13394
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Docker Desktop is removing the "Enable Logs view" beta setting, so drop
the /app/settings check and rely on /features alone. With the setting
gate gone, the compose hook subprocess would print the Logs view hint
regardless of LogsTab; add a flag check in handleHook. Consolidate
engine-label discovery and feature-flag evaluation into internal/desktop.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The stop lifecycle hook added in 672dc14d2 calls stoppingEvent and
stoppedEvent from pkg/compose/plugins.go, but the helpers had been
removed by the earlier wrapper-cleanup in da530c723. The result was a
broken go build for pkg/compose on upstream/main.
Restore the two helpers alongside the symmetric creating/created and
removing/removed pairs so call sites in plugins.go match the existing
pattern used in the same switch.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
A previous change extended IsEmpty() to include `Stop == nil`. For a
provider that advertises only a `stop` block, this caused
commandMetadataIsEmpty to be false in setupPluginCommand, which made
the option-forwarding filter reject every key — silently dropping
all provider options on `up` and `down`.
The Stop-presence signal lives independently in stop.go and the
"stop" case of setupPluginCommand, so reverting the IsEmpty check
to its original semantics is sufficient.
Addresses PR #13779 review feedback.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The previous implementation fetched the provider binary path and
metadata twice per service during `compose stop`: once in stop.go
to gate on the `stop` capability, and again inside runPlugin via
setupPluginCommand.
setupPluginCommand now signals "skip" by returning (nil, nil) when
the requested command is absent from the provider's metadata.
stop.go calls runPlugin directly; the skip-when-unadvertised check
moves into runPlugin.
Addresses PR #13779 review feedback.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Provider-backed services were silently skipped on `docker compose stop`,
leaving external resources running after the user expected the stack to
be paused (e.g. after Ctrl+C during `up --watch`).
Compose now invokes `<provider> compose stop <service>` for providers
that advertise a `stop` block in their `metadata` subcommand output.
Providers that do not advertise stop (or do not implement metadata at
all) are silently skipped, preserving backward compatibility with
existing providers that pre-date this hook.
Closes#13772
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
- Fix stale image/container reuse across test runs - Add registry readiness check and async removal polling
- Skip multi-arch test when docker driver supports it
- Use t.Cleanup for reliable teardown, fix project name mismatches
- Re-enable 4 previously skipped tests that now pass
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
The compose process performs OCI artifact fetches in-process via
containerd's docker resolver, whose default transport only honors
HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars. Users behind PAC-only
corporate proxies hit i/o timeouts on `oci://` includes and on
`compose publish`.
When Docker Desktop is the active engine and exposes httpproxy.sock,
route the resolver through it (PAC-aware). Falls back to the default
transport when DD is unavailable or the socket is missing. Modeled on
docker/mcp-gateway PR #354.
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Provider info and error messages containing newlines broke the TTY
progress display (timer drifting to a new line, broken cursor
movement). Extract only the first line for progress events via
firstLine(). Full messages remain available through the provider's
own debug message type.
Skip provider services during watch rebuild convergence by adding a
SkipProviders flag to CreateOptions, set only by the watch rebuild
path. This prevents unnecessary re-invocation of providers on every
file change while preserving normal provider execution for all other
commands (up, create, run, scale).
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Add CLI hooks handler to show "What's next:" hints pointing to the
Docker Desktop Logs view after `docker logs`, `docker compose logs`,
and `docker compose up -d`.
Add `l` keyboard shortcut in the `compose up` navigation menu to
open the Logs view, gated on Docker Desktop feature flag and settings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
Renames test to Test_preChecks_sensitive_data_detected_decline. Uses a temporary .env file with an AWS token to reliably trigger the DefangLabs secret detector, and confirms that preChecks correctly aborts early on user decline.
Signed-off-by: Ishwar <ishwarcm@iitbhilai.ac.in>
Before this, assertion libraries were mixed, sometimes
even in the same file.
git grep -l '"gotest.tools/v3/' | wc -l
75
git grep -l '"github.com/stretchr/testify' | wc -l
24
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
For Docker daemons older than API 1.44, the extra networks omitted from
ContainerCreate must be connected individually after creation via
NetworkConnect. If any NetworkConnect call fails, remove the freshly
created container to prevent orphans.
Add two tests:
- TestCreateMobyContainerLegacyAPI: happy path verifying NetworkConnect
is called for the secondary network on API 1.43
- TestCreateMobyContainerLegacyAPI_NetworkConnectFailure: verifies
ContainerRemove is called with Force when NetworkConnect fails
Signed-off-by: jarek <jkrochmalski@gmail.com>
Before API 1.44 (Docker Engine 25.0), ContainerCreate only accepted a
single EndpointsConfig entry. Passing multiple entries silently drops
all but one network, leaving containers under-connected on older daemons
such as Docker 20.10 or Synology DSM 7.1/7.2.
Add apiVersion144 constant and wrap the secondary-networks loop in
defaultNetworkSettings so that only the primary network is included in
the ContainerCreate call when the negotiated API version is below 1.44.
Signed-off-by: jarek <jkrochmalski@gmail.com>
After API negotiation, Compose should only rely on the negotiated version
and never use the daemon's raw max version for request shaping. Merge both
functions into a single RuntimeAPIVersion that negotiates via Ping and
returns ClientVersion, erroring if the client reports an empty version
instead of silently falling back to ServerVersion.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
