cli/compose: assorted fixes and cleanups#6803
cli/compose: assorted fixes and cleanups#6803thaJeztah wants to merge 6 commits intodocker:masterfrom
Conversation
thaJeztah
added
status/2-code-review
area/stack
kind/refactor
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The existing code only sorted by PublishedPort (host port), and did not account for multiple ports mapped to the same host-port, but using a different protocol. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- construct merge-opts as a slice - remove intermediate var for overrideServices - use slices.SortFunc for sorting Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The code was using an intermediate map, indexed by name, for both the "base" services _and_ for overrides. This meant that multiple files containing an override for a service would be ignored. Remove the intermediate map for overrides, and apply all overrides for a service instead. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
It's now only used once; let's inline it to remove some abstraction. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Inline it in mergeLoggingConfig and add some vars, which also makes it more readable. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
compose_fixes
branch
from
9d7df02 to
cdc3038
Compare
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small Compose-related fixes and modernizations across the loader and Swarm conversion paths, improving determinism and correctness when merging/sorting configuration.
Changes:
- Modernize
mergeServicesto apply service overrides sequentially (so multiple overrides for the same service aren’t dropped) and useslices.SortFuncfor sorting. - Fix
convertEndpointSpecport sorting to include secondary fields (target/protocol/mode), preventing ambiguous ordering when multiple entries share the same published port. - Simplify
convertUlimitsby removing an unnecessary intermediate map and sorting the resulting slice.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/compose/loader/merge.go |
Refactors service merge logic to apply multiple overrides correctly and modernizes sorting/merge options setup. |
cli/compose/convert/service.go |
Improves port sort determinism for Swarm endpoint specs and simplifies ulimit conversion logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func convertEndpointSpec(endpointMode string, source []composetypes.ServicePortConfig) *swarm.EndpointSpec { | ||
| portConfigs := make([]swarm.PortConfig, 0, len(source)) | ||
| for _, port := range source { | ||
| portConfig := swarm.PortConfig{ | ||
| portConfigs = append(portConfigs, swarm.PortConfig{ | ||
| Protocol: network.IPProtocol(port.Protocol), | ||
| TargetPort: port.Target, | ||
| PublishedPort: port.Published, | ||
| PublishMode: swarm.PortConfigPublishMode(port.Mode), | ||
| } | ||
| portConfigs = append(portConfigs, portConfig) | ||
| }) | ||
| } | ||
|
|
||
| sort.Slice(portConfigs, func(i, j int) bool { | ||
| return portConfigs[i].PublishedPort < portConfigs[j].PublishedPort | ||
| }) | ||
| slices.SortFunc(portConfigs, compareSwarmPortConfig) |
There was a problem hiding this comment.
convertEndpointSpec now sorts ports using compareSwarmPortConfig to handle ties beyond PublishedPort, but the existing TestConvertEndpointSpec only covers distinct PublishedPort values. Add a test case with multiple ports sharing the same PublishedPort but different protocol/target/mode to prevent regressions in the new ordering logic.
There was a problem hiding this comment.
Yeah, that will come with the linked PR; it's how I stumbled on it
| for _, overrideService := range override { | ||
| if baseService, ok := baseServices[overrideService.Name]; ok { | ||
| if err := mergo.Merge(&baseService, &overrideService, mergeOpts...); err != nil { | ||
| return base, fmt.Errorf("cannot merge service %s: %w", overrideService.Name, err) | ||
| } | ||
| baseServices[name] = baseService | ||
| baseServices[overrideService.Name] = baseService | ||
| continue | ||
| } | ||
| baseServices[name] = overrideService | ||
| baseServices[overrideService.Name] = overrideService | ||
| } |
There was a problem hiding this comment.
mergeServices now applies overrides sequentially (fixing the prior behavior of dropping earlier overrides), but there is no unit test asserting that multiple overrides for the same service name are all applied in order. Add a test that passes an override slice containing the same service name multiple times and verifies the final merged service includes all override changes.
There was a problem hiding this comment.
Leaving that for a later exercise
2 participants
cli/compose/convert: convertUlimits: modernize
cli/compose/convert: convertEndpointSpec: fix sorting of ports
The existing code only sorted by PublishedPort (host port), and did
not account for multiple ports mapped to the same host-port, but
using a different protocol.
cli/compose/loader: mergeServices: tidy up and modernize
cli/compose/loader: mergeServices: remove intermediate map for overrides
The code was using an intermediate map, indexed by name, for both the
"base" services and for overrides. This meant that multiple files
containing an override for a service would be ignored.
Remove the intermediate map for overrides, and apply all overrides for
a service instead.
cli/compose/loader: mergeServices: inline mapByName
It's now only used once; let's inline it to remove some abstraction.
cli/compose/loader: remove getLoggingDriver
Inline it in mergeLoggingConfig and add some vars, which also
makes it more readable.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)