feat(component): functional-option resource registration#126
Merged
Conversation
…When (#125) Replace ResourceOptions struct param and ResourceOptionsBuilder with functional options on WithResource, and add IncludeWhen for lazy omit-on-false registration. Options resolution errors now route into the component builder's aggregated buildErrors instead of a separate return. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…invariant; clarify GatedBy doc (#125) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…variant directly (#125) A prior change added a resourceErrors map with retract-on-success logic to satisfy a test. Invalid WithResource options are a programming error and must fail Build loudly; revert to appending the error directly and test the real invariant (errored resource absent from the lookup). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on (#125) Replace all ResourceOptions struct literals, ResourceOptionsBuilder, and ResourceOptionsFor references with the new functional-option API (ReadOnly, Delete, DeleteWhen, GatedBy, Auxiliary, BlockOnAbsence, IgnoreIfAbsent, SuppressGraceInconsistencyWarning). Add documentation for IncludeWhen. Run make fmt-md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR modernizes pkg/component resource registration by replacing the exported ResourceOptions struct and ResourceOptionsBuilder with functional options on Builder.WithResource, and adds Builder.IncludeWhen to lazily omit optional resources without forcing imperative if blocks.
Changes:
- Replaced
WithResource(resource, ResourceOptions)+ResourceOptionsBuilderwithWithResource(resource, opts ...ResourceOption)and option helpers (e.g.,ReadOnly(),DeleteWhen(...),GatedBy(...),Auxiliary()). - Added
IncludeWhen(include, build, opts...)to defer resource construction and omit registration entirely whenincludeis false. - Migrated docs, examples, and (mechanically) e2e call sites to the new API; added/updated unit tests for option resolution and builder behavior.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates quick-start snippets to use WithResource(...) without ResourceOptions{} and demonstrates functional options. |
| pkg/component/resource_options.go | Introduces the functional-option API (ResourceOption) and internal resolved resourceOptions. |
| pkg/component/resource_options_test.go | Adds table tests for option resolution, validation, and feature-gate evaluation behavior. |
| pkg/component/resource_options_builder.go | Removes the legacy ResourceOptionsBuilder implementation. |
| pkg/component/resource_options_builder_test.go | Removes tests for the legacy options builder. |
| pkg/component/create_test.go | Updates internal tests to use resourceOptions (internal) literals in reconcile entries. |
| pkg/component/component.go | Switches reconcileEntry.Options from exported ResourceOptions to internal resourceOptions. |
| pkg/component/component_test.go | Migrates tests to new WithResource option style and internal resourceOptions. |
| pkg/component/builder.go | Implements WithResource(resource, ...ResourceOption) and adds IncludeWhen(...). |
| pkg/component/builder_test.go | Updates builder tests and adds coverage for IncludeWhen and option-resolution error aggregation. |
| examples/mutations-and-gating/README.md | Updates example docs to use component.DeleteWhen(...) instead of the old builder. |
| examples/mutations-and-gating/app/controller.go | Migrates controller code to new WithResource(..., component.DeleteWhen(...)) style. |
| examples/grace-inconsistency/README.md | Updates docs to reference component.SuppressGraceInconsistencyWarning() option. |
| examples/grace-inconsistency/app/controller.go | Migrates controller code to pass component.SuppressGraceInconsistencyWarning() to WithResource. |
| examples/extraction-and-guards/app/controller.go | Removes component.ResourceOptions{} usage in registrations. |
| examples/custom-resource/app/controller.go | Removes component.ResourceOptions{} usage in registrations. |
| examples/component-prerequisites/app/controller.go | Removes component.ResourceOptions{} usage in registrations. |
| e2e/primitives/unstructured_workload_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/unstructured_task_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/unstructured_integration_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/statefulset_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/service_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/replicaset_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/pvc_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/pv_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/pod_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/ingress_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/hpa_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res) (plus import reordering). |
| e2e/primitives/deployment_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/daemonset_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/primitives/cronjob_test.go | Mechanical migration from ResourceOptions{} to default WithResource(res). |
| e2e/framework/reconciler.go | Updates framework reconciler to use WithResource(resource) defaults. |
| e2e/framework/cluster_reconciler.go | Updates cluster reconciler to use WithResource(resource) defaults. |
| e2e/component/prerequisite_gate_test.go | Mechanical migration to new WithResource(...) signature. |
| e2e/component/multi_resource_test.go | Mechanical migration + updates auxiliary registration to component.Auxiliary(). |
| docs/primitives/secret.md | Updates documentation snippets to the new WithResource(...) style. |
| docs/primitives/configmap.md | Updates documentation snippets to the new WithResource(...) style. |
| docs/guidelines.md | Updates guidelines snippets to functional options (Auxiliary, GatedBy, Delete, etc.). |
| docs/custom-resource.md | Updates documentation text/snippets to reference new option helpers. |
| docs/component.md | Rewrites the resource-options section to document functional options and IncludeWhen. |
…ce (#125) Address PR review feedback: - WithResource rejects a nil interface or typed-nil pointer with a build error instead of panicking, at the single registration choke point so IncludeWhen is covered by delegation. - Cache resource.Identity() once per WithResource call. - Correct the resourceConfig doc comment (ResourceOption, not the removed ResourceOptions struct). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A read-only resource is not owned by the component and must never be deleted. Previously, combining ReadOnly with a disabled GatedBy gate or a true DeleteWhen condition forced ReadOnly off and deleted the resource (behavior inherited from the removed ResourceOptionsBuilder), which could destroy a resource owned by the user or another operator. ReadOnly is now mutually exclusive with Delete, DeleteWhen, and GatedBy, validated by option presence and returned as a build error. Conditional read-only presence is expressed with IncludeWhen, which omits the resource without ever deleting it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the two axes explicit: GatedBy/DeleteWhen conditionally render an owned resource and delete it when the condition turns off; IncludeWhen conditionally includes a resource and never deletes. Document IncludeWhen's primary purpose (optional externally-owned read-only references) and its secondary use (untracking a resource without removing it from the cluster). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#125) When include is true and the build closure itself is nil, IncludeWhen panicked on the nil func call before reaching WithResource. Record it as a build error surfaced at Build() instead, matching how WithResource handles a nil resource. A nil build with include=false remains a no-op. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#125) A nil ResourceOption (e.g. a conditionally-assigned option left unset) now resolves to a no-op rather than panicking on the nil func call. Unlike a nil resource or build function, a nil option has a sensible meaning (no modifier), so skipping it supports the conditional-option idiom. Documented on the ResourceOption type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #125
Replaces the
component.ResourceOptionsstruct parameter and the separateResourceOptionsBuilderwith functional options on the component builder, and adds a lazyIncludeWhenfor omit-on-false resource registration. The oldResourceOptionsBuilder.Build()returned an error, which forced a separateopts, err := ...; if err != nilstatement that broke the otherwise-fluent builder chain. Resolution now routes errors into the builder's existing aggregation and surfaces them atBuild().IncludeWhendefers resource construction behind a closure, so optional externally-owned resources (for example a read-only*SecretKeyRefthat may be nil) can be registered inline without a nil dereference.Changes
WithResource(resource, opts ...component.ResourceOption)with optionsReadOnly(),Delete(),DeleteWhen(cond),GatedBy(gate),Auxiliary(),BlockOnAbsence(),IgnoreIfAbsent(),SuppressGraceInconsistencyWarning().IncludeWhen(include bool, build func() component.Resource, opts ...)for omit-on-false registration.buildis not called whenincludeis false, and the resource is never created, read, or deleted, nor counted in the duplicate-Identity check.buildErrorsand surface atBuild()instead of a separate return value.ResourceOptionsBuilder,NewResourceOptionsBuilder,ResourceOptionsFor, and the exportedResourceOptionsstruct. The resolved value type is now internal.Challenges
The omit-on-false case cannot be a functional option. Go evaluates the resource argument before
WithResourceruns, so a constructor that dereferences an absent optional would panic before any option could gate it. Deferring construction requires a closure, soIncludeWhentakesfunc() Resourceand lives on the presence axis, separate from the lifecycle options (DeleteWhenand friends) that act on an already-built resource.Testing
make allpasses (build, golangci-lint, gofmt, and the full unit suite including golden tests).make build-examplesandgo vet ./e2e/...are green. New table tests cover option resolution (gate enabled, disabled, eval-error, and nil; additive-ORDeleteWhen; deletion forcingReadOnlyoff;Auxiliary;BlockOnAbsence/IgnoreIfAbsentvalidation and mutual exclusion) and builder behavior (IncludeWhenwith include true and false, including proofbuildis not called on false; omitted resource excluded from the duplicate check; resolution-error routing and non-registration). The e2e suite was not run against a live kind cluster since the e2e change is a mechanical call-site migration; reviewers with a cluster can spot-check viamake e2e-primitives PRIMITIVE=deployment.