Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new replicaset workload primitive to the operator-component-framework, mirroring the existing deployment/configmap primitive patterns so controllers can manage apps/v1.ReplicaSet resources with the same mutation/flavor/handler APIs.
Changes:
- Introduces
pkg/primitives/replicaset/with builder, resource wrapper, mutator, default handlers, and field-application flavors. - Adds a new
ReplicaSetSpecEditorunderpkg/mutation/editors/to support typed ReplicaSet spec mutations. - Adds a runnable example (
examples/replicaset-primitive/) and primitive docs (docs/primitives/replicaset.md).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/primitives/replicaset/resource.go | ReplicaSet resource wrapper + default field applicator (selector immutability handling). |
| pkg/primitives/replicaset/builder.go | Fluent builder wiring defaults (handlers, applicator, mutator factory, etc.). |
| pkg/primitives/replicaset/mutator.go | Plan/apply mutator with feature-boundary ordering and container/pod/spec editors. |
| pkg/primitives/replicaset/handlers.go | Default converging/grace/suspension handlers for ReplicaSet health & suspension. |
| pkg/primitives/replicaset/flavors.go | Field-application flavors to preserve labels/annotations (object + pod template). |
| pkg/primitives/replicaset/*_test.go | Unit tests for builder, mutator ordering, handlers, and flavors. |
| pkg/mutation/editors/replicasetspec.go | New typed editor for ReplicaSetSpec mutations (replicas, minReadySeconds, raw). |
| pkg/mutation/editors/replicasetspec_test.go | Unit tests for the new ReplicaSetSpecEditor. |
| examples/replicaset-primitive/** | Example controller, features, and resource factory showing primitive usage. |
| examples/replicaset-primitive/README.md | Documentation for running/understanding the example. |
| docs/primitives/replicaset.md | New end-user documentation for the replicaset primitive. |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
|
approved |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
… flavors Implements the ReplicaSet workload primitive following the same pattern as the Deployment primitive. Includes ReplicaSetSpecEditor, DefaultFieldApplicator that preserves the immutable spec.selector, and all standard workload lifecycle handlers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates building a ReplicaSet resource with feature mutations, flavors, data extraction, and suspension using the replicaset primitive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address GitHub Copilot PR review comments: - Fix DefaultFieldApplicator: capture ResourceVersion before DeepCopy overwrites it, so immutable spec.selector is correctly preserved on updates (was silently broken because ResourceVersion was always empty after the copy). - Fix comment typo: "custom customFieldApplicator" → "custom field applicator". - Fix docs capabilities table: remove "Failing" status and "Graceful rollouts" row that described unimplemented functionality. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comprehensive test coverage for the ReplicaSet Resource, matching the patterns established by the deployment primitive: - Identity format verification - Object deep copy semantics - Mutate with feature mutations and env var injection - Feature ordering across multiple mutations - DefaultFieldApplicator: creation applies all fields, update preserves immutable selector and ResourceVersion, desired object not mutated - Status handler delegation (ConvergingStatus, GraceStatus) with both custom mock handlers and default handler verification - DeleteOnSuspend with custom and default handlers - Suspend + Mutate integration (default scales to zero, custom handler) - SuspensionStatus with custom and default handlers - ExtractData captures container image from reconciled object - CustomFieldApplicator selective field application and error propagation Addresses Copilot review comment requesting resource-level test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Iterate by index instead of copying the range variable to avoid returning a pointer to a reused loop-variable copy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| // | ||
| // This function is used as the default handler by the Resource if no custom handler is registered via | ||
| // Builder.WithCustomSuspendMutation. It can be reused within custom handlers. | ||
| func DefaultSuspendMutationHandler(mutator *Mutator) error { |
There was a problem hiding this comment.
DefaultSuspendMutationHandler records mutations without calling BeginFeature(). Since EnsureReplicas() ultimately appends to m.active via EditReplicaSetSpec, this will panic if the caller hasn’t already started a feature plan. To make the handler robust and self-contained, either start a feature plan inside this handler (e.g., call mutator.BeginFeature()), or change the mutator to lazily create a plan when m.active is nil.
| func DefaultSuspendMutationHandler(mutator *Mutator) error { | |
| func DefaultSuspendMutationHandler(mutator *Mutator) error { | |
| if err := mutator.BeginFeature(); err != nil { | |
| return err | |
| } |
| func (m *Mutator) EditReplicaSetSpec(edit func(*editors.ReplicaSetSpecEditor) error) { | ||
| if edit == nil { | ||
| return | ||
| } | ||
| m.active.replicaSetSpecEdits = append(m.active.replicaSetSpecEdits, edit) | ||
| } |
There was a problem hiding this comment.
EditReplicaSetSpec (and the other Edit* / Ensure* methods) dereference m.active without a guard, so using the mutator without calling BeginFeature() will panic. Even if the framework “normally” calls BeginFeature(), this is easy to misuse (including from default handlers). Consider enforcing safety by either (a) lazily BeginFeature() when m.active == nil, or (b) returning an error from Apply() if no plan was started and documenting the contract clearly (prefer (a) if you want the API to be hard to misuse).
| ) *Builder { | ||
| if extractor != nil { | ||
| b.base.WithDataExtractor(func(rs *appsv1.ReplicaSet) error { | ||
| return extractor(*rs) |
There was a problem hiding this comment.
WithDataExtractor passes *rs by value, which is only a shallow copy of a ReplicaSet (maps/slices/pointers remain shared). In resource.go, the public contract says extractors receive a deep copy to prevent accidental mutation. To make that contract true at this boundary, pass a deep copy into the user function (e.g., extractor(*rs.DeepCopy())) or change the extractor signature to accept a pointer to a deep-copied object.
| return extractor(*rs) | |
| if rs == nil { | |
| var empty appsv1.ReplicaSet | |
| return extractor(empty) | |
| } | |
| return extractor(*rs.DeepCopy()) |
| Namespace: "test-ns", | ||
| }, | ||
| } | ||
| res, _ := NewBuilder(rs).Build() |
There was a problem hiding this comment.
Tests are discarding the Build() error in multiple places. If builder validation changes, these tests could start passing a nil resource and then fail later with less actionable panics. Prefer require.NoError(t, err) (and require.NotNil(t, res) where needed) for clearer failures at the correct source.
| run-examples: ## Run all examples to verify they execute without error. | ||
| go run ./examples/deployment-primitive/. | ||
| go run ./examples/configmap-primitive/. | ||
| go run ./examples/replicaset-primitive/. |
There was a problem hiding this comment.
The PR checklist states “Does not modify shared files,” but this PR modifies shared repo files (e.g., Makefile, docs/primitives.md, and adds pkg/mutation/editors/replicasetspec.go). Update the PR description/checklist to reflect the actual scope, or adjust the change set if those shared modifications weren’t intended.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
Implements the
replicasetKubernetes resource primitive following the pattern established by the existingConfigMapandDeploymentprimitives.Summary
replicasetprimitive package underpkg/primitives/replicaset/Checklist