HYPERFLEET-1295 - feat: Conditional creation of resources#236
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant ExecuteAll as ResourceExecutor.ExecuteAll
participant Discovery as pre-discovery
participant Execute as executeResource
participant CEL as evaluateLifecycleWhen
participant Apply as ApplyResource
ExecuteAll->>Discovery: pre-discover resources with create/delete lifecycle
Discovery-->>ExecuteAll: execCtx.Resources populated
ExecuteAll->>Execute: process resource
Execute->>Execute: check execCtx.Resources for existing object
alt absent and lifecycle.create.when set
Execute->>CEL: evaluate create expression
CEL-->>Execute: true/false or error
alt false
Execute->>Execute: mark skipped and set adapter skip metadata
else true
Execute->>Apply: apply resource
end
else present or no create.when
Execute->>Apply: apply resource
end
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 654 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/configloader/validator.go (1)
596-659: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidation logic is sound; consider extracting create/delete validation into helpers.
validateLifecycleConfigis now ~72 lines with two parallel validation blocks. ExtractingvalidateCreateLifecycleandvalidateDeleteLifecyclewould improve readability and keep the function under the 50-line guideline. Not blocking — the current structure is clear and well-commented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/configloader/validator.go` around lines 596 - 659, `validateLifecycleConfig` has grown too large with two parallel create/delete validation blocks, so extract them into focused helpers to improve readability and keep the function under the size guideline. Move the existing create logic into a helper like `validateCreateLifecycle`, and the delete logic into `validateDeleteLifecycle`, passing the resource, index, and any shared state needed. Keep the same validation behavior and error paths/messages by reusing the existing `validateCELExpression`, `FieldLifecycleCreate`, and `FieldLifecycleDelete`-based logic inside the new helpers.Source: Path instructions
internal/executor/resource_executor_test.go (1)
650-674: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
lifecycle.createwith nilwhen(always-create default).
newResourceWithLifecycleCreatealways setsWhento a non-nilLifecycleWhen. The executor handles nilWhenby defaultingshouldCreate=true(line 133-138), and the validator permitslifecycle.createwith nowhenblock. This is a valid config shape with no test coverage.♻️ Suggested test addition
func TestResourceExecutor_LifecycleCreate_NilWhen_DefaultsTrue_Applied(t *testing.T) { // lifecycle.create configured with no `when` block → always create (shouldCreate defaults true). notFoundErr := apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "test-cm") discovered := &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]interface{}{"name": "test-cm", "namespace": "default"}, }} mock := &sequencedGetResourceMock{ MockK8sClient: k8sclient.NewMockK8sClient(), results: []*unstructured.Unstructured{nil, discovered}, errs: []error{notFoundErr, nil}, } mock.ApplyResourceResult = &transportclient.ApplyResult{ Operation: manifest.OperationCreate, Reason: "mock create", } re := newResourceExecutor(&ExecutorConfig{ TransportClient: mock, Logger: logger.NewTestLogger(), }) resource := configloader.Resource{ Name: "test-resource", Transport: &configloader.TransportConfig{Client: "kubernetes"}, Manifest: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]interface{}{"name": "test-cm", "namespace": "default"}, }, Discovery: &configloader.DiscoveryConfig{Namespace: "default", ByName: "test-cm"}, Lifecycle: &configloader.ResourceLifecycle{ Create: &configloader.LifecycleCreate{}, // When is nil }, } execCtx := NewExecutionContext(context.Background(), nil, nil) results, err := re.ExecuteAll(context.Background(), []configloader.Resource{resource}, execCtx) require.NoError(t, err) require.Len(t, results, 1) assert.Equal(t, StatusSuccess, results[0].Status) assert.Equal(t, manifest.OperationCreate, results[0].Operation) assert.False(t, execCtx.Adapter.ResourcesSkipped) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/executor/resource_executor_test.go` around lines 650 - 674, Add coverage for the `lifecycle.create` default path where `LifecycleCreate.When` is nil. Introduce a test alongside the existing resource executor tests that builds a resource with `newResourceWithLifecycleCreate`-style setup but omits the `When` block entirely, then verify the executor still creates the resource by taking the `shouldCreate=true` default in the `ResourceExecutor` create flow. Keep the test aligned with the existing `configloader.Resource`, `ResourceLifecycle`, and `LifecycleCreate` structures so it exercises the validator-accepted nil-when shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/configloader/validator.go`:
- Around line 596-659: `validateLifecycleConfig` has grown too large with two
parallel create/delete validation blocks, so extract them into focused helpers
to improve readability and keep the function under the size guideline. Move the
existing create logic into a helper like `validateCreateLifecycle`, and the
delete logic into `validateDeleteLifecycle`, passing the resource, index, and
any shared state needed. Keep the same validation behavior and error
paths/messages by reusing the existing `validateCELExpression`,
`FieldLifecycleCreate`, and `FieldLifecycleDelete`-based logic inside the new
helpers.
In `@internal/executor/resource_executor_test.go`:
- Around line 650-674: Add coverage for the `lifecycle.create` default path
where `LifecycleCreate.When` is nil. Introduce a test alongside the existing
resource executor tests that builds a resource with
`newResourceWithLifecycleCreate`-style setup but omits the `When` block
entirely, then verify the executor still creates the resource by taking the
`shouldCreate=true` default in the `ResourceExecutor` create flow. Keep the test
aligned with the existing `configloader.Resource`, `ResourceLifecycle`, and
`LifecycleCreate` structures so it exercises the validator-accepted nil-when
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5ab21189-0ef4-4d70-943a-6625dde4e673
📒 Files selected for processing (8)
configs/adapter-task-config-template.yamldocs/adapter-authoring-guide.mdinternal/configloader/constants.gointernal/configloader/types.gointernal/configloader/validator.gointernal/configloader/validator_test.gointernal/executor/resource_executor.gointernal/executor/resource_executor_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
|
|
||
| // discovery is required — without it executeResource cannot determine whether | ||
| // the resource already exists, and will always evaluate the when condition. | ||
| switch { |
There was a problem hiding this comment.
Blocking (Architecture) — Confidence: High
The architecture design doc (PR #182, Migration & Compatibility section) says:
Set without
when→ config validation rejects it (mirrorslifecycle.delete's requirement).
But the implementation accepts lifecycle.create without when (test at validator_test.go:1187 explicitly validates this). The JIRA says "optional when field," so the implementation seems correct — the design doc should be updated to match.
Please align the two PRs: either update the design doc to say when is optional, or add a validation case here to reject lifecycle.create without when.
| Logger: logger.NewTestLogger(), | ||
| }) | ||
|
|
||
| // Invalid CEL syntax — evaluateLifecycleCreateWhen will error. |
There was a problem hiding this comment.
nit (Pattern) — Confidence: High
This comment references evaluateLifecycleCreateWhen — a function that doesn't exist. The PR unified the old evaluateLifecycleDeleteWhen into a shared evaluateLifecycleWhen (taking a kind parameter).
| // Invalid CEL syntax — evaluateLifecycleCreateWhen will error. | |
| // Invalid CEL syntax — evaluateLifecycleWhen will error. |
| if whenErr != nil { | ||
| result.Status = StatusFailed | ||
| result.Error = whenErr | ||
| re.recordResourceError(execCtx, resource, whenErr) | ||
| return result, NewExecutorError(PhaseResources, resource.Name, "failed to evaluate lifecycle.create.when", whenErr) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit (Inconsistency) — Confidence: Medium
The create error path doesn't set result.Operation, but the analogous delete error path (line 177) sets result.Operation = manifest.OperationDelete. Since we're in the !resourceFound branch, the intended operation is creation.
| if whenErr != nil { | |
| result.Status = StatusFailed | |
| result.Error = whenErr | |
| re.recordResourceError(execCtx, resource, whenErr) | |
| return result, NewExecutorError(PhaseResources, resource.Name, "failed to evaluate lifecycle.create.when", whenErr) | |
| } | |
| if whenErr != nil { | |
| result.Status = StatusFailed | |
| result.Operation = manifest.OperationCreate | |
| result.Error = whenErr | |
| re.recordResourceError(execCtx, resource, whenErr) | |
| return result, NewExecutorError(PhaseResources, resource.Name, "failed to evaluate lifecycle.create.when", whenErr) | |
| } |
2 participants
Summary
Adds a conditional creation gate for resources, symmetric to the existing conditional deletion gate. A resource can now declare
lifecycle.create.whenwith a CEL expression that must hold before it is created for the first time; once the resource exists, the gate is ignored and it's reconciled normally like any other resource. This lets adapters express "only create this if X" (a feature flag, a sibling resource's discovered state, an event field) without making the whole resources phase all-or-nothing, the same waylifecycle.delete.whenalready does for deletion.HYPERFLEET-1295
Changes
lifecycle.create/LifecycleCreateconfig type, mirroring the shape oflifecycle.deletecreate.whencheck inexecuteResource: evaluated only when the resource is absent from pre-discovery; skips creation (operationskip) when it evaluates tofalse, is ignored entirely once the resource already existsadapter.resourcesSkipped/adapter.skipReason, the same signal precondition-skips already populate, so post-actionwhengates don't need to special-case which phase produced the skipcreateanddeletelifecycle validation into a singlevalidateLifecycleConfigpass, and unified their CEL evaluation into oneevaluateLifecycleWhenhelper (previously delete-only)preDiscoverAll) now also runs when onlylifecycle.createis configured, not justlifecycle.delete, sincecreate.whenneeds to know whether the resource already existslifecycle.createblock in the adapter authoring guide and the task config templateTest Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)