Add ENGINE column driven by label - #13959
Conversation
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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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?
cadb256 to
47e560e
Compare
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) { |
47e560e to
f5f73b6
Compare
f5f73b6 to
01bb74d
Compare
Show an ENGINE column in `compose ps` default table output when the
com.docker.compose.engine label is present. 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>
01bb74d to
b4a1f61
Compare
|
Non-blocking, because compose is already heavily depending on labels to store state, but we should start looking if we could transition to using Annotations, especially for state that shouldn't be persisted (doing a // HostConfig the non-portable Config structure of a container.
// Here, "non-portable" means "dependent of the host we are running on".
// Portable information *should* appear in Config.
type HostConfig struct {
// Applicable to all platforms
Binds []string // List of volume bindings for this container
ContainerIDFile string // File (path) where the containerId is written
LogConfig LogConfig // Configuration of the logs for this container
NetworkMode NetworkMode // Network mode to use for the container
PortBindings network.PortMap // Port mapping between the exposed port (container) and the host
RestartPolicy RestartPolicy // Restart policy to be used for the container
AutoRemove bool // Automatically remove container when it exits
VolumeDriver string // Name of the volume driver used to mount volumes
VolumesFrom []string // List of volumes to take from other container
ConsoleSize [2]uint // Initial console size (height,width)
Annotations map[string]string `json:",omitempty"` // Arbitrary non-identifying metadata attached to container and provided to the runtimeWe may have to look for some parts w.r.t. filtering though, especially for the compose project the object is associated with (which, may not always be "runtime state", so maybe labels are more OK for that); for project we should definitely look if containerd namespaces would be a viable option (and add namespaces to other objects) |
What I did
Show an ENGINE column in
compose psdefault table output when the com.docker.compose.engine label is present in any containers. Column is omitted otherwise. Expose {{.Engine}} for custom --format templates.Supports future multi-engine coordinators and workloads.
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did