refactor: code deduplication and simplification#13759
refactor: code deduplication and simplification#13759ndeloof wants to merge 9 commits intodocker:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors Compose CLI/service code to reduce duplication by extracting shared helpers and inlining/removing trivial wrappers, while keeping behavior the same.
Changes:
- Added shared helpers in
cmd/compose/(withBackend,optionalTimeout) andpkg/compose/(forEachContainerConcurrent,removeResource,labelFilter) to deduplicate common patterns. - Simplified progress/event emission by collapsing many one-off event helper functions into direct
newEvent(...)calls. - Removed unused utilities/wrappers (graph traversal wrapper fns;
utils.Setmethods/tests) and updated call sites/tests accordingly.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/set_test.go | Removes the Union unit test and its now-unused import. |
| pkg/utils/set.go | Removes unused Set methods and repositions RemoveAll. |
| pkg/compose/restart.go | Switches to newEvent(...) for restart progress events. |
| pkg/compose/remove.go | Simplifies iteration over stopped containers; adds cyclo suppression annotation. |
| pkg/compose/pull.go | Replaces pull progress helpers with newEvent(...) calls. |
| pkg/compose/ps_test.go | Updates label filtering to use api.ConfigHashLabel directly. |
| pkg/compose/progress.go | Collapses many trivial event helper wrappers into newEvent(...); keeps function-value helpers. |
| pkg/compose/pause.go | Uses forEachContainerConcurrent to deduplicate pause/unpause concurrency pattern. |
| pkg/compose/monitor.go | Uses api.ConfigHashLabel directly (removes wrapper indirection). |
| pkg/compose/logs_test.go | Updates label filtering to use api.ConfigHashLabel directly. |
| pkg/compose/kill_test.go | Updates label filtering to use api.ConfigHashLabel directly. |
| pkg/compose/kill.go | Uses forEachContainerConcurrent and newEvent(...) for kill progress events. |
| pkg/compose/filters.go | Introduces labelFilter(...) and removes redundant hasConfigHashLabel(). |
| pkg/compose/down.go | Extracts removeResource(...) helper and switches stop events to newEvent(...). |
| pkg/compose/dependencies.go | Removes redundant wrapper functions via method expressions; replaces custom slice remove with slices.DeleteFunc. |
| pkg/compose/convergence_test.go | Updates label filtering to use api.ConfigHashLabel directly. |
| pkg/compose/convergence.go | Replaces several progress helpers with newEvent(...); refactors shared-namespace resolution via closure. |
| pkg/compose/containers.go | Adds forEachContainerConcurrent; documents sorted() as in-place mutation; uses api.ConfigHashLabel directly. |
| cmd/compose/stop.go | Uses withBackend(...) and optionalTimeout(...) to deduplicate backend/timeout boilerplate. |
| cmd/compose/start.go | Uses withBackend(...) to deduplicate backend creation boilerplate. |
| cmd/compose/restart.go | Uses withBackend(...) and optionalTimeout(...) to deduplicate backend/timeout boilerplate. |
| cmd/compose/pause.go | Uses withBackend(...) to deduplicate backend creation boilerplate. |
| cmd/compose/kill.go | Uses withBackend(...) to deduplicate backend creation boilerplate. |
| cmd/compose/backend.go | Adds new shared backend/timeout helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (s Set[T]) RemoveAll(elements ...T) { | ||
| for _, e := range elements { | ||
| s.Remove(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Set is an exported type, and this change removes exported methods (Clear, Union). Even if they’re unused internally, this is a compile-time breaking API change for any downstream consumers importing pkg/utils. Consider keeping these methods as thin wrappers (potentially marked deprecated) or moving Set to an internal/ package if it’s not meant to be part of the public API.
There was a problem hiding this comment.
Restored. Both Clear and Union are back as thin wrappers with // Deprecated: godoc comments, preserving API compatibility for any downstream consumers. Fixed in the latest commit.
| resource := "Image " + service.Image | ||
| s.events.On(pullingEvent(service.Image)) | ||
| s.events.On(newEvent("Image "+service.Image, api.Working, api.StatusPulling)) |
There was a problem hiding this comment.
resource is computed as "Image "+service.Image, but the progress events re-create the same string literal. Using the resource variable for the newEvent calls would avoid duplication and guarantees the event IDs stay consistent if the formatting ever changes.
There was a problem hiding this comment.
Fixed — the newEvent calls now use the already-computed resource variable instead of re-constructing the string literal. Fixed in the latest commit.
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
…pause/kill logic Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
63752ea to
de5e11d
Compare
Assisted-By: docker-agent Signed-off-by: Nicolas De loof <nicolas.deloof@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Pure refactoring — no functional changes. All existing tests pass.
Changes:
withBackend()helper incmd/compose/to replace repetitiveNewComposeService → backend.Fooboilerplate in pause, unpause, kill, start, stop, restartoptionalTimeout()helper to deduplicate timeout-pointer construction in stop/restartprogress.gotonewEvent()calls; keep only functions used ≥3 times or as function valuesdependencies.go(leaves,roots,getChildren,filterChildren,filterParents) — replaced with method expressions; replaceremove(slice, item)withslices.DeleteFuncremoveResource()helper indown.goto deduplicate the remove-emit pattern shared byremoveImageandremoveVolumeforEachContainerConcurrent()incontainers.goto deduplicate the errgroup iteration pattern in pause, unpause, killresolveSharedNamespaces(NetworkMode/Ipc/Pid) into aresolveclosurecontainers.forEach(only 1 remaining call site); add mutation comment tosorted(); replacehasConfigHashLabel()withapi.ConfigHashLabeldirectly; addlabelFilter()factory for label filters; remove unusedSet.ClearandSet.Union