Add ENGINE column driven by com.docker.compose.engine label#13959
Add ENGINE column driven by com.docker.compose.engine label#13959nicksieger wants to merge 3 commits into
Conversation
Detect the `com.docker.compose.project.config` context metadata key (bool
true or string "true"); when set, POST the full project config to the
coordinator at `/v{version}/compose/project` over the engine socket dialer
at the start of `compose up`, before any other Docker API call.
- version-gated behind new apiVersionComposeProjectConfig constant
- skipped in dry-run
- non-fatal: warn and continue on request or engine error
Signed-off-by: Nick Sieger <nick@nicksieger.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The ENGINE column feature is clean and correct. The implementation:
- Correctly guards against nil Labels maps in the new
Engine()method — no panic risk for containers without labels - Only shows the ENGINE column when at least one container carries the
com.docker.compose.enginelabel - The
hasEnginedetection loop correctly reflects the filtered container set - Format string construction order (ENGINE before SIZE) is internally consistent
- New test covers the happy-path scenario
No bugs were found in the added code.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
hasEngine scans the filtered container set (cmd/compose/ps.go), so column presence depends on which services you name. Say svc1 carries the com.docker.compose.engine label and svc2 doesn't:
compose ps→ ENGINE column shown (svc2gets an empty cell)compose ps svc2→ no ENGINE column, even thoughsvc2's row is identical to the one just showncompose ps svc1→ shown again
Same rows, different schema. That breaks table-parsing and diverges from docker ps, where columns are never data-gated.
As this is a very specific display, could we gate it on the coordinator context (the same context-metadata detection as the project-config work, living under internal/) instead of the data? Presence then stays stable across ps / ps <subset>.
Nit: api.EngineLabel (com.docker.compose.engine) collides by name with desktop.EngineLabel (com.docker.desktop.address), can we consider a more specific name?
Relocate the coordinator-specific project-config push out of pkg/compose, which is Compose's reusable surface, into a dedicated internal/coordinator package mirroring internal/desktop. Detection, the engine-dialer HTTP client, the versioned URL and outcome handling now live there; up.go keeps a thin call-site. Bound the push with an http.Client timeout so a coordinator that accepts the connection but never responds cannot hang "compose up", preserving the non-fatal warn-and-continue guarantee. Signed-off-by: Nick Sieger <nick@nicksieger.com>
Show an ENGINE column in `compose ps` default table output when the
current Docker context is coordinator-enabled (detected via the
project-config context metadata, internal/coordinator.PushEnabled),
rather than gating on which listed containers carry the engine label.
Column presence is now stable across `ps` and `ps <subset>`; per-row
value comes from the com.docker.compose.engine label. Expose
{{.Engine}} for custom --format templates.
Rename api.EngineLabel to api.ContainerEngineLabel to avoid the
name collision with desktop.EngineLabel (com.docker.desktop.address).
Signed-off-by: Nick Sieger <nick@nicksieger.com>
cadb256 to
47e560e
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The ENGINE column feature is well-structured. The new internal/coordinator package correctly gates the feature on Docker context metadata, the label constant matches the documented name (com.docker.compose.engine), and error paths (HTTP timeouts, non-200 responses, missing coordinator) are handled. Test coverage is present for the new coordinator logic, the formatter, and compose ps.
No high- or medium-severity bugs were found in the changed code.
| assert.Assert(t, strings.Contains(out.String(), "moby"), out.String()) | ||
| }) | ||
|
|
||
| t.Run("engine column hidden when no label present", func(t *testing.T) { |
There was a problem hiding this comment.
| t.Run("engine column hidden when no label present", func(t *testing.T) { | |
| t.Run("engine column hidden when coordinateur disabled", func(t *testing.T) { |
3 participants
What I did
Show an ENGINE column in
compose psdefault table output when thecurrent Docker context is coordinator-enabled, using the
com.docker.compose.engine label value per row. Column is omitted
otherwise. Expose {{.Engine}} for custom --format templates.
Supports future multi-engine coordinators and workloads.
NOTE: based on #13958, wait to rebase after merging.
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did