Conversation
There was a problem hiding this comment.
Pull request overview
Implements a new Kubernetes Job primitive for the operator-component-framework, mirroring the existing primitive patterns (builder/resource/handlers/mutator/flavors), and adds an example + documentation to demonstrate usage.
Changes:
- Added
pkg/primitives/job/with builder/resource/handlers/flavors and aJob-specific mutator. - Added a shared
JobSpecEditorunderpkg/mutation/editors/with tests. - Added a runnable
examples/job-primitive/plus new documentation atdocs/primitives/job.md.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/primitives/job/resource.go | Job primitive Resource wrapper over internal/generic.TaskResource. |
| pkg/primitives/job/builder.go | Builder for configuring Job resources (mutations, flavors, handlers, extractors). |
| pkg/primitives/job/handlers.go | Default completion + suspension handlers for Jobs. |
| pkg/primitives/job/mutator.go | Plan/apply mutator for Job (metadata/spec/pod/container mutations). |
| pkg/primitives/job/flavors.go | Field application flavors for preserving live labels/annotations. |
| pkg/primitives/job/*_test.go | Unit tests for builder, handlers, mutator, and flavors. |
| pkg/mutation/editors/jobspec.go | New shared typed editor for batchv1.JobSpec. |
| pkg/mutation/editors/jobspec_test.go | Tests for the new JobSpecEditor. |
| examples/job-primitive/** | Example controller + features demonstrating the job primitive. |
| docs/primitives/job.md | User-facing documentation for the new Job primitive API and behavior. |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed: Intentionally ignored:
|
Implements the Job primitive using the Task lifecycle (Completable, Suspendable) from internal/generic. Includes default handlers for completion status, suspension via Job.Spec.Suspend, and deletion on suspend. Follows the same pod-template mutation pattern as Deployment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Provides SetCompletions, SetParallelism, SetBackoffLimit, SetActiveDeadlineSeconds, SetTTLSecondsAfterFinished, and SetCompletionMode methods with full test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the Mutator for Job resources following the same pattern as the Deployment mutator. Supports editing job metadata, job spec, pod template metadata, pod spec, and container presence/edits with snapshot semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates building a Job primitive with version-gated image updates, tracing env vars, retry policy configuration, field preservation flavors, custom completion status handler, and data extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers capabilities, building, mutations, internal ordering, editors, convenience methods, suspension behavior, flavors, and guidance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use corev1.ConditionTrue instead of string literal "True" in status condition checks (handlers.go) - Fix range variable address issue in findEnv test helper by iterating by index (mutator_test.go) - Fix doc comments to reference concepts.X instead of component.X for interface types (resource.go) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ce tests - Use cond.Reason as fallback when cond.Message is empty in Job failure status handler, since Kubernetes often provides details in Reason only - Fix doc comments to reference component.Resource instead of concepts.Resource, matching conventions used by other primitives - Add resource_test.go with comprehensive coverage for Resource-level behaviors including Suspend/DeleteOnSuspend wiring, SuspensionStatus and ConvergingStatus delegation, ExtractData, and custom field applicator Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates that a container added by one mutation can be selected and configured by a subsequent mutation, exercising the feature-boundary semantics of the mutator's plan-and-apply pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Match the pattern used by deployment/configmap primitives: initialize with an empty featurePlan directly instead of calling beginFeature(), which would create a duplicate empty feature when the generic mutate_helper also calls beginFeature(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align with upstream architectural fixes: export beginFeature() to BeginFeature() to satisfy the FeatureMutator interface, and add status preservation to DefaultFieldApplicator so spec-level reconciliation does not clear server-written status data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c8fbe86 to
8f166b9
Compare
| @@ -0,0 +1,52 @@ | |||
| package editors | |||
There was a problem hiding this comment.
The PR description checklist says "Does not modify shared files", but this PR adds a new shared editor under pkg/mutation/editors/ (and also updates shared docs). Please either update the checklist item (if shared changes are expected for this primitive) or move/encapsulate the shared editor changes so the checklist remains accurate.
pkg/primitives/job/resource.go
Outdated
| // The return value includes a descriptive status (Completed, TaskRunning, TaskPending, | ||
| // or TaskFailing) and a human-readable reason, which are used to update the component's | ||
| // conditions. |
There was a problem hiding this comment.
The comment references statuses TaskRunning, TaskPending, and TaskFailing, but the code (and tests) use concepts.CompletionStatusRunning/Pending/Failing. Please align the terminology in this comment (and any similar text in docs) to the actual concepts.CompletionStatus* names to avoid confusing API consumers.
| // The return value includes a descriptive status (Completed, TaskRunning, TaskPending, | |
| // or TaskFailing) and a human-readable reason, which are used to update the component's | |
| // conditions. | |
| // The return value includes a descriptive status (concepts.CompletionStatusCompleted, | |
| // concepts.CompletionStatusRunning, concepts.CompletionStatusPending, or | |
| // concepts.CompletionStatusFailing) and a human-readable reason, which are used to | |
| // update the component's conditions. |
| func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { | ||
| if edit == nil { | ||
| return | ||
| } | ||
| m.active.jobMetadataEdits = append(m.active.jobMetadataEdits, edit) | ||
| } |
There was a problem hiding this comment.
m.active is dereferenced without a guard; calling any of the Edit*/Ensure* methods before BeginFeature() will cause a nil-pointer panic. Since this is a public helper type, consider adding a small internal guard (e.g., an ensureActive() that either initializes a plan or panics with a clear message) so failures are deterministic and actionable rather than a generic nil dereference.
| builder.WithDataExtractor(func(j batchv1.Job) error { | ||
| fmt.Printf("Reconciling job: %s, active: %d, succeeded: %d, failed: %d\n", | ||
| j.Name, j.Status.Active, j.Status.Succeeded, j.Status.Failed) | ||
|
|
||
| // Print the complete job resource object as yaml | ||
| y, err := yaml.Marshal(j) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal job to yaml: %w", err) | ||
| } | ||
| fmt.Printf("Complete Job Resource:\n---\n%s\n---\n", string(y)) | ||
|
|
||
| return nil | ||
| }) |
There was a problem hiding this comment.
This example dumps the full Job object (including pod template) to stdout. In real usage, that can inadvertently expose sensitive env vars or other configuration in logs. Consider either removing the full YAML dump, redacting sensitive fields, or gating it behind an explicit debug flag/logger configuration (while keeping the rest of the example intact).
…ant names Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Resolve conflicts in docs/primitives.md (add job to expanded primitives table), jobspec.go (take main's doc comments), and jobspec_test.go (combine require.NotNil checks with editor variable naming convention). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| | `pkg/primitives/statefulset` | Workload | [statefulset.md](primitives/statefulset.md) | | ||
| | `pkg/primitives/replicaset` | Workload | [replicaset.md](primitives/replicaset.md) | | ||
| | `pkg/primitives/daemonset` | Workload | [daemonset.md](primitives/daemonset.md) | | ||
| | `pkg/primitives/job` | Task | [job.md](primitives/job.md) | |
There was a problem hiding this comment.
PR description checklist says this PR “does not modify shared files”, but this PR updates shared docs (this file) and also changes shared editor tests (pkg/mutation/editors/jobspec_test.go). Please either update the checklist/description to reflect these shared changes, or revert them if they were unintended.
| | Capability | Detail | | ||
| | ----------------------- | ----------------------------------------------------------------------------------------------- | | ||
| | **Completion tracking** | Monitors Job conditions and reports `Completed`, `TaskRunning`, `TaskPending`, or `TaskFailing` | | ||
| | **Suspension** | Sets `spec.suspend=true` or deletes the Job (default); reports `Suspending` / `Suspended` | |
There was a problem hiding this comment.
The capabilities table says suspension “sets spec.suspend=true or deletes the Job (default)”, but in the framework suspension flow Suspend() is still called even when DeleteOnSuspend() is true, and deletion only happens after the resource reaches Suspended. Consider rewording this to avoid implying these are mutually exclusive (e.g., suspend/stop creation, then delete by default once suspended).
| | **Suspension** | Sets `spec.suspend=true` or deletes the Job (default); reports `Suspending` / `Suspended` | | |
| | **Suspension** | Suspends/stops the Job (for example via `spec.suspend=true`) and, by default, deletes it once `Suspended`; reports `Suspending` / `Suspended` | |
Implements the
jobKubernetes resource primitive following the pattern established by the existingConfigMapandDeploymentprimitives.Summary
jobprimitive package underpkg/primitives/job/Checklist