adopt compose-go jobs and container-spec layering - #14093
Conversation
f6ea219 to
ccaa8ce
Compare
ccaa8ce to
31714e5
Compare
8114744 to
3811fa7
Compare
3811fa7 to
0a8ee1b
Compare
glours
left a comment
There was a problem hiding this comment.
A few things worth fixing before this lands, even given the interim/client-side nature of this jobs support:
cmd/compose/up.go:362 — warnIgnoredJobs iterates project.AllJobs() (includes profile-disabled jobs), while rejectScheduledJobs (up.go:379, also called from create.go:104 and start.go:63) correctly uses project.Jobs (profile-enabled
only). Result: up can warn about jobs that aren't even part of this invocation's profile selection. Should use project.Jobs here too.
cmd/compose/run.go:287-292 — the job-detection fallback now retries the full project load on any narrowed-load error, not just "no such service". Any unrelated load failure (bad include:, interpolation error) now triggers a second full
load — duplicating remote include: fetches and unsupported-attribute warnings — before falling through to the (still correct) original error. Worth narrowing the retry condition back to the selection-failure case, since job detection doesn't
need the broader retry.
pkg/compose/pre_start_test.go — still no test exercising the security-sensitive fields (Privileged, CapAdd/CapDrop, SecurityOpt, ReadOnly, Sysctls) that hooks now inherit from the service by default. This is pre-existing/permanent
hook behavior, not something the interim jobs work will replace — worth covering given what's at stake if inheritance breaks silently.
pkg/compose/publish.go — the new job branches (pushApplicationIndex, generateImageDigestsOverride, collectEnvCheckFindings, checkOnlyBuildSection, checkForBindMount, checkForSensitiveData) are correct on inspection but have zero
dedicated test coverage — publish_test.go's only changes are mechanical ContainerSpec adaptations. This is the secret-scanning gate for publish; a future regression here would go undetected by CI.
Nits, non-blocking:
- pre_start.go:
resolveHookServiceReferencesrebuilds the per-service container map inline instead of reusinggetContainersByService. - publish.go: findings for a job are still labeled
"service %q: ..."in the merged prompt message.
|
Addressed in d04a245, 447c867, dff0e0e:
All new/changed tests fail without their respective fix, confirmed locally. build/vet/lint/tests all green. |
compose-spec/compose-go#866 is merged: jobs top-level element, container specification layered as ContainerSpec/WorkloadSpec, pre_start hooks carrying the full container specification resolved at load time. Bump to the merged head and adapt in the same movement — composite literals setting moved fields wrap them into the embedded ContainerSpec / WorkloadSpec (promoted field access was already source-compatible, so this is literal-only, no behavior change), and pre_start handling is typed against PreStartHook. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
… start The model may declare jobs this runtime cannot execute yet. Commands that materialize the application warn about them, and refuse an active (profile-enabled) scheduled job before any resource is created — up, create and start alike: silently not scheduling would break expectations, while manual jobs just wait for an explicit trigger. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
dff0e0e to
07f3de8
Compare
|
Rebased onto main (07f3de8) — root cause of the lint/test failure: the relay work merged into main after this branch was cut added tests using the pre-layering ServiceConfig shape (DependsOn/Networks set directly, PreStart as []ServiceHook), which don't compile against the ContainerSpec/WorkloadSpec split this branch adopts. GitHub tests the hypothetical merge of the PR into main, which is why this only showed up in CI and not locally against the branch's own tip. Adapted both files to the new shape; not a real regression in the reviewed commits. Full build/vet/lint/test green locally afterward (pkg/watch's fsevents flakes aside, unrelated and pre-existing). |
`docker compose run <job>` materializes the job — a ContainerSpec + WorkloadSpec, the same layers a service is made of — as a service for the one-off machinery: its profile is activated, the project narrowed to its dependencies by WithSelectedJob, and the exit code flows back. The materialization happens during project loading, BEFORE service selection and environment resolution: the job's own env_file and label_file resolve exactly like a selected service's would, unrelated services' env_file still don't need to exist, and a target that is neither a service nor a job keeps the precise selection error (the retry is keyed on "the target is a declared job", never on matching an error message). A service and a job may share a name: the service wins, matching what the selector resolved. A job may depend on other jobs: the whole job closure materializes, so every depends_on edge resolves to a runnable service satisfying its declared condition — a dependency job runs to completion through the exact machinery a service dependency does — instead of dangling as an unresolvable name. Job extensions (x-*) survive materialization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
pre_start hooks carry the full container specification, but the init-container runner only consumed a handful of attributes (image, command, user, env, workdir) — everything else was silently dropped. Instead of wiring attributes one by one, the hook's specification is merged over the service's ContainerSpec through compose-go's own file merge machinery (override.Merge on the canonical yaml tree): command and entrypoint replace, environment merges per key with the hook winning, extra_hosts and dns accumulate entries, ulimits merge — the exact per-attribute rules of multi-file compose, maintained in one place. Every ContainerSpec attribute inherits this way, current and future, with zero attribute-specific code. The merged spec then runs through the standard create path (getCreateConfigs) as a service-shaped one-off, so resources, capabilities, dns, sysctls, logging... materialize exactly as they would for a service container. Hook containers keep their minimal labels and carry no container-number, so tooling telling replicas apart does not count them. The only deliberate exception is volumes: mounts inherit at runtime through volumes_from — the only mechanism that shares the service's anonymous and image volumes — and the hook's own volume declarations, materialized by the standard path, take precedence per target. This is what lets an init container get read-write access to a volume the service mounts read-only (fixes: see PR). e2e scenarios lock extra_hosts inheritance and accumulation, volume override and completion, and the unit tests pin the merge rules. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…chain Scenario-DSL coverage for the jobs entry points: - up refuses a project declaring an active scheduled job before creating any resource; - run executes a manual job like a service, starting its depends_on services first, and refuses a schedule-only job; - a job's own env_file feeds its environment through run — the materialization happens before environment resolution; - a job depending on another job runs the dependency to completion first, through the exact machinery a service dependency uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Per the spec, any job can be triggered manually by an explicit run command, its automated triggers notwithstanding — so run now accepts scheduled jobs too. The exception is a job explicitly declared with triggers.manual: false, which run rejects: meant for scheduled jobs whose out-of-schedule execution would be harmful. compose-go is bumped to the jobs-branch commit making Manual tri-state (*bool) and allowing manual and schedule to be combined. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
compose-go resolves each hook against its service at load time: the model itself carries the full container specification a hook runs with, and the runtime consumes it as-is through the standard create path — the runtime-side merge helper goes away. Consuming the full spec means honoring ALL of it: - service references it may carry — volumes_from entries, and service:-scoped network_mode/ipc/pid, inherited or declared — resolve to live container IDs exactly like the service create path does; the daemon knows nothing about service names and rejected them, failing up for any service combining volumes_from with a pre_start hook (locked by an e2e scenario). - hook labels — declared or inherited — merge into the container's labels, the runtime identification set winning on conflicts. - the <API 1.44 network-connect fallback joins the HOOK's networks, not the parent service's: a hook overriding networks was connected to the wrong ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Every publish safeguard and image-pinning path used to walk only project.Services, leaving jobs invisible: - the sensitive-data checks (literal environment values, env_file scans, bind-mount warnings, build-only rejection) now cover jobs — a job declaring AWS_SECRET_ACCESS_KEY=... was published without the guard rail a service gets; - the image-digest override pins job images too (jobs dressed as services run through the exact WithImagesResolved semantics), so the published artifact is reproducible for jobs as well; - the application index references job images; - `config --images` lists them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Rebasing onto main pulled in the relay work (added after this branch was cut), whose tests build ServiceConfig/PreStart literals against the flat, pre-layering shape: DependsOn/Networks set directly, and PreStart typed as []ServiceHook. Both are incompatible with the ContainerSpec/WorkloadSpec split this branch adopts -- the promoted DependsOn/Networks fields need go1.27 to set via a flat literal (this module stays on 1.26.3), and PreStart is now []PreStartHook, a full container specification, not a plain hook. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
95afff2 to
d28ad4c
Compare
The fallback reload runProject uses to detect a job target passed warnUnsupportedAttributes, the same option as the first, selective load right before it -- which had already fired every warning once. For the common case where the target is neither a service nor a job (a plain typo), every finding got logged twice before falling through to the original, correct error anyway. skipUnsupportedAttributesWarning already exists for exactly this secondary-load case (see its other call sites in compose.go and completion.go); use it here too. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…mage-digests Only the interpolated path (imagesOnly) and pkg/compose/publish.go walked project.Jobs the same way they walk project.Services. Three sibling paths in config.go were missed: - runConfigImages (--images) only printed service images. - lockModel (--no-interpolate --lock-image-digests) dropped the top-level "jobs" key entirely along with every other non-services key, instead of filtering it down to image/type:image-volumes like services. - resolveImageDigests (--no-interpolate --resolve-image-digests) only built its pseudo-project from model["services"], so job images and type: image volume sources never resolved -- confirmed by a new test that failed against the first attempt at this fix (job image stayed unpinned) until dressed as its own pseudo-project the same way generateImageDigestsOverride in publish.go already does, since WithImagesResolved only walks a project's Services, never its Jobs. The extra complexity that last one picked up is split into resolveServiceImageDigests (services, unchanged from before this commit) and resolveJobImageDigests (new), called from a small dispatcher. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker compose create <job> / start <job> failed with the raw compose-go error "no such service: <name>", since project loading rejects the job name before rejectScheduledJobs or any job-aware logic ever runs -- unlike run, neither command knows how to detect that a name is a declared job. The result gave no hint that jobs exist or that run is the right command. Jobs stay run-only by design; this only replaces the error message when the target is a declared job, via a small helper (jobTargetErr) shared by both commands, reloading unselected exactly like run's own job-detection fallback. Any other "no such service" (a real typo) keeps the original, precise error. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
materializeJobClosure walked job-typed depends_on edges without re-checking triggers.manual: false on anything but the top-level run target, so `docker compose run A` where A depends_on job B (manual: false) silently ran B anyway. manual: false declares a job harmful to trigger outside its schedule; depends_on doesn't change who caused the execution or when -- pulling B in to satisfy A's dependency is still the run command causing that out-of-schedule execution, one hop removed. Refuse it the same way the top-level check already does, before anything in the closure is created. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Addressed the follow-up review:
All four verified locally: build/vet/lint/test green ( |
jobTargetErr was called with the full [SERVICE...] argument list, but compose-go's selection error only ever names the one target it actually failed on. The loop matched any name in the list against the job set, so `docker compose create totally-bogus migrate` (a real typo plus a legitimate job) reported the job error instead of the real "no such service: totally-bogus", masking the typo. Match the name the error actually names, not any job name that happens to share the invocation. unselectedJobs is extracted out of jobTargetErr so start's own project-resolution fallback (next commit) can reuse the same unselected-reload logic. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
projectOrName falls back to a label-driven, file-less project on any load failure when COMPOSE_PROJECT_NAME is set -- swallowing the load error entirely, including "no such service" for a target that's actually a declared job. Since that branch returns err == nil, jobTargetErr (gated on err != nil) and rejectScheduledJobs (gated on project != nil) were both skipped: `docker compose start migrate` fell through to a label-driven start that finds no container for a job that was never run, and exited 0 having silently done nothing. Refuse a named job explicitly on that fallback path too, reusing unselectedJobs from the previous commit. Also adds the test coverage this and the previous fix (job-target error messages on create/start) were entirely missing: create.go and start.go had no unit tests at all, and pkg/e2e/jobs_test.go only exercised up/run. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Addressed a third-party review of the previous commit (c116397) plus an independent fresh-eyes pass I ran separately, both converging on the same two real gaps in
All verified locally: build/vet/lint/test green, including a manual regression check on the correlation fix (reverting it back-to-back against the new unit test confirmed it catches the bug). |
The previous fix only covered create/start. projectOrName backs every other service-targeting command too (stop, kill, pause, unpause, logs, rm, down, ps, events), so they still surfaced the same two problems: a raw "no such service" error for a job name, and -- when COMPOSE_PROJECT_NAME is set -- a silent no-op, since projectOrName's label-driven fallback swallows the load error entirely. Fold the check into projectOrName so every caller is fixed at once, instead of duplicating the wiring per command. start.go's own job-handling code, now redundant, is removed; jobTargetErr and unselectedJobs (cmd/compose/run.go) are unchanged, just called from one more place. Adds unit coverage for projectOrName itself (including the COMPOSE_PROJECT_NAME path and the typo-correlation edge case) and e2e coverage for two more callers (stop, down) to confirm the fix lives in the shared helper rather than being re-implemented per command. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
glours
left a comment
There was a problem hiding this comment.
Sounds like a good 1st iteration
Adopts compose-spec/compose-go#866 (jobs top-level element, container specification layered as ContainerSpec/WorkloadSpec, pre_start hooks carrying the full container specification).
This first step adapts the types and gives jobs their first runtime entry point:
composite literals setting moved fields are wrapped into the embedded
ContainerSpec/WorkloadSpec(no behavior change; promoted field access was already source-compatible)pre_starthandling is typed againstPreStartHook; exec hooks (post_start/pre_stop) lose the image field they never honored — now enforced by constructionupwarns about declared jobs (pointing atdocker compose runto trigger them) and fails on active (profile-enabled) scheduled jobs withscheduled jobs are not supported in this version: silently not scheduling would break expectations, while manual jobs just wait for an explicit triggerdocker compose run <job>executes a job exactly like a service: the job — aContainerSpec+WorkloadSpec, the same layers a service is made of — is materialized as a service for the one-off machinery; its profile is activated, its dependencies start as they would for a service, the exit code flows back. Per the spec, manual execution is always available — scheduled jobs included — unless the job explicitly opts out withtriggers.manual: false(tri-state*boolin compose-go), which run rejects. All locked by e2e scenarios (scheduled rejection byupbefore any resource is created; manual run starting itsdepends_onservices first;manual: falserefusal).pre_starthooks inherit the whole container specification, resolved at load time: compose-go completes each hook with the service's container-spec attributes while loading (idempotently acrossconfiground-trips, thanks to the strictly-identical-entry merge dedup), so the model itself carries the spec a hook runs with and the runtime consumes it as-is through the standard create path — everyContainerSpecattribute (resources, capabilities, dns, sysctls, ...) inherits with zero attribute-specific code, following the exact multi-file merge rules (command replaces, environment merges per key, extra_hosts accumulate). The only deliberate exception is volumes, inherited at runtime throughvolumes_from(the only way to share anonymous and image volumes), with hook-declared mounts taking precedence per target. This closes the two standing asks against init containers: inheriting/declaringextra_hosts(fixes pre_start containers don't inherit/support extra_hosts #13939) and giving the hook read-write access to a volume the service mounts read-only (fixes pre_start/init-Containers should support own/overwritten Volumes #13934) — both locked by e2e scenarios.compose-spec/compose-go#866 is merged:
go.modnow points at the merged compose-go head (v2.15.1-0.20260910154416-11feead015fc), no replace directive involved — this PR is self-contained and mergeable. Jobs are hereby exposed end to end: declared in the model, warned about and guarded byup(scheduled jobs rejected until a scheduler exists), and runnable throughdocker compose run <job>. A compose-go tagged release can still substitute the pseudo-version before merge if preferred.🤖 Generated with Claude Code
Update (independent review follow-ups). An adversarial review pass surfaced real gaps, all addressed in the rebuilt stack:
volumes_from(andservice:-scopednetwork_mode/ipc/pid) inherited into apre_starthook now resolve to live container IDs like the service create path — the daemon used to receive raw service names and failupfor any service combiningvolumes_fromwith a hook (e2e-locked). Hooklabelsmerge into the container's labels (the runtime identification set wins), and the <API 1.44 network fallback joins the hook's own networks.env_file/label_fileresolve throughrun: materialization now happens during loading, before environment resolution — and the retry is keyed on "the target is a declared job", never on matching an error message, so a plain typo keeps the precise selection error and unrelated services' env_file still don't need to exist.depends_onexecutes: the whole job closure materializes, each dependency running to completion through the machinery a service dependency uses (e2e-locked). Materialized jobs carry the standard compose labels — without them their containers were invisible to every label-driven path — and theirx-*extensions.createandstartrefuse active scheduled jobs likeupdoes, keeping the "before any resource is created" promise on every entry point.publishsafeguards and image pinning cover jobs: sensitive-env/env_file/bind-mount/build-only checks, digest override, application index, andconfig --images.Note on config hashes: the container-spec layering reorders
ServiceConfigfields, which would have changed every service's config-hash (mass recreation on upgrade) — #14215 pins the hash byte layout to its historical form — fully backward compatible, zero recreation — and must land first; this PR then deletes its transient continuity test in the compose-go bump commit, as that test documents.