feat(PL-6288): surface reconcile status on joy resources - #34
Conversation
|
Warning Review limit reached
Next review available in: 59 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughChangesThe operator now generates status-enabled CRDs for Releases, Environments, and Catalogs, records reconciliation outcomes as Ready conditions, and updates reconciliation tests to tolerate resource-version conflicts. CRD status reporting
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
aee7872 to
a0fca21
Compare
silphid
changed the title
b9418ba to
03020e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/operator/main_test.go (1)
600-610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the
Readystatus condition somewhere in this test.
TestHappyReconciliationsexercises the full reconcile flow for Catalog/Environment/Release but never asserts.Status.Conditionsor.Status.ObservedGeneration, which is the core new behavior this PR introduces. Worth adding a check before the cleanup/delete calls to catch regressions inwriteStatus.🤖 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 `@cmd/operator/main_test.go` around lines 600 - 610, Before the cleanup deletes in TestHappyReconciliations, fetch the reconciled Catalog, Environment, and Release resources and assert their Ready status condition and ObservedGeneration values. Use the existing Kubernetes interfaces and the applied resource generations to verify writeStatus updated each object, while preserving the current non-destructive cleanup assertions.
🤖 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 `@cmd/operator/main_test.go`:
- Around line 600-610: Before the cleanup deletes in TestHappyReconciliations,
fetch the reconciled Catalog, Environment, and Release resources and assert
their Ready status condition and ObservedGeneration values. Use the existing
Kubernetes interfaces and the applied resource generations to verify writeStatus
updated each object, while preserving the current non-destructive cleanup
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08e2192c-7ed4-457c-8bd3-b3c8120b98c8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
chart/templates/crds.yamlchart/tests/common-labels/expected.yamlchart/tests/helm/expected.yamlchart/tests/standard/expected.yamlcmd/crd-gen/main.gocmd/operator/main_test.gocmd/operator/reconciler_catalog.gocmd/operator/reconciler_environment.gocmd/operator/reconciler_release.gocmd/operator/status.gogo.mod
03020e4 to
91135ab
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/operator/status.go (1)
24-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPanic during a reconcile would report a false "success" status.
Since
retErris only set viareturn ..., errstatements, a panic in any of the three reconcilers after thedefer writeStatus(...)registration leavesretErr == nil, so the deferred call would recordConditionTrue/"Successfully reconciled" despite the crash — potentially masking real failures from anyone consuming the status/health condition. This only matters if nothing upstream (the yokectrl.Funcsruntime) already recovers panics and turns them into errors before this point; please confirm. If not already handled upstream, consider recovering within the deferred closure and forcing a failure condition on panic.🤖 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 `@cmd/operator/status.go` around lines 24 - 64, Verify whether the yoke ctrl.Funcs runtime converts reconciler panics into errors before deferred status handling; if not, update each reconciler’s deferred writeStatus closure to recover panics and pass a failure error instead of the nil retErr, preserving ConditionFalse/ReconcileError reporting for crashes.
🤖 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 `@cmd/operator/status.go`:
- Around line 24-64: Verify whether the yoke ctrl.Funcs runtime converts
reconciler panics into errors before deferred status handling; if not, update
each reconciler’s deferred writeStatus closure to recover panics and pass a
failure error instead of the nil retErr, preserving
ConditionFalse/ReconcileError reporting for crashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00cc42b0-e20d-4e8b-a144-00d2c4479d24
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
chart/templates/crds.yamlchart/tests/common-labels/expected.yamlchart/tests/helm/expected.yamlchart/tests/standard/expected.yamlcmd/crd-gen/main.gocmd/operator/main_test.gocmd/operator/reconciler_catalog.gocmd/operator/reconciler_environment.gocmd/operator/reconciler_release.gocmd/operator/status.gogo.mod
d4caa3f to
d1e4960
Compare
| changed = true | ||
| } | ||
|
|
||
| // As the controller re-enqueues on every update, let's avoid an infinite reconcile loop. |
There was a problem hiding this comment.
I actually don't think this is true. Yes for kubebuilder by default and controller-runtime. false for yoke/k8s/ctrl
| changed := apimeta.SetStatusCondition(&status.Conditions, cond) | ||
| if status.ObservedGeneration != obj.GetGeneration() { | ||
| status.ObservedGeneration = obj.GetGeneration() | ||
| changed = true |
There was a problem hiding this comment.
If observed generation has changed, I think we should exit early no?
| cond.Reason = "ReconcileSuccess" | ||
| cond.Message = "Successfully reconciled" | ||
| } | ||
|
|
There was a problem hiding this comment.
In my experience, before updating a status, it's important to refetch the resource from the API since, you need to check for the most recent generation, and then all of this need to be wrapper in a retry.
There was a problem hiding this comment.
Yes, exactly what David said above. ☝️
That is one of the tricky problems I encountered on infra-operator when adding SDM support.
| cond.Reason = "ReconcileSuccess" | ||
| cond.Message = "Successfully reconciled" | ||
| } | ||
|
|
There was a problem hiding this comment.
Yes, exactly what David said above. ☝️
That is one of the tricky problems I encountered on infra-operator when adding SDM support.
3 participants
Related PRs — PL-6288
One of 3 coordinated PRs:
joy(API/type layer) — feat(PL-6288): add reconcile status to joy resources joy#294 ✅ merged, released as v0.99.0joy-operator(status subresource + reconciler wiring) — feat(PL-6288): surface reconcile status on joy resources #34infra(joy.nesto.ca/*Argo CD health check) — nestoca/infra#4872Merge order:
joyis merged and released; this PR now depends on the taggedjoy v0.99.0.infracan merge any time (no-ops until resources carry conditions).What
Reconcile outcomes now live on each resource's
statusinstead of only in the operator logs.cmd/crd-gen: enable the/statussubresource for Release, Environment, Catalog (Project keeps its no-op reconciler and gets none). Also emit the{{- if .Values.installCRDs -}}guard from the generator sotask crd-genoutput is reproducible — previously the guard had to be re-added by hand after every regen.cmd/operator/status.go: a single genericwriteStatus[T, PT]helper, driven by the sharedv1alpha1.StatusObject[T]constraint, sets aReadycondition (ReconcileSuccess/ReconcileError) +observedGenerationviaUpdateStatus. The write is idempotent — the controller re-enqueues on every update (including our own status writes), so it only callsUpdateStatuswhen the condition orobservedGenerationactually changed, avoiding an infinite reconcile loop. A failed status write is logged, never fatal.deferswriteStatusright after fetching the resource, capturing the outcome of every return path with no per-path edits.testchartgolden files.main_test: refetch resources before mutating them (withretry.RetryOnConflict), now that the operator writes a status subresource on them.Health mapping (see
infraPR)Ready=True→ Healthy ·Ready=False→ Degraded · no conditions yet → Progressing.Dependency
Requires
github.com/nestoca/joy v0.99.0, which carries theResourceStatus/StatusObjectAPI.Testing
go build ./...andgo vet ./cmd/...clean.testchart run— all 4 chart cases pass (status subresource + schema added to the 3 CRDs;no-crdsunchanged).test-app) passes in CI.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
observedGeneration,conditions, and enabled Kubernetes status subresources.Bug Fixes
Tests
statusschema and subresource behavior.