Skip to content

feat: implement pdb primitive#24

Merged
sourcehawk merged 25 commits intomainfrom
feature/pdb-primitive
Mar 25, 2026
Merged

feat: implement pdb primitive#24
sourcehawk merged 25 commits intomainfrom
feature/pdb-primitive

Conversation

@sourcehawk
Copy link
Owner

@sourcehawk sourcehawk commented Mar 22, 2026

Implements the pdb Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds pdb primitive package under pkg/primitives/pdb/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Adds new editor (pdbspec) and docs entry following established patterns for new primitives

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new PodDisruptionBudget (PDB) primitive to the operator component framework, following the existing static-primitive patterns (e.g., ConfigMap) and providing a typed mutation/editor surface plus documentation and an end-to-end example.

Changes:

  • Introduces pkg/primitives/pdb (resource, builder, mutator, flavors) for managing policy/v1 PodDisruptionBudgets.
  • Adds a new typed editor PodDisruptionBudgetSpecEditor under pkg/mutation/editors to support structured PDB spec mutations.
  • Adds documentation (docs/primitives/pdb.md, updates docs/primitives.md) and a runnable example (examples/pdb-primitive/*).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/primitives/pdb/resource.go Static PDB primitive resource wrapper over the generic static resource.
pkg/primitives/pdb/builder.go Fluent builder for configuring PDB field applicator, flavors, mutations, extractors.
pkg/primitives/pdb/mutator.go Plan-and-apply mutator with metadata/spec edit categories.
pkg/primitives/pdb/flavors.go Field-application flavors for preserving current labels/annotations.
pkg/primitives/pdb/*_test.go Unit tests for builder/mutator/flavors behavior.
pkg/mutation/editors/pdbspec.go New typed editor for PodDisruptionBudgetSpec.
pkg/mutation/editors/pdbspec_test.go Unit tests for the new PDB spec editor.
docs/primitives/pdb.md New primitive documentation page for PDB.
docs/primitives.md Adds PDB primitive and editor to the docs index tables.
examples/pdb-primitive/* New runnable example demonstrating the PDB primitive in a fake-client reconcile loop.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • No code changes were necessary.

Intentionally ignored:

  • docs/primitives.md L127 — "shared files" checklist discrepancy: This comment claims the PR metadata checklist says "Does not modify shared files," but the actual PR checklist explicitly states: "Adds new editor (pdbspec) and docs entry following established patterns for new primitives." There is no discrepancy. Adding a new editor under pkg/mutation/editors/ and a row in docs/primitives.md is the established pattern that every primitive follows (ConfigMap, Deployment, and now PDB). The shared file additions are minimal, expected, and already acknowledged in the PR description.

Verification:

  • go build ./... — compiles cleanly
  • go test ./... — all tests pass

Copy link
Owner Author

@sourcehawk sourcehawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cycle 1: Copilot review comments addressed. Re-requesting review.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • None required — build and all tests pass cleanly.

Intentionally ignored:

  • docs/primitives.md L127 — Copilot claimed the PR checklist says "Does not modify shared files", but the actual PR checklist contains no such item. The checklist explicitly acknowledges "Adds new editor (pdbspec) and docs entry following established patterns for new primitives." Adding a typed editor under pkg/mutation/editors/ and a row in the shared docs table is the established pattern for new primitives (same as ConfigMap and Deployment). No discrepancy exists.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

@sourcehawk
Copy link
Owner Author

approved

Copilot AI review requested due to automatic review settings March 23, 2026 03:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • builder.go:29 — Renamed shadowed variable p to pdb in the identityFunc closure inside NewBuilder to avoid shadowing the outer parameter. This aligns with the pattern used by other primitives (deployment uses d, configmap uses c).

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copilot AI review requested due to automatic review settings March 23, 2026 16:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 17:00
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • resource.go:17 (PreserveStatus): The code already called generic.PreserveStatus(current, original) but the function comment and docs did not mention Status preservation. Updated the comment and docs/primitives/pdb.md to accurately describe that DefaultFieldApplicator preserves the Status subresource.

Intentionally ignored:

  • mutator.go:49 (FeatureMutator unexported beginFeature): Technically valid observation — pdb.Mutator cannot satisfy the FeatureMutator interface because beginFeature() is unexported in internal/generic. However, this is a pre-existing framework-level pattern shared by ALL primitives (deployment, configmap, pdb). Fixing it requires changing shared infrastructure (internal/generic), which is out of scope for this PR. The mutator still works correctly — all edits land in the initial plan with metadata-before-spec ordering preserved.
  • docs/primitives/pdb.md:49 (DeepCopy): The reviewer claimed desired.Spec.DeepCopy() won't compile, but this is incorrect. PodDisruptionBudgetSpec has DeepCopy() on a pointer receiver (*PodDisruptionBudgetSpec), and since desired.Spec is addressable (accessed through a *PodDisruptionBudget pointer), Go auto-addresses it. The expression *desired.Spec.DeepCopy() is valid Go.

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.

sourcehawk and others added 8 commits March 23, 2026 20:16
Implement the PDB primitive as a Static resource following the ConfigMap
pattern. Includes builder, resource, mutator (plan-and-apply with metadata
and spec edit categories), field application flavors, and the typed
PodDisruptionBudgetSpecEditor for mutation surfaces.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates PodDisruptionBudget management with feature-gated mutations
that switch between MinAvailable and MaxUnavailable based on runtime
conditions, version labelling, and field application flavors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add docs/primitives/pdb.md covering building, mutations, editors, flavors,
and guidance. Update docs/primitives.md to list the PDB primitive in the
built-in primitives table and the PodDisruptionBudgetSpecEditor in the
editors table.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rename inner parameter `p` to `pdb` in the identityFunc closure to
avoid shadowing the outer NewBuilder parameter, improving readability.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Matches the fix applied to deployment and configmap mutators - initialize
the plans slice inline instead of calling beginFeature(), which would
create a duplicate empty feature when the generic helper also calls it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…reserveStatus

Update the function comment and documentation to accurately describe that
DefaultFieldApplicator preserves the Status subresource in addition to
server-managed metadata and shared-controller fields.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align with main branch change in #43 that exported BeginFeature() on
the FeatureMutator interface.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk sourcehawk force-pushed the feature/pdb-primitive branch from d08614a to 4ad9bac Compare March 23, 2026 21:33
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • EditObjectMetadata/EditSpec nil-active panic (lines 61, 76): Both methods now return error and check m.active == nil, returning ErrNoActiveFeature. Replaced ObjectMutator interface assertion test with ErrNoActiveFeature coverage. Updated Mutator doc comment to note the intentional interface divergence.
  • newValidPDB missing Selector (line 24): Added Spec.Selector with a basic MatchLabels to make the test helper produce a Kubernetes-valid PDB.
  • Example error propagation: Updated example mutations.go to propagate errors from EditObjectMetadata/EditSpec instead of discarding them.

Intentionally ignored:

  • Version string "v1" vs "1.0.0" (line 71): All other primitive tests (configmap, deployment) consistently use "v1" as the version string. This is the established convention in the codebase.
  • fmt.Printf in example data extractor (line 55): All other example primitives (configmap, deployment) use fmt.Printf in their data extractors. This is the established pattern for example/demo code in the repo.

<!-- claude-review-cycle -->

@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:17
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment on lines +77 to +83
Mutate: func(m *Mutator) error {
m.EditSpec(func(e *editors.PodDisruptionBudgetSpecEditor) error {
e.SetMaxUnavailable(intstr.FromInt32(1))
return nil
})
return nil
},
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test mutation ignores the error returned by m.EditSpec(...). Since EditSpec can return an error (e.g., ErrNoActiveFeature), this can cause the test to pass even if the mutation failed to register. Return the result of m.EditSpec(...) (or check and propagate it) so failures are surfaced.

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +119
m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "a")
return nil
})
return nil
},
}).
WithMutation(Mutation{
Name: "feature-b",
Feature: feature.NewResourceFeature("v1", nil).When(true),
Mutate: func(m *Mutator) error {
m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "b")
return nil
})
return nil
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutation ignores the error returned by m.EditObjectMetadata(...). Propagating that error (instead of always returning nil) makes the test correctly fail if the mutator rejects edits or if the editor returns an error.

Suggested change
m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "a")
return nil
})
return nil
},
}).
WithMutation(Mutation{
Name: "feature-b",
Feature: feature.NewResourceFeature("v1", nil).When(true),
Mutate: func(m *Mutator) error {
m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "b")
return nil
})
return nil
return m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "a")
return nil
})
},
}).
WithMutation(Mutation{
Name: "feature-b",
Feature: feature.NewResourceFeature("v1", nil).When(true),
Mutate: func(m *Mutator) error {
return m.EditObjectMetadata(func(e *editors.ObjectMetaEditor) error {
e.EnsureLabel("order", "b")
return nil
})

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +77
m.EditSpec(func(e *editors.PodDisruptionBudgetSpecEditor) error {
e.ClearMinAvailable()
e.SetMaxUnavailable(intstr.FromInt32(1))
return nil
})
return nil
},
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation examples drop the error returned from m.EditSpec(...)/m.EditObjectMetadata(...) (and then return nil). Since these edit-registration calls return error, the examples should show returning/propagating that error so readers don’t accidentally ignore failures.

Copilot uses AI. Check for mistakes.
…nd docs

Return errors from m.EditSpec() and m.EditObjectMetadata() instead of
silently discarding them, so test failures and editor errors are properly
surfaced.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • pkg/primitives/pdb/resource_test.go:83 — Propagated error from m.EditSpec() in TestResource_Mutate_WithMutation instead of discarding it
  • pkg/primitives/pdb/resource_test.go:119 — Propagated errors from m.EditObjectMetadata() in both feature-a and feature-b mutations in TestResource_Mutate_FeatureOrdering
  • docs/primitives/pdb.md:77 — Updated all documentation examples inside Mutate functions to return errors from m.EditSpec() and m.EditObjectMetadata() instead of discarding them

Intentionally ignored:
None

<!-- claude-review-cycle -->

Satisfy errcheck linter by wrapping EditSpec/EditObjectMetadata calls
with require.NoError, and fix goimports grouping in resource_test.go.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 25, 2026 16:52
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:56
Include clusterrolebinding, clusterrole, and pvc primitives from main
alongside pdb primitive additions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:
None — all three comments had already been fixed in prior commits on this branch.

Intentionally ignored:

  • File resource_test.go line 73 (EditSpec error ignored): Already propagated — line 69 uses return m.EditSpec(...). Fixed in commit bb8c0e2.
  • File resource_test.go line 119 (EditObjectMetadata error ignored): Already propagated — lines 91 and 101 use return m.EditObjectMetadata(...). Fixed in commit bb8c0e2.
  • File pdb.md line 77 (doc examples drop errors): Already fixed — all doc examples use return m.EditSpec(...) / return m.EditObjectMetadata(...). Fixed in commit bb8c0e2.

Additionally resolved merge conflicts from main (clusterrole, clusterrolebinding, pvc primitives).

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comment on lines +117 to +127
| Editor | Scope |
| ------------------------------- | --------------------------------------------------------------------------------- |
| `ContainerEditor` | Environment variables, arguments, resource limits, ports |
| `PodSpecEditor` | Volumes, tolerations, node selectors, service account, security context |
| `DeploymentSpecEditor` | Replicas, update strategy, label selectors |
| `ConfigMapDataEditor` | `.data` entries — set, remove, deep-merge YAML patches, raw access |
| `PodDisruptionBudgetSpecEditor` | MinAvailable, MaxUnavailable, selector, eviction policy |
| `PolicyRulesEditor` | `.rules` entries on Role and ClusterRole objects — add, remove, clear, raw access |
| `BindingSubjectsEditor` | Subjects on RoleBinding or ClusterRoleBinding — ensure, remove, raw |
| `PVCSpecEditor` | Access modes, storage class, volume mode, storage requests |
| `ObjectMetaEditor` | Labels and annotations on any Kubernetes object |
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as in docs/primitives/pdb.md: the editor list table uses || which introduces an extra empty column / inconsistent rendering. Convert to standard Markdown table rows beginning with a single |.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +34
// Unlike other primitive mutators, EditObjectMetadata and EditSpec return an
// error when called without an active feature scope, rather than silently
// succeeding. This means Mutator does not satisfy editors.ObjectMutator.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is slightly inconsistent with the implementation: EditObjectMetadata(nil) and EditSpec(nil) return nil even when there is no active feature scope. Consider clarifying the comment to reflect that the error is returned only when a non-nil edit function is provided without BeginFeature().

Copilot uses AI. Check for mistakes.
The comment on the Mutator type incorrectly implied that EditObjectMetadata
and EditSpec always error without an active feature scope. In reality, nil
edit functions are silently ignored regardless of scope state. Updated the
comment to reflect this nuance.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • pkg/primitives/pdb/mutator.go line 32-34: Clarified the Mutator type comment to reflect that nil edit functions are silently ignored regardless of feature scope, and that the error is only returned when a non-nil edit function is provided without BeginFeature().

Intentionally ignored:

  • docs/primitives.md line 127: The comment claims || (double-pipe) syntax is used in the editor list table, but inspection shows the table already uses correct single-pipe Markdown syntax. No || patterns exist anywhere in the docs directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants