NO-ISSUE: pkg/cli/admin/upgrade/recommend: Drop obsolete precheckEnabled knob #2113
Conversation
Since 8bdac16 (pkg/cli/admin/upgrade/recommend: Enable precheck and accept gates, 2025-09-03, openshift#2088), this knob has always been enabled. Save ourselves a no longer necessary level of indentation by removing it completely.
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: 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. |
WalkthroughThe precheck feature flag is removed, making prechecks always run. The Run flow in recommend.go is refactored to unconditionally execute precheck, consolidate condition handling (happy/accepted/unhappy), and centralize output formatting. A test case removes manual precheck flag manipulation after options completion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
openshift-ci
bot
added
the
approved
| } | ||
|
|
||
| if o.precheckEnabled { | ||
| conditions, err := o.precheck(ctx) |
There was a problem hiding this comment.
GitHub doesn't seem to have a knob like Git's -w / --ignore-all-space, but here's the diff rendered that way, to show that I'm just dropping that level of indentation:
$ git --no-pager show --oneline -w
cb28d2778 (HEAD -> remove-obsolete-precheckEnabled, wking/remove-obsolete-precheckEnabled) pkg/cli/admin/upgrade/recommend: Drop obsolete precheckEnabled knob
...
@@ -188,7 +185,6 @@ func (o *options) Run(ctx context.Context) error {
issues.Insert(string(configv1.OperatorProgressing))
}
- if o.precheckEnabled {
conditions, err := o.precheck(ctx)
if err != nil {
if !o.quiet {
@@ -232,7 +228,6 @@ func (o *options) Run(ctx context.Context) error {
}
}
}
- }
if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status != configv1.ConditionTrue {
if !o.quiet {
There was a problem hiding this comment.
Check out the "Hide whitespace" here
![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
🧹 Nitpick comments (1)
pkg/cli/admin/upgrade/recommend/recommend.go (1)
211-230: LGTM! Well-structured consolidated output formatting.The refactored output logic is clean and maintainable:
- Single quiet flag check wraps all output
- Consistent formatting across condition categories
- Proper sorting for deterministic output
- Clear separation between happy, accepted, and unhappy conditions
Optional: Consider collecting condition details during the initial loop (lines 198-209) to avoid re-iterating through all conditions at lines 224-228. This would be a minor performance improvement:
var happyConditions []string var acceptedConditions []string var unhappyConditions []string +var unhappyConditionDetails []*metav1.Condition for _, condition := range conditions { if condition.Status == metav1.ConditionTrue { happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason)) } else { issues.Insert(condition.acceptanceName) if accept.Has(condition.acceptanceName) { acceptedConditions = append(acceptedConditions, condition.Type) } else { unhappyConditions = append(unhappyConditions, condition.Type) + unhappyConditionDetails = append(unhappyConditionDetails, &condition) } } }Then use
unhappyConditionDetailsdirectly in the output block instead of looping through all conditions again.
📜 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 (2)
pkg/cli/admin/upgrade/recommend/examples_test.go(0 hunks)pkg/cli/admin/upgrade/recommend/recommend.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/cli/admin/upgrade/recommend/examples_test.go
🔇 Additional comments (2)
pkg/cli/admin/upgrade/recommend/recommend.go (2)
188-194: LGTM! Clean refactoring to make precheck unconditional.The precheck is now always invoked, with appropriate error handling that respects the quiet flag and tracks failures in the issues set.
195-209: No issues with acceptanceName usage
All non-true conditions in alerts.go include an acceptanceName; the insertion logic in recommend.go is correct.
|
/cc |
|
/lgtm Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tchap, wking 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 |
|
/verified by @wking based on no functional changes and green presubmits |
openshift-ci-robot
added
the
verified
|
@wking: 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. |
|
/retest-required |
|
/retest-required |
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Since 8bdac16 (#2088), this knob has always been enabled. Save ourselves a no longer necessary level of indentation by removing it completely.