Add SSA conditions helpers in internal/utils - #335
Conversation
|
Warning Review limit reached
Next review available in: 37 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds utilities to convert Kubernetes status conditions into apply configurations and to append or update apply-configuration conditions with tested transition-time semantics. ChangesCondition Utilities
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Consumer PRs that depend on this foundation:
Final cleanup that removes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/utils/conditions.go (1)
118-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
strChangedandint64Changedsignatures.Both helpers take a double-pointer (
**string/**int64) forabut only read*a— they never write through it. Passing the single pointer directly removes the unnecessary indirection and makes call sites cleaner.♻️ Proposed refactor
-func strChanged(a **string, b *string) bool { - if *a == nil && b == nil { +func strChanged(a, b *string) bool { + if a == nil && b == nil { return false } - if *a == nil || b == nil { + if a == nil || b == nil { return true } - return **a != *b + return *a != *b } -func int64Changed(a **int64, b *int64) bool { - if *a == nil && b == nil { +func int64Changed(a, b *int64) bool { + if a == nil && b == nil { return false } - if *a == nil || b == nil { + if a == nil || b == nil { return true } - return **a != *b + return *a != *b }And update call sites:
- if strChanged(&existing.Reason, newCondition.Reason) { + if strChanged(existing.Reason, newCondition.Reason) { existing.Reason = newCondition.Reason changed = true } - if strChanged(&existing.Message, newCondition.Message) { + if strChanged(existing.Message, newCondition.Message) { existing.Message = newCondition.Message changed = true } - if int64Changed(&existing.ObservedGeneration, newCondition.ObservedGeneration) { + if int64Changed(existing.ObservedGeneration, newCondition.ObservedGeneration) { existing.ObservedGeneration = newCondition.ObservedGeneration changed = true }🤖 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/utils/conditions.go` around lines 118 - 138, Change strChanged and int64Changed to accept single pointers for both arguments, preserving their nil and value-comparison behavior. Update every call site to pass the existing pointer value directly rather than its address, and adjust any affected comments or type references.
🤖 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/utils/conditions.go`:
- Around line 118-138: Change strChanged and int64Changed to accept single
pointers for both arguments, preserving their nil and value-comparison behavior.
Update every call site to pass the existing pointer value directly rather than
its address, and adjust any affected comments or type references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1658255-2e6f-496a-9ede-4e8b53bbab3c
📒 Files selected for processing (1)
internal/utils/conditions.go
181d69b to
7d13627
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/utils/conditions_test.go (1)
130-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
ObservedGenerationupdates and nil-field comparisons.This block thoroughly covers
Status/Reason/Messagetransitions, but none of theBeforeEachfixtures or new-condition builders ever setObservedGeneration, soint64Changedis never exercised with a real value change. There's also no case forexisting.Status == nilor mismatched nilReason/Message, which the nil-safe comparison logic inSetApplyConfigurationStatusConditionis specifically designed to handle.Consider adding cases like:
It("updates ObservedGeneration and reports changed", func() { gen := int64(5) changed := SetApplyConfigurationStatusCondition(&conditions, *k8sacmetav1.Condition(). WithType("Ready"). WithStatus(metav1.ConditionFalse). WithReason("NotReady"). WithMessage("waiting"). WithObservedGeneration(gen)) Expect(changed).To(BeTrue()) Expect(*conditions[0].ObservedGeneration).To(Equal(gen)) })🤖 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/utils/conditions_test.go` around lines 130 - 222, Add test coverage in the existing “updating an existing condition” Describe block for SetApplyConfigurationStatusCondition: include an ObservedGeneration value that changes and assert the update is reported and stored, plus cases exercising nil existing Status and mismatched nil/non-nil Reason or Message comparisons. Keep the assertions focused on changed status and resulting fields, while preserving the existing transition-time behavior.
🤖 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/utils/conditions_test.go`:
- Around line 130-222: Add test coverage in the existing “updating an existing
condition” Describe block for SetApplyConfigurationStatusCondition: include an
ObservedGeneration value that changes and assert the update is reported and
stored, plus cases exercising nil existing Status and mismatched nil/non-nil
Reason or Message comparisons. Keep the assertions focused on changed status and
resulting fields, while preserving the existing transition-time behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f87c997f-4ed5-4252-b8ae-fa0b7b0e8d27
📒 Files selected for processing (2)
internal/utils/conditions.gointernal/utils/conditions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/utils/conditions.go
7d13627 to
dd0de9c
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/utils/conditions_test.go`:
- Around line 236-246: Add a regression test alongside the existing
status-condition tests for two conditions whose Status pointers are both nil,
asserting SetApplyConfigurationStatusCondition returns false and preserves
LastTransitionTime. Update SetApplyConfigurationStatusCondition in
internal/utils/conditions.go to treat two nil Status values as equal, while
retaining existing behavior for differing or non-nil statuses.
🪄 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
Run ID: 390a83f9-1078-419d-93e8-713246965b30
📒 Files selected for processing (2)
internal/utils/conditions.gointernal/utils/conditions_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/utils/conditions.go
Introduce ConditionFromStatus and SetApplyConfigurationStatusCondition as the foundation for migrating all controllers to server-side apply of status subresources. ConditionFromStatus converts a single metav1.Condition (as stored in a status subresource) to a ConditionApplyConfiguration suitable for SSA. ObservedGeneration is preserved so gating logic (e.g. the taint controller's skip-check) keeps working. SetApplyConfigurationStatusCondition mirrors the semantics of meta.SetStatusCondition on the apply-configuration slice: append or update by Type, refresh LastTransitionTime only when Status changes, and guard against nil/empty Type to prevent panics. Together they enable per-condition seeding so each controller claims field ownership only over the conditions it actually manages, without disturbing conditions owned by other field managers on Apply.
dd0de9c to
40e4306
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
| statusChanged := (existing.Status == nil) != (newCondition.Status == nil) || | ||
| (existing.Status != nil && *existing.Status != *newCondition.Status) |
There was a problem hiding this comment.
Why can't this use strChanged? If I read it correctly ConditionStatus is a type of string.
There was a problem hiding this comment.
Ok, it's because golang doesn't see it as string anymore if it's a named string type.
AI suggests to "introduce a generic ptrChanged[T comparable] helper" to replace all 3 needed compare helpers. In any case, I got my answer.
notandy
left a comment
notandy
left a comment
There was a problem hiding this comment.
I was thinking that we could extend v1.Hypervisor{} with a unexported field that holds (an optional) SSA HypervisorStatusApplyConfiguration. This way we wouldn't need to pass it along every other function and could check for changes (against it's own self.Status) on demand when needed, instead of unconditionally.
Not sure, if I got your point correctly: In my mind, SSA make it easier to just declare the desired "state". No need to check, if something needs to get added/removed, changed or not. That being said, that is the ideal state, and often you need the information. The code now is more or less just replacing the patch with SSA apply and just for the status and intentionally not refactored, as the delta is already huge. I think, there is still a lot of room of improvement, it was just the "smallest unit of change" I thought sensible. |
Exactly, the code is just replacing SSA apply without really caring about SSA, and forcing every callee to accept an additional field in their signature. The request makes sense because it would reduce the boilerplate introduced by the switch to SSA. Although this PR is large, you're asking to cling on some old behaviour while introducing a massive change - and requiring to follow up with other big changes to make it "nicer". Not sure I see the logic in this. |
A proper refactoring would have touched effectively all lines of codes. I think, now I understand:
I do not think, that is a good idea. Preserving the signature of a function only makes sense to me if there are external users, and an API need to be kept. I do not understand the need to enforce that in private functions. Putting it inside another object simply hides that the function needs that information. I do see that there is a duplication between |
It's less about not changing the signature at all, but that you introduce a behavioural change. Before SSA, the
Let's keep the hypothetical out of this discussion, there are multiple ways to achieve the same goal, let's find the best one. |
3 participants
Foundation for migrating all controllers to Server-Side Apply of the status subresource.
Introduces two helpers in
internal/utils/conditions.go:ConditionFromStatus: converts a singlemetav1.Condition(as stored in a status subresource) to aConditionApplyConfigurationsuitable for SSA.ObservedGenerationis preserved so gating logic (e.g. the taint controller's skip-check) keeps working.SetApplyConfigurationStatusCondition: mirrors the semantics ofmeta.SetStatusConditionon the apply-configuration slice: append or update byType, refreshLastTransitionTimeonly whenStatuschanges, guard against nil/emptyTypeto prevent panics.Together they enable per-condition seeding so each controller claims field ownership only over the conditions it actually manages, without disturbing conditions owned by other field managers on Apply.
Summary by CodeRabbit
New Features
Tests