Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new pv primitive to the framework to manage Kubernetes PersistentVolume resources using the existing primitive patterns (builder/resource/mutator/flavors/status handler), plus documentation and an example.
Changes:
- Introduces
pkg/primitives/pv/with builder, resource wrapper, mutator, flavors, and operational status handler (+ unit tests). - Adds a shared
PVSpecEditorunderpkg/mutation/editors/to support typed PV spec mutations (+ tests). - Adds a runnable
examples/pv-primitive/example and new documentation pagedocs/primitives/pv.md.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/primitives/pv/resource.go | PV integration resource wrapper + default PV field applicator preserving immutable fields |
| pkg/primitives/pv/builder.go | Fluent builder for PV resources with mutations, flavors, operational status handler, extractors |
| pkg/primitives/pv/mutator.go | PV mutator implementing the “plan-and-apply” pattern with metadata/spec edit ordering |
| pkg/primitives/pv/handlers.go | Default operational status mapping from PV phases to framework operational statuses |
| pkg/primitives/pv/flavors.go | PV-specific wrappers for common field-application flavors |
| pkg/primitives/pv/*_test.go | Unit tests for builder, mutator ordering, flavors, and status handler |
| pkg/mutation/editors/pvspec.go | New shared typed editor for PersistentVolumeSpec |
| pkg/mutation/editors/pvspec_test.go | Unit tests for the new PVSpecEditor |
| examples/pv-primitive/main.go | Demonstration program showing PV mutation behavior and status handler output |
| examples/pv-primitive/resources/pv.go | Example factory wiring PV builder + example feature mutations + flavors |
| examples/pv-primitive/features/mutations.go | Example feature-gated PV mutations for the demo |
| examples/pv-primitive/app/controller.go | Example package-level commentary about PV ownership constraints |
| examples/pv-primitive/README.md | Example documentation and run instructions |
| docs/primitives/pv.md | New primitive documentation page describing PV builder/mutation/status/flavors |
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:
<!-- claude-review-cycle --> |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
Introduces a typed editor for corev1.PersistentVolumeSpec with methods for capacity, access modes, reclaim policy, storage class, mount options, volume mode, and node affinity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the PV primitive as a cluster-scoped Integration resource: - Builder validates Name only (no namespace for cluster-scoped resources) - DefaultFieldApplicator preserves immutable fields on existing PVs (volume source, volume mode, claim ref) - DefaultOperationalStatusHandler reports status based on PV phase - Mutator with plan-and-apply pattern for metadata and spec edits - Flavors for preserving external labels and annotations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the PV primitive's integration lifecycle, cluster-scoped identity, immutable field preservation, mutation pipeline, editors, flavors, operational status mapping, and usage guidance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates PV builder, mutations, flavors, and operational status: - Cluster-scoped identity (no namespace) - Immutable field preservation on existing PVs - Boolean-gated mutations for reclaim policy and mount options - PreserveCurrentAnnotations flavor for external fields - Operational status mapping from PV phases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix compile error: use Spec.PersistentVolumeSource.HostPath instead of non-existent Spec.HostPath field, with nil check - Update README to accurately describe what the example does (in-memory mutations, not fake client; operational status, not status conditions) - Fix controller.go comment to remove reference to non-existent SkipOwnerRef option Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover create-vs-update behavior, immutable field preservation (PersistentVolumeSource, VolumeMode, ClaimRef), nil immutable field handling, and verify the desired object is not mutated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { | ||
| if edit == nil { | ||
| return | ||
| } | ||
| m.active.metadataEdits = append(m.active.metadataEdits, edit) | ||
| } |
There was a problem hiding this comment.
EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If a caller forgets to call BeginFeature() first (possible since NewMutator leaves active nil), this will panic at runtime. Consider making the mutator safe-by-default by auto-starting an initial plan (e.g., initialize plans/active in NewMutator, or lazily call BeginFeature() inside these methods when m.active == nil) so misuse can’t crash reconciliation.
| func (m *Mutator) EditPVSpec(edit func(*editors.PVSpecEditor) error) { | ||
| if edit == nil { | ||
| return | ||
| } | ||
| m.active.specEdits = append(m.active.specEdits, edit) | ||
| } |
There was a problem hiding this comment.
EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If a caller forgets to call BeginFeature() first (possible since NewMutator leaves active nil), this will panic at runtime. Consider making the mutator safe-by-default by auto-starting an initial plan (e.g., initialize plans/active in NewMutator, or lazily call BeginFeature() inside these methods when m.active == nil) so misuse can’t crash reconciliation.
pkg/primitives/pv/handlers.go
Outdated
| // A PV in the Pending phase is reported as OperationPending. A PV in the | ||
| // Released or Failed phase is reported as OperationFailing. |
There was a problem hiding this comment.
The comment refers to OperationPending / OperationFailing, but the implementation returns concepts.OperationalStatusPending / concepts.OperationalStatusFailing. Updating the comment to match the actual status names will avoid confusion for readers searching for those identifiers.
| // A PV in the Pending phase is reported as OperationPending. A PV in the | |
| // Released or Failed phase is reported as OperationFailing. | |
| // A PV in the Pending phase is reported as concepts.OperationalStatusPending. A PV in the | |
| // Released or Failed phase is reported as concepts.OperationalStatusFailing. |
docs/primitives/pv.md
Outdated
|
|
||
| | Capability | Detail | | ||
| | ------------------------- | -------------------------------------------------------------------------------------------------- | | ||
| | **Integration lifecycle** | Reports `Operational`, `OperationPending`, or `OperationFailing` based on the PV's phase | |
There was a problem hiding this comment.
The doc uses OperationPending / OperationFailing, but the code (and tests) use concepts.OperationalStatusPending / concepts.OperationalStatusFailing. Please align the documentation terminology with the actual framework statuses (and keep it consistent with handlers.go and other primitive docs) to prevent users from looking for non-existent status values.
| | **Integration lifecycle** | Reports `Operational`, `OperationPending`, or `OperationFailing` based on the PV's phase | | |
| | **Integration lifecycle** | Reports `Operational`, `concepts.OperationalStatusPending`, or `concepts.OperationalStatusFailing` based on the PV's phase | |
Update documentation and handler comments to use the actual concepts.OperationalStatusPending / concepts.OperationalStatusFailing / concepts.OperationalStatusOperational identifiers instead of the shorthand OperationPending / OperationFailing / Operational names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
| func (m *Mutator) BeginFeature() { | ||
| m.plans = append(m.plans, featurePlan{}) | ||
| m.active = &m.plans[len(m.plans)-1] | ||
| } |
There was a problem hiding this comment.
EditObjectMetadata / EditPVSpec dereference m.active unconditionally. If BeginFeature() hasn’t been called (or if future refactors call these before BeginFeature()), this will panic at runtime. Consider making the mutator resilient by ensuring an active plan exists (e.g., lazily calling BeginFeature() the first time an edit is registered), or by failing fast with a dedicated, clearer panic/message rather than a nil-pointer dereference.
| type Mutator struct { | ||
| pv *corev1.PersistentVolume | ||
|
|
||
| plans []featurePlan | ||
| active *featurePlan | ||
| } |
There was a problem hiding this comment.
Storing active as a pointer to a slice element is fragile because any future append/re-slice patterns on m.plans could invalidate pointers and make bugs hard to debug. A more robust approach is to store an activeIndex int (or *featurePlan allocated independently), and reference m.plans[m.activeIndex] when appending edits.
| func (m *Mutator) BeginFeature() { | ||
| m.plans = append(m.plans, featurePlan{}) | ||
| m.active = &m.plans[len(m.plans)-1] | ||
| } |
There was a problem hiding this comment.
Storing active as a pointer to a slice element is fragile because any future append/re-slice patterns on m.plans could invalidate pointers and make bugs hard to debug. A more robust approach is to store an activeIndex int (or *featurePlan allocated independently), and reference m.plans[m.activeIndex] when appending edits.
Implements the
pvKubernetes resource primitive following the pattern established by the existingConfigMapandDeploymentprimitives.Summary
pvprimitive package underpkg/primitives/pv/Checklist