Conversation
…d service A service reachable only as another service's build additional_contexts must still build even when its own profile is inactive: it's referenced for its image, not started as a workload -- the same tolerance the depends_on consistency check already grants a disabled, non-required dependency, and the same case addBuildDependencies already special-cases on the docker/compose side. compose-go's own consistency check doesn't grant that tolerance yet: it rejects the reference outright, failing the whole project load before addBuildDependencies (or anything else, up included) ever runs -- for up, build and up --no-build alike, exactly as reported. This is expected to fail until compose-go bumps to include compose-spec/compose-go#931. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
⚠️ Verification inconclusive: The verifier returned a malformed response (duplicatefinding_idin the verdicts array), so findings below are surfaced unverified from the drafter. Please evaluate them manually.
Unverified findings (verification inconclusive)
-
[medium]
pkg/e2e/build_test.go:619 —up -dwithout--buildmay not exercise the build path the regression test targetsThe step uses
ComposeCmd("--profile", "classroom", "up", "-d")without the--buildflag. Docker Compose only builds an image duringupif no local image already exists for that service. Ife2e-additional-context-base:latestore2e-additional-context-classroom:latestare present in the local daemon (e.g. leaked from another test, or from a prior run whoseDefercleanup raced),upwill skip the build entirely — theImageExistscheck passes trivially, and the regression (which lives in the build path foradditional_contextstargeting a disabled-profile service) is never actually exercised. Every other build-focused test in this file that verifies building behaviour usesComposeCmd("up", "--build", ...)or a separatebuildstep. Fix: add"--build"to theupinvocation so the build path is always triggered. -
[low]
pkg/e2e/testdata/TestBuildAdditionalContextDisabledService/compose.yaml:20 —classroomservice relies solely onENTRYPOINTindockerfile_inline; nocommandat compose levelThe
classroomservice setsENTRYPOINT ["sleep", "infinity"]in the inline Dockerfile. WhileENTRYPOINTis reliable for Alpine, acommand: sleep infinityin the compose file would be more explicit. This is low severity and likely not a real bug.
docker-agent review: without --build, an image tagged e2e-additional-context-base:latest left over from a previous run would let up skip the build entirely, passing the test without exercising the additional_contexts-to-a-disabled-service path it exists to lock in. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Fixed in f29bd3d: added --build so a stale image from a previous run cannot let this step skip the build path the test exists to exercise. |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This review covers only the commits pushed since b22e94e.
The single change — adding --build to compose up -d in TestBuildAdditionalContextDisabledService — is correct and well-motivated. Without it, a stale image from a previous run could silently bypass the build step the test exists to exercise. The flag, its placement in the command, and the accompanying comment are all accurate. No bugs introduced.
Draft: adds a regression test for #14223, expected to fail until compose-go bumps to include the fix.
What this PR does, in one sentence
Locks in, as an e2e scenario, that a service reachable only through another service's
build.additional_contextsmust build even while its own profile is inactive — and must never start.Context
Reported in #14223: a build-only
baseservice with an inactive profile, referenced byclassroomviaadditional_contexts: base: service:base, makes the whole project fail to load —up,buildandup --no-buildalike — withservice "classroom" declares unknown service "base" as additional contexts base.The root cause lives entirely in compose-go's own consistency check (
loader/validate.go): it validatesadditional_contextstargets viaGetService, which errors on a disabled service, and — unlike thedepends_oncheck a few lines below, which already tolerates a disabled, non-required dependency — has no such tolerance foradditional_contexts. This blocks the project at load time, before docker/compose's ownaddBuildDependencies(pkg/compose/build.go) ever gets a chance to run — and that function already special-cases exactly this: it walksadditional_contextsintoDisabledServicesand re-enables what it finds, scoped to the build step only (seeWithServicesEnabled, called on adeepCopy, with the result only ever surfaced as a digest map — the enabled-for-build project never reaches the code that decides what to create or start). In other words, docker/compose already does the right thing here; it just never gets the chance to.A fix for the compose-go side is up at compose-spec/compose-go#931.
What the PR brings
TestBuildAdditionalContextDisabledService(pkg/e2e/build_test.go): aclassroomservice (active profile) buildingFROM baseviaadditional_contexts,base(inactive profile) providing only aBuild. Assertsclassroomreachesrunning,base's image gets built, andbaseitself is never created.🤖 Generated with Claude Code