Push custom project config api at start of Up#13958
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.
🟢 No issues found — LGTM! View logs.
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
The project-config push is coordinator-specific integration code, and pkg/compose isn't the right home for it. pkg/ is Compose's reusable, general-purpose surface, tool-specific integrations should stay under internal/, the way Docker Desktop integration lives entirely in internal/desktop (endpoint detection, HTTP client, feature gating) with pkg/compose only calling into it.
Could we move this into a dedicated internal/coordinator package following that same pattern? The detection (the com.docker.compose.project.config context-metadata check), the HTTP client over the engine dialer, the versioned URL and the outcome handling would all live there, and pkg/compose would keep just a thin call-site in up.go.
Besides keeping the core library clean, this gives us a natural home as more coordinator-specific features land, rather than scattering them across pkg/compose.
Separate concern - the request needs a timeout. The http.Client has no Timeout and the Up context has no deadline; since this runs before s.create, a coordinator that accepts the connection but never responds would hang up indefinitely, so the "non-fatal, warn and continue" guarantee doesn't hold. This should be bounded (I'll leave the exact approach up to you).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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>
628304c to
cc0858b
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
-
[medium]
internal/coordinator/coordinator.go:80 —PushEnabledtype-switch lacks*map[string]anyarm — contexts stored via testStoreCfg factory may silently return false (confidence: weak 52/100)The
testStoreCfguses a factory returning&map[string]any{}. If the Docker CLI context store calls this factory to allocate a deserialization target, round-tripped metadata of typemap[string]anywould arrive as*map[string]any. The type-switch inPushEnabledonly handlescommand.DockerContextandmap[string]any— a*map[string]anyfalls to thedefaultbranch and returnsfalse, silently disabling the project-config push for any such context. If this matters in production (contexts backed by generic map metadata), adding acase *map[string]any:arm is the fix.
http.Transport is allocated per call and discarded; without CloseIdleConnections its persistConn goroutines linger until IdleConnTimeout. Tear them down eagerly in the deferred cleanup. Signed-off-by: Nick Sieger <nick@nicksieger.com>
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
1 last small change to ensure the warning message will be properly displayed by the ttyWriter
Co-authored-by: Guillaume Lours <705411+glours@users.noreply.github.com> Signed-off-by: Nick Sieger <nick@nicksieger.com>
"compose up" with no service arguments and no profiles applied resolves the whole project; naming services narrows it to a subset (+ dependency closure), and an applied profile disables the services outside it. Advertise which one the coordinator received via a new X-Compose-Project-Complete header so it can merge partial pushes with stored config instead of treating every push as authoritative. - mark complete only when the service selection is empty and no profile is active; blank profile entries (compose-go's no-profile sentinel) are ignored via hasActiveProfiles - set the header explicitly (true/false); absent means older client and must be read as "false" so no prune is ever triggered on its behalf Signed-off-by: Nick Sieger <nick@nicksieger.com>
Use the metadata to indicate coordinator presense, not just capable of accepting the push project config api. Signed-off-by: Nick Sieger <nick@nicksieger.com>
8ec96b4 to
95d5383
Compare
3 participants
What I did
POST /v{version}/compose/projectis a new custom api to be sent over the engine connection at the start ofcompose upthat sends the full project configuration to the server to allow pre-populating caches and service maps. An engine marks that it can receive the config api by putting acom.docker.compose.project.configkey in the context metadata with value booltrueor string"true".Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did