NO-JIRA: Move constants of ClusterStatusConditionType into one place #1252
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
hongkailiu
force-pushed
the
ClusterStatusConditionType
branch
5 times, most recently
from
6150d88 to
cd6c7c2
Compare
|
With 36fb0ca, 
|
We cannot keep them in `pkg/cvo/status.go` as it would create dependency cycles.
hongkailiu
force-pushed
the
ClusterStatusConditionType
branch
from
36fb0ca to
bfc7f69
Compare
|
/test e2e-extended-tests |
|
/test e2e-hypershift |
No regression issue found in above test, the failed two cases in e2e-extended-tests are both automation issues. |
|
/verified by "the above tests" |
openshift-ci-robot
added
the
verified
|
@hongkailiu: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-hypershift |
WalkthroughMoved several ClusterStatusConditionType constants from pkg/cvo into pkg/internal and updated production code and tests to reference the new internal.* constants (e.g., ClusterStatusFailing → internal.ClusterStatusFailing, DesiredReleaseAccepted → internal.ReleaseAccepted). No other behavioral changes introduced. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
Comment |
|
/test e2e-extended-tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cvo/status.go (1)
666-669: Bug in conflict retry path: wrong error variable checkedIn
applyClusterVersionStatus, the conflict retry checks the originalerrinstead of thecErrfrom the follow-upGet:existing, cErr := client.ClusterVersions().Get(ctx, required.Name, metav1.GetOptions{}) if err != nil { return nil, cErr }This will always take the branch (because
erris the prior conflict) and can return(nil, nil)whencErr == nil, despite not having an updated object.Recommend fixing to check
cErr:- existing, cErr := client.ClusterVersions().Get(ctx, required.Name, metav1.GetOptions{}) - if err != nil { - return nil, cErr - } + existing, cErr := client.ClusterVersions().Get(ctx, required.Name, metav1.GetOptions{}) + if cErr != nil { + return nil, cErr + }This issue is pre-existing but worth addressing while touching this area.
🧹 Nitpick comments (1)
pkg/cvo/cvo_scenarios_test.go (1)
32-35: Scenario tests consistently migrated to internal condition-type constantsThe many expectations on ClusterVersion conditions now use
internal.ImplicitlyEnabledCapabilities,internal.ClusterStatusFailing, andinternal.DesiredReleaseAccepted, keeping tests in lockstep with the centralized definitions while preserving all behaviors; any remaining raw"Failing"string types are functionally equivalent and can be unified later if desired.Also applies to: 188-3860
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
pkg/cvo/cvo_scenarios_test.go(31 hunks)pkg/cvo/cvo_test.go(50 hunks)pkg/cvo/metrics.go(1 hunks)pkg/cvo/metrics_test.go(6 hunks)pkg/cvo/status.go(7 hunks)pkg/cvo/status_test.go(19 hunks)pkg/cvo/upgradeable.go(9 hunks)pkg/cvo/upgradeable_test.go(3 hunks)pkg/internal/constants.go(2 hunks)pkg/payload/precondition/clusterversion/upgradeable.go(2 hunks)pkg/start/start_integration_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cvo/cvo_scenarios_test.gopkg/cvo/upgradeable_test.gopkg/start/start_integration_test.gopkg/cvo/status.gopkg/cvo/metrics.gopkg/internal/constants.gopkg/cvo/upgradeable.gopkg/cvo/cvo_test.gopkg/cvo/metrics_test.gopkg/payload/precondition/clusterversion/upgradeable.gopkg/cvo/status_test.go
🔇 Additional comments (10)
pkg/internal/constants.go (1)
3-7: Centralized ClusterStatusConditionType constants look correct and well-documentedThe new internal constants keep the existing string values and types, and the added GoDoc makes these user-facing conditions clearer while avoiding dependency cycles.
Also applies to: 37-70
pkg/cvo/metrics_test.go (1)
26-27: Metrics tests correctly migrated to internal condition constantsUsing
internal.ClusterStatusFailingfor failing conditions and plain string literals for custom/unknown types keeps the tests aligned with the new central constants while preserving behavior and label expectations.Also applies to: 334-336, 484-485, 522-523, 721-742
pkg/start/start_integration_test.go (1)
34-35: Integration tests now consistently use internal.ClusterStatusFailingBoth failing-condition guards in
waitForUpdateAvailablecorrectly referenceinternal.ClusterStatusFailing, matching the new centralized definition without changing behavior.Also applies to: 607-613, 681-683
pkg/cvo/metrics.go (1)
520-534: Failure metric lookup correctly switched to internal.ClusterStatusFailingThe failing-condition lookup in
Collectnow uses the centralizedinternal.ClusterStatusFailingconstant, preserving behavior while reducing duplication.pkg/cvo/upgradeable_test.go (1)
12-13: Upgradeable tests correctly reference internal.DesiredReleaseAcceptedSwitching the condition
Typefields tointernal.DesiredReleaseAcceptedkeeps the tests aligned with the new shared constant while leaving the throttle-period logic unchanged.Also applies to: 27-49
pkg/payload/precondition/clusterversion/upgradeable.go (1)
16-18: Overrides precondition now uses centralized UpgradeableClusterVersionOverridesUsing
internal.UpgradeableClusterVersionOverridesinClusterVersionOverridesConditioncentralizes the condition type without altering the upgrade-blocking logic.Also applies to: 34-41
pkg/cvo/status_test.go (1)
6-7: Tests correctly track internal.ClusterStatusFailing moveImporting
pkg/internaland swapping all failing-conditionTypefields and lookups tointernal.ClusterStatusFailingkeeps the test behavior identical while centralizing the constant definition. No issues spotted with the updated expectations.Also applies to: 116-117, 157-158, 256-259, 268-271, 287-297, 318-329, 351-360, 383-394, 426-460, 486-497, 523-533, 563-567, 590-599, 621-632, 654-665, 688-699, 746-747
pkg/cvo/cvo_test.go (1)
47-50: Internal condition-type constants are used consistently in testsThe switch from public constants/literals to
internal.ClusterStatusFailing,internal.ImplicitlyEnabledCapabilities, andinternal.ClusterVersionInvalidin the various sync/available-updates/invalid-clusterversion tests is consistent and preserves the asserted status shapes. This tightly couples tests to the condition type strings in one place, which is appropriate given the refactor.Also applies to: 225-228, 257-262, 300-306, 378-381, 450-456, 538-541, 668-671, 712-716, 755-759, 813-819, 868-875, 928-933, 981-987, 1081-1084, 1125-1131, 1210-1214, 1279-1284, 1371-1376, 1450-1456, 1526-1534, 1612-1617, 1698-1702, 1796-1799, 1871-1877, 1979-1987, 2067-2073, 2170-2176, 2210-2216, 2483-2486, 2546-2552, 2613-2619, 2675-2681, 2715-2719
pkg/cvo/status.go (1)
25-29: Condition-type constants successfully centralized in pkg/internalUsing
internal.ClusterVersionInvalid,internal.ClusterStatusFailing,internal.ImplicitlyEnabledCapabilities, andinternal.DesiredReleaseAcceptedin status calculation and sync-failure paths keeps the logic identical while avoiding direct constant definitions incvo. Given the tests now also reference these internal constants, the refactor looks safe and maintainable.Also applies to: 248-257, 300-321, 447-481, 483-511, 486-511, 628-632
pkg/cvo/upgradeable.go (1)
30-35: Upgradeable condition and throttle constants correctly moved to pkg/internalThe upgradeable checks now emit condition Types via
internal.UpgradeableClusterOperators,internal.UpgradeableClusterVersionOverrides,internal.UpgradeableDeletesInProgress, andinternal.UpgradeableAdminAckRequired, andthrottlePeriodkeys offinternal.DesiredReleaseAccepted. Together with usinginternal.AdminGatesConfigMap/internal.AdminAcksConfigMap, this centralizes condition and ConfigMap names without altering the surrounding logic.Also applies to: 188-193, 241-243, 298-300, 315-320, 362-367, 386-436, 533-539
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/cvo/cvo_scenarios_test.go (1)
31-3653: Test expectations consistently aligned to internal condition-type constantsAcross these scenarios, switching
Typefields tointernal.ImplicitlyEnabledCapabilities,internal.ClusterStatusFailing, andinternal.ReleaseAcceptedkeeps the condition strings stable while routing everything through the new shared constants. This improves maintainability and avoids divergence between tests and production code, with no observable behavior change.You might later consider pulling remaining hard-coded condition-type strings (like
"Failing"/"Available"where applicable) into the same internal constants for full consistency, but that is not required for this PR.pkg/cvo/upgradeable.go (1)
25-31: Upgradeable checks correctly refactored to use internal condition-type constantsThe upgradeable checks and throttle logic now consistently use
internal.Upgradeable*andinternal.ReleaseAccepted, centralizing condition-type definitions without altering behavior. This reduces the risk of drift and dependency cycles between packages.If you continue consolidating condition-type strings, you could also move
"UpgradeableUpgradeInProgress"intopkg/internalfor symmetry with the other upgradeable condition types in a future change.Also applies to: 187-232, 239-263, 296-309, 311-438, 533-539
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
pkg/cvo/cvo_scenarios_test.go(31 hunks)pkg/cvo/status.go(7 hunks)pkg/cvo/upgradeable.go(9 hunks)pkg/cvo/upgradeable_test.go(3 hunks)pkg/internal/constants.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/internal/constants.gopkg/cvo/status.gopkg/cvo/cvo_scenarios_test.gopkg/cvo/upgradeable.gopkg/cvo/upgradeable_test.go
🔇 Additional comments (3)
pkg/internal/constants.go (1)
37-69: Centralizing condition-type constants looks correctThe new
configv1.ClusterStatusConditionTypeconstants are well-documented, string values match their names, and they provide a single source of truth for the user-facing condition types without changing behavior.pkg/cvo/upgradeable_test.go (1)
12-49: Tests correctly switched to internalReleaseAcceptedconstantUsing
internal.ReleaseAcceptedin these test conditions aligns them with the new centralized constant definitions without altering the expected behavior.pkg/cvo/status.go (1)
25-28: Status condition wiring cleanly migrated to internal constantsUsing
internal.ClusterVersionInvalid,internal.ClusterStatusFailing,internal.ImplicitlyEnabledCapabilities, andinternal.ReleaseAcceptedhere keeps condition-type strings centralized while preserving existing behavior for validation, failing, capabilities, and release-acceptance conditions.Also applies to: 248-257, 300-320, 447-480, 483-511, 627-632
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/cvo/status.go (1)
248-257: Consistent use ofinternalcondition types looks correctSwitching the various condition types to
internal.*(ClusterVersionInvalid,ClusterStatusFailing,ImplicitlyEnabledCapabilities,ReleaseAccepted) plus extractingsetReleaseAcceptedConditionis a straightforward refactor: control flow, status transitions, and messages are unchanged and only the constant source moves.As long as the new
internalconstants preserve the existing string values, this should be a pure, non‑breaking change and improves maintainability by centralizing condition type definitions.If you want extra guardrails, consider a small unit test in
pkg/internal(or a doc comment) asserting these condition strings remain stable over time, since they are surfaced via status and may be consumed externally.Also applies to: 262-263, 300-320, 466-481, 483-511, 628-632
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cvo/status.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cvo/status.go
🔇 Additional comments (1)
pkg/cvo/status.go (1)
25-29: Internal constants import improves dependency structureImporting
pkg/internalhere to source sharedClusterStatusConditionTypeconstants centralizes their definition and avoids local duplication/cycles. No issues with this change.
5 participants
We cannot keep them in
pkg/cvo/status.goas it would createdependency cycles.