Repository navigation
refactor: extract inspect and projectloader from pkg/compose into internal packages - #14331
Conversation
ps, ls, top, port, volumes, events and viz only read the state of a project: they need the Docker API, the list of the project's containers and the concurrency bound, nothing else of composeService. They now live in internal/inspect, which declares that need as a Backend interface. composeService implements it in a new file of pkg/compose and forwards the seven api.Inspector methods, so no other file of pkg/compose changes, and neither does the behavior: the service-level tests stay in pkg/compose and run unchanged through the forwarders. The few tiny helpers the operations share with the rest of pkg/compose (canonical container name, concurrency-limited errgroup, project filter) are copied for now: the shared kernel they belong to is a later step. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The refactor correctly moves seven read-only Compose operations (ps, ls, top, port, volumes, events, viz) from pkg/compose into the new internal/inspect package. The Backend interface is well-designed and the inspectBackend adapter faithfully translates all types (including the OneOff enum via toOneOff). Forwarding methods pass all arguments and return values correctly. The copied helpers (canonicalContainerName, limitedGroup, projectFilter) are faithful copies of the originals, with limitedGroup correctly guarding against SetLimit(0) deadlock. Goroutine closures in ps.go and top.go are safe under Go 1.22+ per-iteration loop variable semantics.
Lower-confidence findings (not posted inline)
- [low]
internal/inspect/top.go:48— Variablesin errgroup closure shadows method receiver (low severity, not verified; cosmetic concern, no behavioral impact) - [low]
internal/inspect/ps.go:43— Double service-filter application:Containers()already receives the service list, andinServices()is applied again in-memory (low severity, not verified; result is correct but subtly widens filter to single-service case without documentation) - [low]
pkg/compose/inspect.go:34—inspector()allocates a newinspect.Serviceon every forwarding call (low severity, not verified; correct behavior, minor allocation inefficiency)
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Clean refactor. The Backend interface correctly covers all dependencies the moved implementations use, the forwarding wrappers in pkg/compose/inspect.go are correctly wired, error propagation is intact, and tests properly use t.Context().
Lower-confidence findings (not posted inline)
- [low]
internal/inspect/inspect.go:97—canonicalContainerNameslicesc.ID[:12]without a length guard (not verified; Docker IDs are always 64-hex chars, so unreachable in practice — only a risk with synthetic test stubs) - [low]
internal/inspect/ps.go:41—Psfilters by service names twice (once viabackend.Containers, then again viainServices); the second pass is a no-op but adds minor redundancy - [low]
pkg/compose/inspect.go:37—inspector()allocates a newinspect.Service+inspectBackendwrapper on every forwarded call; stateless, so storing it as a field ofcomposeServicewould avoid repeated allocation on hot paths like pollingPs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…nal/projectloader LoadProject and Generate (the api.ProjectLoader operations) need two things from the engine: the Docker API, to read the containers a project is generated from, and the Docker CLI, which resolves remote Compose files. They now live in internal/projectloader, along with the unsupported attributes detection only the loader uses, behind a Backend interface that composeService implements in a new file of pkg/compose, like internal/inspect. The tests move with the code; their assertions are unchanged, only the way the service is built differs. The canonical container name, which both this package and internal/inspect need, moves to a tiny internal/naming package instead of being copied a third time. pkg/compose keeps its own copy until the shared kernel is extracted. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
changed the title
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits pushed since d965367.
Assessment: 🟢 APPROVE
3 participants
What this PR does, in one sentence
Moves two self-contained groups of operations out of the flat
pkg/composepackage into their own internal packages — the read-only ones (ps,ls,top,port,volumes,events,viz) intointernal/inspect, and project loading and generation (LoadProject,Generate) intointernal/projectloader— as the first steps of splittingpkg/composeinto focused sub-packages.Context
pkg/composeis one package of about seventy files and 21k lines, where every operation is a method of the samecomposeServiceand can therefore reach anything: the Docker client, the event bus, the prompt, any other operation. Nothing but convention keeps a read-only command from depending on lifecycle code, and every test builds a whole service. The recent preparation work (theapi.Composesplit into focused interfaces, the removal of the package-level state, the inversion of the CLI dependency) removed what made a split impossible; this PR is the first actual move, and each of the two packages matches one of those interfaces (api.Inspector,api.ProjectLoader).What the PR brings
internal/inspectandinternal/projectloader, each declaring what it needs from the engine as a smallBackendinterface (the Docker client and the concurrency bound for the former, the Docker client and the Docker CLI for the latter) and unable to reach anything else: importingpkg/composeback would be an import cycle.composeServiceimplements those interfaces in two new files ofpkg/composeand forwards the correspondingapi.Composemethods. Behavior is unchanged: the service-level tests ofps,port,vizandvolumesare untouched and run through the forwarders, and the loader tests moved with their code with the same assertions.internal/namingpackage for the one helper both packages need (the canonical container name), rather than copying it a third time. The few other helpers still copied belong to a shared kernel that a later step extracts.pkg/composechanges, so the open pull requests don't have to be rebased; the only one touching a moved file (the unsupported-attributes detection) merges cleanly with this change and still builds and passes its tests.Why this is the right first brick
These are the most isolated clusters of
pkg/compose: together about 1,400 lines that nothing else in the package calls, with no API beyond the interfaces they implement, and two or three things each needed from the engine. That makes them the cheapest place to settle the pattern — sub-package, narrowBackendinterface, forwarders inpkg/compose— that the following moves reuse, with the heavier clusters (provider plugins, images, then the lifecycle ones once the start-phase series has landed) coming after.🤖 Generated with Claude Code