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
| // call. Failures are non-fatal: warn and continue bringing the project up. | ||
| if !s.dryRun && coordinator.PushEnabled(s.dockerCli) { | ||
| if err := s.pushProjectConfig(ctx, project); err != nil { | ||
| logrus.Warnf("project config push to coordinator failed, continuing: %v", err) |
There was a problem hiding this comment.
To ensure the warning is well displayed by ttyWriter (don't forget to remove the logrus import 😉 )
| logrus.Warnf("project config push to coordinator failed, continuing: %v", err) | |
| s.events.On(newEvent(api.ResourceCompose, api.Warning, "project config push to coordinator failed, continuing", err.Error())) |
| // other Docker API call in "compose up", so without a deadline a coordinator | ||
| // that accepts the connection but never responds would hang "up" indefinitely, | ||
| // defeating the non-fatal "warn and continue" guarantee. | ||
| const defaultTimeout = 30 * time.Second |
There was a problem hiding this comment.
Isn't 10 second enough?
| const defaultTimeout = 30 * time.Second | |
| const defaultTimeout = 10 * time.Second |
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