Add SSA conditions helpers in internal/utils#335
Open
fwiesel wants to merge 1 commit into
Open
Conversation
This was referenced Jul 13, 2026
📝 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 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 |
This was referenced Jul 13, 2026
Contributor
Author
|
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
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.
fwiesel
force-pushed
the
ssa-conditions-helpers
branch
from
181d69b to
7d13627
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
|
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
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.
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