Centralize and harmonize condition/reason constants across controllers#795
Open
Centralize and harmonize condition/reason constants across controllers#795
Conversation
Condition and reason constants were scattered across controller files with inconsistent naming conventions (mixed prefixes, unexported names, BIOS/BMC-specific names for generic concepts) and true duplicates using identical string values. This made it hard to discover existing constants and led to accidental duplication when adding new controllers. Consolidate shared condition and reason constants into a central conditions.go file and normalize naming to a consistent Condition<Name>/Reason<Name> pattern. Generalize BIOS/BMC-specific names and string values where the concepts are generic (e.g., ConditionBMCResetIssued → ConditionResetIssued, "BIOSSettingsTimedOut" → "SettingsTimedOut"). This is a breaking change for existing CRs due to changed condition type and reason string values.
Contributor
WalkthroughConsolidates controller condition and reason constants into a new shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/controller/bmcversion_controller.go (1)
294-363:⚠️ Potential issue | 🟠 MajorDon’t rename these persisted upgrade checkpoints without a compatibility layer.
ConditionVersionUpgradeIssued,ConditionVersionUpgradeCompleted,ConditionVersionUpgradeReboot, andConditionVersionUpgradeVerificationare the upgrade state machine’s checkpoints. After this rollout, aBMCVersionthat was already mid-flight with the legacy condition types will read as “not started” here, and we can issue a second firmware upgrade task or BMC reset against the same device. Please keep the storedTypestrings compatible or migrate legacy entries before these checks run.As per coding guidelines "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmcversion_controller.go` around lines 294 - 363, The checks for ConditionVersionUpgradeIssued, ConditionVersionUpgradeCompleted, ConditionVersionUpgradeReboot and ConditionVersionUpgradeVerification will treat existing BMCVersion resources with legacy condition Type strings as "not started" and may re-issue upgrades; to fix this, add a compatibility/migration step that runs before these GetCondition calls (e.g., a function MigrateLegacyConditionTypes or Extend GetCondition to accept legacy aliases) which scans bmcVersion.Status.Conditions, maps legacy Type values to the current constants (ConditionVersionUpgradeIssued, ConditionVersionUpgradeCompleted, ConditionVersionUpgradeReboot, ConditionVersionUpgradeVerification) and updates the resource status (or treats legacy names as equivalent at read time) so the existing in-flight upgrades are recognized; ensure this migration is idempotent and runs once per resource before issuing upgrades or resets, and reference the existing symbols in this file (GetCondition, r.Conditions.Update, r.patchBMCVersionStatusAndCondition, and the ConditionVersionUpgrade* constants) when implementing it.internal/controller/biosversion_controller.go (1)
305-327:⚠️ Potential issue | 🔴 CriticalPreserve legacy condition checkpoints before re-entering upgrade/reset steps.
BIOSVersion.Status.Conditionsis acting as the reconciler’s checkpoint store here. After this rename, any already-runningBIOSVersionthat still carries the old condition types/reasons will miss theseGetCondition(...)gates and can re-enterupgradeBIOSVersion()orresetBMCOfServer()after an operator rollout. That turns a status-string rename into a duplicate external action hazard.Please add backward-compatible lookups or migrate legacy condition entries before branching on these checkpoints.
As per coding guidelines "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".
Also applies to: 413-445, 466-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/biosversion_controller.go` around lines 305 - 327, GetCondition lookups against BIOSVersion.Status.Conditions must tolerate legacy condition names/types before deciding to call upgradeBIOSVersion or resetBMCOfServer; update the code around the GetCondition(...) calls (e.g., where ConditionVersionUpgradeIssued and ConditionVersionUpgradeCompleted are checked) to first search for legacy condition keys or migrate legacy entries into the new condition names (or treat legacy matches as equivalent) so old in-flight BIOSVersion objects won’t slip past the gate; ensure this compatibility lookup/migration happens prior to branching (before calling upgradeBIOSVersion, resetBMCOfServer, or returning) and apply the same pattern to the other blocks noted (around lines checking ConditionVersionUpgradeCompleted and the reset logic).internal/controller/biosversion_controller_test.go (1)
603-622:⚠️ Potential issue | 🟡 MinorAssert the reboot checkpoints instead of rechecking completion.
Lines 603 and 622 still look up
ConditionVersionUpgradeCompleted.processInProgressState()now records those steps underConditionUpgradePowerOffandConditionUpgradePowerOn, so these assertions can pass without ever validating the renamed reboot conditions.Suggested test fix
- g.Expect(acc.FindSlice(biosVersion.Status.Conditions, ConditionVersionUpgradeCompleted, rebootStart)).To(BeTrue()) + g.Expect(acc.FindSlice(biosVersion.Status.Conditions, ConditionUpgradePowerOff, rebootStart)).To(BeTrue()) ... - g.Expect(acc.FindSlice(biosVersion.Status.Conditions, ConditionVersionUpgradeCompleted, rebootComplete)).To(BeTrue()) + g.Expect(acc.FindSlice(biosVersion.Status.Conditions, ConditionUpgradePowerOn, rebootComplete)).To(BeTrue())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/biosversion_controller_test.go` around lines 603 - 622, Replace the two assertions that look up ConditionVersionUpgradeCompleted for the reboot checkpoints with the new reboot condition names: when validating the pre-reboot checkpoint currently stored in variable rebootStart use ConditionUpgradePowerOff, and when validating the post-reboot checkpoint stored in rebootComplete use ConditionUpgradePowerOn; keep the same pattern of using acc.FindSlice(...) and asserting the condition's Status == metav1.ConditionTrue so the test asserts the actual reboot checkpoints instead of the renamed completion condition.
🧹 Nitpick comments (1)
internal/controller/bmc_controller.go (1)
163-166: CentralizeResetCompletetoo.
ConditionResetis shared now, but the terminal reason is still a raw string here, so the condition contract is still split across controller code and tests. Adding a sharedReasonResetCompleteconstant would finish the cleanup and make future renames compile-time safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmc_controller.go` around lines 163 - 166, The code uses the raw string "ResetComplete" as the terminal reason when calling updateConditions for ConditionReset; introduce a package-level constant ReasonResetComplete (matching the existing ReasonConnected/ReasonXXX pattern) and replace the literal "ResetComplete" with ReasonResetComplete in the updateConditions call (and any other occurrences/tests) so the reset reason is centralized and compile-time-safe; add the new ReasonResetComplete constant next to the other reason constants and run tests to update any assertions referencing the raw string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/biossettings_controller.go`:
- Around line 37-63: The constants like ConditionPendingSettingsCheck,
ConditionSettingsDuplicateKeys, ConditionSettingsUpdateStartTime, etc. are used
as serialized keys in status.conditions and flowState[].conditions; renaming
them breaks idempotency for existing CRs. Fix by adding legacy aliases and a
small migration in the controller: define legacy constant values mapping
oldName->newName (e.g., LegacyConditionX -> ConditionX) and update the condition
lookup utilities (the codepaths that call GetCondition / FindSlice) to treat
either legacy or new strings as equivalent (or on reconcile, detect legacy keys
in status.conditions/flowState and replace them with the new constants once).
Ensure both the lookup and the reconciliation step that emits conditions write
the new names going forward so upgrades remain compatible and no duplicate
actions are reissued.
In `@internal/controller/bmcsettings_controller.go`:
- Line 377: The reconcile check using handleBMCReset and ConditionResetIssued
must accept legacy condition type names before deciding a reset has been issued;
update the gate that calls handleBMCReset(ctx, settings, bmcObj,
ConditionResetIssued) (and similar checks around SetBMCAttributesImmediately and
GetCondition) to first map/translate any legacy status.conditions[].type values
to the current constants (or check both the new and legacy names) so existing
InProgress BMCSettings are recognized as already progressed and do not
re-trigger resetting or re-issuing annotations; ensure the translation occurs
before invoking GetCondition/handleBMCReset and apply the same mapping in the
other ranges noted (around lines that call SetBMCAttributesImmediately and
additional GetCondition checks).
In `@internal/controller/conditions.go`:
- Around line 7-74: Controllers will miss legacy condition Type values after
renaming constants like ConditionServerMaintenanceCreated,
ConditionServerMaintenanceDeleted, ConditionServerMaintenanceWaiting,
ConditionResetIssued, ConditionVersionUpgradeIssued, etc., so add a
normalization/migration step: implement a small helper (e.g.,
NormalizeConditionTypes or migrateLegacyConditionTypes) that scans an object's
status.Conditions and rewrites legacy Type strings to the new constant values,
use this helper in places that call GetCondition (or in the controller Reconcile
before any condition reads/writes) so existing objects are normalized at
runtime; alternatively provide a one-shot migration controller that performs the
same update on all objects and then removes itself.
---
Outside diff comments:
In `@internal/controller/biosversion_controller_test.go`:
- Around line 603-622: Replace the two assertions that look up
ConditionVersionUpgradeCompleted for the reboot checkpoints with the new reboot
condition names: when validating the pre-reboot checkpoint currently stored in
variable rebootStart use ConditionUpgradePowerOff, and when validating the
post-reboot checkpoint stored in rebootComplete use ConditionUpgradePowerOn;
keep the same pattern of using acc.FindSlice(...) and asserting the condition's
Status == metav1.ConditionTrue so the test asserts the actual reboot checkpoints
instead of the renamed completion condition.
In `@internal/controller/biosversion_controller.go`:
- Around line 305-327: GetCondition lookups against
BIOSVersion.Status.Conditions must tolerate legacy condition names/types before
deciding to call upgradeBIOSVersion or resetBMCOfServer; update the code around
the GetCondition(...) calls (e.g., where ConditionVersionUpgradeIssued and
ConditionVersionUpgradeCompleted are checked) to first search for legacy
condition keys or migrate legacy entries into the new condition names (or treat
legacy matches as equivalent) so old in-flight BIOSVersion objects won’t slip
past the gate; ensure this compatibility lookup/migration happens prior to
branching (before calling upgradeBIOSVersion, resetBMCOfServer, or returning)
and apply the same pattern to the other blocks noted (around lines checking
ConditionVersionUpgradeCompleted and the reset logic).
In `@internal/controller/bmcversion_controller.go`:
- Around line 294-363: The checks for ConditionVersionUpgradeIssued,
ConditionVersionUpgradeCompleted, ConditionVersionUpgradeReboot and
ConditionVersionUpgradeVerification will treat existing BMCVersion resources
with legacy condition Type strings as "not started" and may re-issue upgrades;
to fix this, add a compatibility/migration step that runs before these
GetCondition calls (e.g., a function MigrateLegacyConditionTypes or Extend
GetCondition to accept legacy aliases) which scans bmcVersion.Status.Conditions,
maps legacy Type values to the current constants (ConditionVersionUpgradeIssued,
ConditionVersionUpgradeCompleted, ConditionVersionUpgradeReboot,
ConditionVersionUpgradeVerification) and updates the resource status (or treats
legacy names as equivalent at read time) so the existing in-flight upgrades are
recognized; ensure this migration is idempotent and runs once per resource
before issuing upgrades or resets, and reference the existing symbols in this
file (GetCondition, r.Conditions.Update, r.patchBMCVersionStatusAndCondition,
and the ConditionVersionUpgrade* constants) when implementing it.
---
Nitpick comments:
In `@internal/controller/bmc_controller.go`:
- Around line 163-166: The code uses the raw string "ResetComplete" as the
terminal reason when calling updateConditions for ConditionReset; introduce a
package-level constant ReasonResetComplete (matching the existing
ReasonConnected/ReasonXXX pattern) and replace the literal "ResetComplete" with
ReasonResetComplete in the updateConditions call (and any other
occurrences/tests) so the reset reason is centralized and compile-time-safe; add
the new ReasonResetComplete constant next to the other reason constants and run
tests to update any assertions referencing the raw string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7496651c-e543-4bf6-badf-74769565f2b9
📒 Files selected for processing (22)
internal/controller/biossettings_controller.gointernal/controller/biossettings_controller_test.gointernal/controller/biossettingsset_controller.gointernal/controller/biosversion_controller.gointernal/controller/biosversion_controller_test.gointernal/controller/biosversionset_controller.gointernal/controller/bmc_controller.gointernal/controller/bmc_controller_test.gointernal/controller/bmcsettings_controller.gointernal/controller/bmcsettingsset_controller.gointernal/controller/bmcuser_controller.gointernal/controller/bmcversion_controller.gointernal/controller/bmcversion_controller_test.gointernal/controller/bmcversionset_controller.gointernal/controller/conditions.gointernal/controller/endpoint_controller.gointernal/controller/helper.gointernal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/controller/serverclaim_controller.gointernal/controller/serverclaim_controller_test.gointernal/controller/servermaintenance_controller.go
💤 Files with no reviewable changes (1)
- internal/controller/helper.go
Member
Author
|
@coderabbitai create an issue to remove the condition migration logic in the next release. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me first look for the condition migration logic in the codebase. 🏁 Script executed: Length of output: 0 Let me search more broadly for any condition-related migration or transition logic. 🏁 Script executed: Length of output: 21020 Found the migration logic. Let me now find all its call sites before creating the issue. 🏁 Script executed: Length of output: 5537 Now I have all the details. Let me create the GitHub issue. 🏁 Script executed: Length of output: 57
The issue covers all five items marked with
🧠 Learnings used |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/controller/bmcsettings_controller.go (1)
58-62:⚠️ Potential issue | 🔴 CriticalMigrate the renamed BMCSettings checkpoint types too.
Line 61 only rewrites
BMCResetIssued, but this controller now uses other renamed persisted BMCSettings condition types as workflow checkpoints as well (ConditionBMCResetPostSettingApply,ConditionBMCPoweredOff,ConditionBMCSettingsChangesIssued,ConditionBMCSettingsChangesVerified). ExistingInProgressCRs that already passed one of those steps can look unfinished after rollout and re-issue settings or the post-apply reset.As per coding guidelines "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmcsettings_controller.go` around lines 58 - 62, Update the legacyBMCSettingsConditionTypes migration map to include the other renamed checkpoint condition types so persisted CRs are rewritten to the new constants and reconciliation stays idempotent; specifically add mappings for the old string names corresponding to ConditionBMCResetPostSettingApply, ConditionBMCPoweredOff, ConditionBMCSettingsChangesIssued, and ConditionBMCSettingsChangesVerified into the existing legacyBMCSettingsConditionTypes map and ensure the reconciliation code that applies this map (the logic using legacyBMCSettingsConditionTypes) will translate any of those legacy keys to the new constants before deciding workflow checkpoints.internal/controller/biossettings_controller.go (1)
66-77:⚠️ Potential issue | 🔴 CriticalComplete the BIOSSettings legacy map for the flow-state checkpoints.
This map only migrates part of the renamed serialized condition types. The later flow-state gates still read renamed persisted checkpoints like
ConditionSettingsUpdateIssued,ConditionSettingsRebootPowerOff,ConditionSettingsRebootPowerOn,ConditionSettingsVerify, andConditionSettingsInvalid. An existingInProgressBIOSSettings can therefore fall back into earlier phases after upgrade and replay setting writes or reboot steps.As per coding guidelines "Implement idempotent reconciliation logic - safe to run the same reconciliation multiple times without side effects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/biossettings_controller.go` around lines 66 - 77, The legacyBIOSSettingsConditionTypes map is missing mappings for later flow-state checkpoint condition names, causing upgraded InProgress BIOSSettings to replay earlier phases; update the map in internal/controller/biossettings_controller.go (legacyBIOSSettingsConditionTypes) to include the old serialized keys for ConditionSettingsUpdateIssued, ConditionSettingsRebootPowerOff, ConditionSettingsRebootPowerOn, ConditionSettingsVerify, and ConditionSettingsInvalid mapped to their corresponding new constants (ConditionSettingsUpdateIssued, ConditionSettingsRebootPowerOff, ConditionSettingsRebootPowerOn, ConditionSettingsVerify, ConditionSettingsInvalid) so deserialization uses the new names and reconciliation remains idempotent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/biossettings_controller.go`:
- Around line 66-77: The legacyBIOSSettingsConditionTypes map is missing
mappings for later flow-state checkpoint condition names, causing upgraded
InProgress BIOSSettings to replay earlier phases; update the map in
internal/controller/biossettings_controller.go
(legacyBIOSSettingsConditionTypes) to include the old serialized keys for
ConditionSettingsUpdateIssued, ConditionSettingsRebootPowerOff,
ConditionSettingsRebootPowerOn, ConditionSettingsVerify, and
ConditionSettingsInvalid mapped to their corresponding new constants
(ConditionSettingsUpdateIssued, ConditionSettingsRebootPowerOff,
ConditionSettingsRebootPowerOn, ConditionSettingsVerify,
ConditionSettingsInvalid) so deserialization uses the new names and
reconciliation remains idempotent.
In `@internal/controller/bmcsettings_controller.go`:
- Around line 58-62: Update the legacyBMCSettingsConditionTypes migration map to
include the other renamed checkpoint condition types so persisted CRs are
rewritten to the new constants and reconciliation stays idempotent; specifically
add mappings for the old string names corresponding to
ConditionBMCResetPostSettingApply, ConditionBMCPoweredOff,
ConditionBMCSettingsChangesIssued, and ConditionBMCSettingsChangesVerified into
the existing legacyBMCSettingsConditionTypes map and ensure the reconciliation
code that applies this map (the logic using legacyBMCSettingsConditionTypes)
will translate any of those legacy keys to the new constants before deciding
workflow checkpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01928ccd-9998-468b-b52e-890eadadb257
📒 Files selected for processing (5)
internal/controller/biossettings_controller.gointernal/controller/biosversion_controller.gointernal/controller/bmcsettings_controller.gointernal/controller/bmcversion_controller.gointernal/controller/helper.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
2 participants
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.
Proposed Changes
Condition and reason constants were scattered across controller files with inconsistent naming conventions (mixed prefixes, unexported names, BIOS/BMC-specific names for generic concepts) and true duplicates using identical string values. This made it hard to discover existing constants and led to accidental duplication when adding new controllers.
Consolidate shared condition and reason constants into a central conditions.go file and normalize naming to a consistent Condition/Reason pattern. Generalize BIOS/BMC-specific names and string values where the concepts are generic (e.g., ConditionBMCResetIssued → ConditionResetIssued,
"BIOSSettingsTimedOut" → "SettingsTimedOut").
Since this change would be breaking, a migration logic has been introduced which migrate the legacy conditions to the new format.
Summary by CodeRabbit
Release Notes
Refactor
Chores / Migration