Skip to content

feat: implement daemonset primitive#23

Open
sourcehawk wants to merge 19 commits intomainfrom
feature/daemonset-primitive
Open

feat: implement daemonset primitive#23
sourcehawk wants to merge 19 commits intomainfrom
feature/daemonset-primitive

Conversation

@sourcehawk
Copy link
Owner

@sourcehawk sourcehawk commented Mar 22, 2026

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

Summary

  • Adds daemonset primitive package under pkg/primitives/daemonset/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, flavors, and builder
  • Adds DaemonSetSpecEditor to shared pkg/mutation/editors/ package
  • Updates docs/primitives.md with daemonset primitive documentation

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md
  • Shared files intentionally modified: docs/primitives.md, pkg/mutation/editors/daemonsetspec.go

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 Kubernetes DaemonSet primitive to the operator-component-framework, following the established patterns of existing workload primitives (e.g., deployment) and providing an end-to-end example plus documentation.

Changes:

  • Introduces pkg/primitives/daemonset/ with builder, resource wrapper, status handlers, mutator (with selectors/editors), flavors, and tests.
  • Adds a new shared editor pkg/mutation/editors/DaemonSetSpecEditor with unit tests.
  • Adds an examples/daemonset-primitive/ runnable example and docs/primitives/daemonset.md documentation.

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/daemonset/resource.go Workload resource wrapper delegating lifecycle hooks to generic workload resource.
pkg/primitives/daemonset/builder.go Fluent builder wiring defaults (applicator, handlers) and mutation/flavor/extractor registration.
pkg/primitives/daemonset/handlers.go Default converge/grace/suspend handlers for DaemonSet lifecycle/health.
pkg/primitives/daemonset/flavors.go Field-application “flavors” to preserve selected live fields (labels/annotations/template metadata).
pkg/primitives/daemonset/mutator.go Planned mutation engine for DaemonSet (metadata/spec/podspec/containers + presence ops + snapshots).
pkg/primitives/daemonset/builder_test.go Builder API/validation/handler wiring tests.
pkg/primitives/daemonset/handlers_test.go Unit tests for default status/suspension handlers.
pkg/primitives/daemonset/flavors_test.go Unit tests for flavors and flavor ordering/error propagation.
pkg/primitives/daemonset/mutator_test.go Extensive mutator behavior tests (ordering, presence ops, snapshots, nil-safety).
pkg/mutation/editors/daemonsetspec.go Adds typed editor for appsv1.DaemonSetSpec.
pkg/mutation/editors/daemonsetspec_test.go Unit tests for the new DaemonSetSpec editor.
examples/daemonset-primitive/app/owner.go Re-exports shared example CRD types for the new example.
examples/daemonset-primitive/app/controller.go Example controller wiring a component with the DaemonSet primitive.
examples/daemonset-primitive/features/mutations.go Example feature-gated mutations using the DaemonSet mutator APIs.
examples/daemonset-primitive/features/flavors.go Example flavors wiring for preserving labels/annotations.
examples/daemonset-primitive/resources/daemonset.go Example DaemonSet resource factory composing builder + features + flavors + extraction.
examples/daemonset-primitive/main.go Runnable fake-client reconciliation sequence demonstrating the primitive.
examples/daemonset-primitive/README.md Example documentation and run instructions.
docs/primitives/daemonset.md Public documentation for the new primitive (API, ordering, editors, suspension, status).

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 19 out of 19 changed files in this pull request and generated 8 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • resource.go:57 — Fixed "custom customFieldApplicator" typo (duplicate word)
  • resource.go:73 — Updated ConvergingStatus comment to accurately describe DefaultConvergingStatusHandler semantics (>= check, zero-desired + ObservedGeneration handling)
  • resource.go:85 — Fixed GraceStatusUp → GraceStatusHealthy to match actual constant name
  • builder.go:110 — Updated WithCustomConvergeStatus doc comment to match actual handler behavior
  • handlers.go:51 — Emit dedicated reason "Waiting for controller to observe latest generation" when ObservedGeneration is stale, instead of misleading "Waiting for pods: 0/0 ready"

Intentionally ignored:

  • mutator.go:75 (FeatureMutator unexported method) — This is a framework-level architectural concern that equally affects all external primitives (including deployment). The internal/generic.FeatureMutator interface uses an unexported method by design. Fixing this would require modifying shared infrastructure (internal/generic), which is out of scope for this PR.
  • docs/primitives/daemonset.md:98 (ordering guarantees) — Same root cause as above. The documented ordering is accurate for the within-mutation category ordering. Cross-mutation feature boundaries depend on the framework-level FeatureMutator issue.
  • pkg/mutation/editors/daemonsetspec.go:10 (PR description says no shared files modified) — This is a PR description accuracy concern, not a code issue. The DaemonSetSpecEditor is a new file added by this PR as part of the daemonset feature, not a modification to an existing shared file.

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

Copilot AI review requested due to automatic review settings March 22, 2026 19:57
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 20 out of 20 changed files in this pull request and generated 5 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • handlers.go:29 — Gated the healthy path in DefaultConvergingStatusHandler on ObservedGeneration >= Generation to prevent reporting convergence for stale status after an update. Added test case for stale ObservedGeneration with desired > 0, and updated existing healthy-path tests to set Generation/ObservedGeneration.
  • handlers.go:69 — Fixed comment referencing nonexistent GraceStatusUp; corrected to GraceStatusHealthy to match the actual constant.
  • resource.go:80 — The doc comment now accurately describes the handler behavior after the ObservedGeneration fix above (the handler does gate on ObservedGeneration for both code paths). No change needed.

Intentionally ignored:

  • docs/primitives/daemonset.md:14 and :96 — The tables already use standard |-delimited markdown syntax, not ||. The rendered output is correct. These appear to be false positives from the review.

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 20 out of 20 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • Added comprehensive Resource-level tests (resource_test.go) matching deployment primitive coverage: Identity, Object deep-copy behavior, Mutate with mutations and feature ordering, custom/default handler wiring for ConvergingStatus, GraceStatus, DeleteOnSuspend, Suspend, SuspensionStatus, ExtractData, and CustomFieldApplicator.

Intentionally ignored:

  • handlers.go:127 (create→delete churn on suspend): Valid concern, but the fix requires modifying pkg/component/suspend.go (shared infrastructure) to short-circuit createOrUpdateResources when DeleteOnSuspend is true and the object doesn't exist. The DaemonSet handlers themselves cannot prevent this — they don't control whether createOrUpdateResources is called. Note that suspend.go:124-127 already handles IsNotFound on the delete path, so the churn is non-fatal but wasteful. This should be addressed as a separate framework-level change.
  • daemonsetspec.go:15 (shared editor location): The DaemonSetSpecEditor follows the same pattern as DeploymentSpecEditor in pkg/mutation/editors/ — this is the correct and idiomatic location. The PR description/checklist should be updated to reflect this shared addition, but the code placement itself is correct.

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

Copilot AI review requested due to automatic review settings March 23, 2026 00:12
@sourcehawk sourcehawk force-pushed the feature/daemonset-primitive branch from c0099c7 to 2638488 Compare March 23, 2026 00:12
@sourcehawk
Copy link
Owner Author

approved

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 20 out of 20 changed files in this pull request and generated 5 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • docs/primitives/daemonset.md line 9: Removed Failing from capabilities table since DefaultConvergingStatusHandler never returns it
  • pkg/primitives/daemonset/handlers.go line 77: Gated DefaultGraceStatusHandler zero-desired healthy path on ObservedGeneration >= Generation, returning Degraded when the controller hasn't observed the latest generation
  • pkg/primitives/daemonset/handlers_test.go line 151: Updated test to set Generation/ObservedGeneration explicitly, added new test case for stale observed generation returning Degraded
  • docs/primitives/daemonset.md GraceStatus table: Updated to reflect the new ObservedGeneration check and Degraded status for stale generation

Intentionally ignored:

  • docs/primitives/daemonset.md line 60 (mutation ordering): The framework does enforce per-mutation boundaries via beginFeature() in mutate_helper.go, so mutations are applied sequentially in registration order. The existing documentation is accurate.
  • pkg/primitives/daemonset/builder.go line 63 (sequential comment): Same as above — the comment correctly describes the behavior. Each mutation gets its own feature boundary before ApplyIntent() is called.

<!-- 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 20 out of 20 changed files in this pull request and generated 2 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:

  • Fixed findEnv in mutator_test.go returning pointer to range variable copy — now iterates by index and returns &env[i] instead of &e

Intentionally ignored:

  • Double leading pipes (||) in docs/primitives/daemonset.md tables — the tables already use single pipes correctly; no || pattern exists in the file

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 20 out of 20 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • pkg/primitives/daemonset/mutator_test.go: Replaced all t.Fatalf/t.Errorf usage with require.NoError/require.Len/assert.Equal to match the repo's testify convention
  • examples/daemonset-primitive/resources/daemonset.go: Fixed misleading data extractor that printed zero-value status fields (NumberReady/DesiredNumberScheduled) — now prints spec/metadata (Namespace/Name) since ExtractData runs on the desired object

Intentionally ignored:

  • docs/primitives/daemonset.md table syntax comment: The tables already use standard single-pipe (|) markdown syntax throughout. No double-pipe (||) separators exist in the file.

Copilot AI review requested due to automatic review settings March 23, 2026 16:08
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02:19
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02:20
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02: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 21 out of 21 changed files in this pull request and generated 3 comments.

Align the daemonset DefaultFieldApplicator doc comment with the deployment
primitive to explicitly mention that the Status subresource is preserved,
matching the actual implementation behavior.

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

Claude Review Cycle 3 Complete

Addressed:

  • Updated DefaultFieldApplicator doc comment in pkg/primitives/daemonset/resource.go to mention Status subresource preservation, aligning with the deployment primitive (comment 1). The implementation (PreserveStatus call) and test (TestDefaultFieldApplicator_PreservesStatus) were already in place from commit 239e4b3.

Intentionally ignored:

  • Comment 2 (add Status preservation test): Already exists — TestDefaultFieldApplicator_PreservesStatus at resource_test.go:413 covers this with assertions on all five DaemonSetStatus fields.
  • Comment 3 (PR description checklist about shared files): This is a PR description accuracy concern, not a code issue. The shared-file changes (docs/primitives.md table entry, pkg/mutation/editors/daemonsetspec.go) are intentional and required for the daemonset primitive — they are part of the feature, not modifications to unrelated shared infrastructure.

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

@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 24, 2026 02:25
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 21 out of 21 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 4 Complete

Addressed:
None — all comments were already resolved in prior cycles.

Intentionally ignored:

  • resource.go:18 (DefaultFieldApplicator docstring): Already fixed in commit 0cc4ea2. The doc comment at lines 10-13 already mentions Status subresource preservation. This appears to be a stale review comment that was not dismissed after the fix was applied.

<!-- 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 21 out of 21 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 5 Complete

Addressed:

  • Updated PR description checklist to accurately reflect that shared files (docs/primitives.md, pkg/mutation/editors/daemonsetspec.go) are intentionally modified as part of adding the DaemonSet primitive (Copilot comment on docs/primitives.md:160)

Intentionally ignored:
None

<!-- 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 21 out of 21 changed files in this pull request and generated no new comments.

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