refactor(status): one writer for the kstatus trio, one status write per reconcile#266
Merged
Conversation
|
Warning Review limit reached
Next review available in: 12 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 selected for processing (73)
📝 WalkthroughWalkthroughThis PR standardizes Kubernetes status handling through staged commits and readiness aggregation, changes generic success reasons to ChangesStatus contract and reconciliation pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant Readiness
participant StatusPatch
participant KubernetesEvents
Controller->>Readiness: contribute gate outcomes
Readiness-->>Controller: derive Ready, Reconciling, Stalled trio
Controller->>StatusPatch: commit status merge patch
StatusPatch->>KubernetesEvents: record persisted Ready transition
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/controller/watchrule_kstatus_test.go (1)
125-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest names drifted from the functions they now exercise. Both tests were named after helpers this refactor renamed/replaced, but the names weren't updated to match.
internal/controller/watchrule_kstatus_test.go#L125-L181: renameTestApplyRuleKstatus_SourceAuthorizationIsAPrerequisiteto something reflectingruleReadiness/trio(), e.g.TestRuleReadiness_SourceAuthorizationIsAPrerequisite.internal/controller/clusterprovider_controller_unit_test.go#L321-L333: renameTestClusterProviderUpdateStatus_DeletedObjectto reflect the generic status-session flow under test, e.g.TestReconcileStatusCommit_DeletedObject.As per path instructions for
**/*_test.go: "name tests asTestFunctionName_Scenario(t *testing.T)".🤖 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/controller/watchrule_kstatus_test.go` around lines 125 - 181, Rename TestApplyRuleKstatus_SourceAuthorizationIsAPrerequisite to TestRuleReadiness_SourceAuthorizationIsAPrerequisite in internal/controller/watchrule_kstatus_test.go:125-181 so it matches ruleReadiness and trio(). Rename TestClusterProviderUpdateStatus_DeletedObject to TestReconcileStatusCommit_DeletedObject in internal/controller/clusterprovider_controller_unit_test.go:321-333 to reflect the generic status-session flow under test.Source: Path instructions
🤖 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.
Inline comments:
In `@api/v1alpha3/gittarget_types.go`:
- Around line 281-282: Update the comment near the generated GitTarget column
definitions to state four default-priority columns, matching the CRD priorities
for Ready, Reason, Streams, and Age; leave the priority-1 identity fields
unchanged.
In `@docs/spec/status-conditions-guide.md`:
- Around line 42-56: Revise the status-conflict guidance around
reconcileStatus.commit so it does not claim a dropped IsConflict automatically
enqueues a fresh reconcile when GenerationChangedPredicate filters status-only
updates. Either make reconcileStatus.commit return an explicit requeue signal
that callers honor on conflict, or update this documentation to describe
deferred convergence and add a regression test covering status-only conflicts.
In `@internal/controller/gittarget_source_cluster.go`:
- Around line 179-193: Update gitProviderReadiness to preserve
metav1.ConditionUnknown when the referenced GitProvider readiness condition has
Unknown status, mirroring clusterProviderReadiness instead of falling through to
False. Keep explicit True and False handling unchanged, and add a Ready=Unknown
scenario to TestGitProviderReadiness_AllScenarios.
In `@internal/controller/watchrule_controller.go`:
- Around line 111-131: Update stallRule to always set
ConditionTypeStreamsRunning to False with the invalid-reference/no-streams
reason before applying the stalled status, ensuring every terminal stall path
replaces the seeded Unknown condition. Preserve existing GitTargetReady handling
and remove only redundant StreamsRunning updates in refuseSourceNamespace if
centralization makes them unnecessary; add regression coverage for missing
Target.name, missing GitTarget, and missing GitProvider.
---
Nitpick comments:
In `@internal/controller/watchrule_kstatus_test.go`:
- Around line 125-181: Rename
TestApplyRuleKstatus_SourceAuthorizationIsAPrerequisite to
TestRuleReadiness_SourceAuthorizationIsAPrerequisite in
internal/controller/watchrule_kstatus_test.go:125-181 so it matches
ruleReadiness and trio(). Rename TestClusterProviderUpdateStatus_DeletedObject
to TestReconcileStatusCommit_DeletedObject in
internal/controller/clusterprovider_controller_unit_test.go:321-333 to reflect
the generic status-session flow under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe330f71-a66b-478c-815e-ffae55edc38f
📒 Files selected for processing (72)
api/v1alpha3/clusterwatchrule_types.goapi/v1alpha3/gitprovider_types.goapi/v1alpha3/gittarget_types.goapi/v1alpha3/watchrule_types.goapi/v1alpha3/zz_generated.deepcopy.gocmd/main.goconfig/crd/bases/configbutler.ai_clusterwatchrules.yamlconfig/crd/bases/configbutler.ai_gittargets.yamlconfig/crd/bases/configbutler.ai_watchrules.yamlconfig/rbac/role.yamldocs/UPGRADING.mddocs/design/reconcile-triggering.mddocs/facts/subresources.mddocs/future/flux-maintainer-review-status-and-config-model.mddocs/spec/status-conditions-guide.mdinternal/controller/clusterprovider_controller.gointernal/controller/clusterprovider_controller_unit_test.gointernal/controller/clusterwatchrule_admission_test.gointernal/controller/clusterwatchrule_controller.gointernal/controller/commitrequest_controller.gointernal/controller/condition_helper.gointernal/controller/condition_helper_test.gointernal/controller/constants.gointernal/controller/gitprovider_controller.gointernal/controller/gitprovider_controller_test.gointernal/controller/gittarget_controller.gointernal/controller/gittarget_controller_test.gointernal/controller/gittarget_controller_unit_test.gointernal/controller/gittarget_dependency_status.gointernal/controller/gittarget_placement_validation_test.gointernal/controller/gittarget_source_cluster.gointernal/controller/gittarget_source_cluster_test.gointernal/controller/gittarget_status_test.gointernal/controller/readiness.gointernal/controller/ssh_test.gointernal/controller/status.gointernal/controller/stream_status.gointernal/controller/stream_status_test.gointernal/controller/watchrule_controller.gointernal/controller/watchrule_kstatus_test.gointernal/controller/watchrule_source_namespace.gointernal/watch/stream_readiness.gotest/e2e/aggregated_apiserver_e2e_test.gotest/e2e/argocd_bi_directional_e2e_test.gotest/e2e/audit_route_attribution_e2e_test.gotest/e2e/commit_author_attribution_e2e_test.gotest/e2e/commit_request_e2e_test.gotest/e2e/commit_window_batching_e2e_test.gotest/e2e/crd_lifecycle_e2e_test.gotest/e2e/deletecollection_intent_e2e_test.gotest/e2e/demo_e2e_test.gotest/e2e/deployment_scale_author_attribution_e2e_test.gotest/e2e/deployment_scale_subresource_e2e_test.gotest/e2e/e2e_test.gotest/e2e/flux_bi_directional_e2e_test.gotest/e2e/foreign_content_e2e_test.gotest/e2e/gitprovider_validation_e2e_test.gotest/e2e/gittarget_isolation_e2e_test.gotest/e2e/gittarget_overlap_e2e_test.gotest/e2e/helmrelease_mirror_edit_e2e_test.gotest/e2e/inplace_edit_e2e_test.gotest/e2e/new_file_placement_e2e_test.gotest/e2e/prune_mode_e2e_test.gotest/e2e/quickstart_framework_e2e_test.gotest/e2e/restart_reconcile_e2e_test.gotest/e2e/signing_e2e_test.gotest/e2e/source_cluster_e2e_test.gotest/e2e/source_namespace_e2e_test.gotest/e2e/tilt_playground_e2e_test.gotest/e2e/unsupported_folder_e2e_test.gotest/e2e/values_file_acceptance_e2e_test.gotest/e2e/watchrule_configmap_secret_e2e_test.go
💤 Files with no reviewable changes (2)
- config/crd/bases/configbutler.ai_clusterwatchrules.yaml
- config/crd/bases/configbutler.ai_watchrules.yaml
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
Review sweep for 78906bf — one real bug, one doc correction, two declines, plus a merge with main. Fixed
Declined (rationale in-thread): the Merge with main — #264 and #265 had landed. #264 replaced the second Local validation: |
…er reconcile Acts on the Flux-maintainer review in docs/future/flux-maintainer-review-status-and-config-model.md, taking its own ordering: the "Before the next release" and "Next" blocks (F1, F2, F3, F5, F7, F8, F11) plus the cheap F12 nits. The API-surface block (F6 suspend/interval, F9, F10) is left for a separate change. Two pieces of shared machinery carry most of it, and both replace code that had been copied five or six times: internal/controller/readiness.go — gates no longer set Ready/Reconciling/Stalled, they contribute a verdict, and the trio is derived once at the end from a precedence stated in one place. This fixes F1: a GitTarget with a refused Git path (terminal) was handed to the source/provider projection, which stamped Stalled=False, Reconciling=True over the refusal, flipping the object from kstatus Failed to InProgress so `kubectl wait` and every CI gate on it hung to timeout. It replaces eight trio-setting helpers, downgradeReady among them. internal/controller/status.go — a per-reconcile status session that captures the object as read and writes the difference exactly once, as a status patch with optimistic concurrency. It replaces five near-identical updateStatusWithRetry copies and six setCondition wrappers, and with them: the unconditional write that made every reconcile cost roughly two (F3), and the read-modify-write that could stamp an observedGeneration never actually observed (F11). Also: - F2: zero resolved types is Ready=True. A GitTarget with no WatchRules is step 3 of the documented setup flow, and it sat at Reconciling=True forever with nothing that could ever resolve, re-reconciling every 10s for its whole life. status.streams.summary still reads 0/0 and the reason is still NoResolvedTypes. - F3: GenerationChangedPredicate on the For() of GitTarget, WatchRule and ClusterWatchRule, which had none; status.lastReconcileTime and status.streams.observedTime removed rather than excluded from the comparison. - F5: upsertCondition delegates to apimeta.SetStatusCondition, so a touched condition keeps its position instead of migrating to the tail every pass. - F7: every reconciler takes an EventRecorder; the shared writer emits one Event per persisted Ready transition, after the patch rather than beside each write. - F8: generic reasons alias github.com/fluxcd/pkg/apis/meta, already a dependency. "OK" and "Ready" as reasons become "Succeeded"; domain reasons stay. - F12: GitTarget printer columns cut from seven default to four; GitProvider conditions gain the patch-strategy markers; ObjectMeta json tags unified. F4 is resolved the other way, deliberately: the abnormal-true pair keeps being written when False, and docs/spec/status-conditions-guide.md now records that as the single knowing deviation from the API conventions, with the reasons. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dence by kind Six e2e specs still expected reason "Ready" on a GitProvider's Ready condition; the first sweep only caught the argument shape without a message. Reason expectations across the suite are now uniformly "Succeeded". Also splits the requeue cadence rather than sharing one rule, because the two kinds recover differently. A stalled rule waits for an event — a policy change, a Namespace label change, an edit — and every one of those has a watch edge, so it takes the steady interval as before. A stalled GitTarget waits for the Git FOLDER to change, which produces no Kubernetes event at all, so it keeps the fast loop it already had. Drops two reason aliases that nothing used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Newer staticcheck (CI's bundled version, ahead of the pinned golangci-lint 2.12.2) flags mgr.GetEventRecorderFor as deprecated in favour of the events-API GetEventRecorder. GetEventRecorderFor returns the record.EventRecorder the shared status writer emits on, is the ubiquitous kubebuilder idiom, and the replacement's Eventf demands an action verb and a note that do not fit a one-line status Event. Scope the allowance to cmd/main.go, mirroring the existing scheme.Builder SA1019 exclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tTarget gitProviderReadiness documented the same contract clusterProviderReadiness implements — only an EXPLICIT Ready=False downgrades — but its switch had no ConditionFalse arm, so a provider reporting Ready=Unknown fell through to the message/default arms and was published as False. Since gitTargetReadinessGates only gates on ConditionFalse, that pinned every dependent GitTarget at Ready=False for as long as the provider was still validating. Also documents what a dropped status conflict actually costs: every For() in this project carries GenerationChangedPredicate, so a conflict from a metadata- or status-only write does NOT re-enqueue, and the requeue cadence is the backstop. Renames two tests onto the functions they now exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sunib
force-pushed
the
feat/flux-status-contract
branch
from
July 24, 2026 21:12
78906bf to
683a296
Compare
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
1 participant
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.
Acts on the Flux-maintainer review in
docs/future/flux-maintainer-review-status-and-config-model.md,taking that document's own ordering: its §4 "Before the next release" and "Next" blocks
(F1, F2, F3, F5, F7, F8, F11) plus the cheap F12 nits. The API-surface block — F6 (
spec.suspend,spec.interval, reconcile-request annotation), F9, F10 — is deliberately left for a separatechange, because each of those adds or changes a spec field and deserves its own review.
specis unchanged. Everything here is status behaviour, plus two removed status fields and onerenamed reason;
docs/UPGRADING.mdhas the reader-facing entry.Two pieces of shared machinery carry most of it
Both replace code that had been copied five or six times.
internal/controller/readiness.go— the accumulator that owns the trio. Gates no longer setReady/Reconciling/Stalled; they contribute a verdict at one of three levels, and the trio isderived once at the end. Within a level the first contributor wins, so the precedence is the order of
a handful of adjacent calls in
gitTargetReadinessGates/ruleReadiness— readable in one placerather than emergent from which gate happened to run last.
That is F1: a GitTarget whose Git path had been refused (terminal) was then handed to the
source/provider projection, which stamped
Stalled=False, Reconciling=Trueover the refusal becausea provider happened to be mid-check. To a human reading
Readylittle changed. To kstatus — whichnever reads
Ready, only the abnormal-true pair — the object flipped fromFailedtoInProgress,so
kubectl waitand every CI gate built on it waited out its timeout on an object that was nevergoing to converge. It replaced eight trio-setting helpers,
downgradeReadyamong them.internal/controller/status.go— the per-reconcile status session.beginStatuscaptures theobject as read,
set/applyReadinesscollect what the reconcile wants to say, andcommitwritesthe difference exactly once, as a status patch with optimistic concurrency. It replaced five
near-identical
updateStatusWithRetrycopies and sixsetConditionwrappers, and with them:resourceVersionon every pass, fired an Updatewatch event, and re-queued the object — so every reconcile cost roughly two (F3);
observedGenerationthisreconcile never actually observed (F11). A conflict is now dropped rather than retried: the
write that beat us has already enqueued a fresh pass on real data.
The rest
CurrentstreamsAxisreportsTruewhenReady == Total, vacuously true at zero. That state is step 3 of the documented setup flow, and it used to sit atReconciling=Trueforever with nothing that could ever resolve, re-reconciling every 10s for the object's whole life.status.streams.summarystill reads0/0and the reason is stillNoResolvedTypes. "Data plane not wired" is now a different state so the two can't be confused.GenerationChangedPredicateon theFor()of GitTarget, WatchRule and ClusterWatchRule, which had none while both provider kinds did.status.lastReconcileTimeandstatus.streams.observedTimeremoved rather than excluded from the comparison — stamped withmetav1.Now()every pass, they defeated no-op suppression by construction.upsertConditionreordered the list on every touchapimeta.SetStatusCondition, which updates in place. The test now pins position stability instead of de-duplicating an inputlistType=mapmakes impossible.EventRecorder; the shared writer emits one Event per persistedReadytransition. After the patch, not beside eachset, so a reconcile that writesReadytwice announces only the value actually stored.Reason == Typegithubproxy.fjygbaifeng.eu.org/fluxcd/pkg/apis/meta, already a dependency.OKandReadyas reasons becameSucceededacross all six kinds. Domain reasons kept.patchStrategy/patchMergeKeyadded toGitProviderStatus.Conditions;ObjectMetajson tags unified;status.streams.summary's field doc now says why it duplicates the counts beside it.fluxcd/pkg/runtimewas not taken as a dependency forpatch.Helper. That is a newsupply-chain decision, and the ~40-line
commitgets the same three properties fromfluxcd/pkg/apis/meta, which is already pinned.F4 is resolved the other way, on purpose
The review is right that the code and
docs/spec/status-conditions-guide.mddisagreed aboutabnormal-true polarity, and right that "pick one" was the answer. The one picked is keep writing
the pair when False.
kstatus tolerates it (it tests for
== Trueand ignores everything else), so nothing downstreammisreads it. Against removing it:
kubectl wait --for=condition=Stalled=falsereads the explicitFalse, this repo's e2e suite asserts it in several specs, and a condition that vanishes is harderto reason about than one that reads
False. The cost of the deviation is three extra entries in-o yaml; the cost of removing it is a real capability plus a suite-wide rewrite. The guide nowrecords it as the single knowing departure from the API conventions, with the reasons, so the two
cannot silently drift again. What was wrong and is fixed is the incoherent message the review
quoted — it is now written from one place rather than left over from a gate that lost.
Requeue cadence
Split by kind rather than shared, because the two recover differently. A stalled rule waits for
an event (a policy change, a Namespace label change, an edit) and every one has a watch edge, so it
keeps the steady interval. A stalled GitTarget waits for the Git folder to change, which
produces no Kubernetes event at all, so it keeps the fast loop it already had — cheap now that a
re-check finding nothing new writes nothing.
Validation
task lint,task test(coverage 77.7%, baseline 77.8%, within tolerance) andtask test-e2e(69 passed, 0 failed) all pass. A first e2e run caught six real failures where the reason sweep had
missed one call shape; fixed in the second commit and re-run clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Readytransitions (Normal forReady=True, Warning otherwise).GitTargetnow reports terminal readiness asReady=Truewhen noWatchRulesare configured.GitTargetlist output prioritizesProvider,Branch, andPathcolumns for wide views.Breaking Changes
Succeededinstead ofReady/OK.Documentation