OCPBUGS-61063: pkg/cli/admin/upgrade/recommend: Enable precheck and accept gates #2088
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request references Jira Issue OCPBUGS-61063, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/valid-bug
WalkthroughRemoves feature-gate checks from the upgrade recommend command: always registers --quiet/--accept flags, always enables prechecks in Complete, and always enforces acceptance logic in Run. If any issues remain unaccepted, Run returns an error; if none remain and not quiet, it prints the accepted-issues message. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as upgrade recommend CLI
participant Complete as Complete()
participant Run as Run()
participant Prechecks as Prechecks
participant Issues as Issues Checker
Note over CLI: `--quiet` and `--accept` always registered
User->>CLI: Invoke command
CLI->>Complete: Parse flags, setup
Note right of Complete: o.precheckEnabled = true (always)
Complete-->>CLI: Options ready
CLI->>Run: Execute
Run->>Prechecks: Run prechecks (always)
Prechecks-->>Run: Precheck results
Run->>Issues: Collect/partition issues
Issues-->>Run: accepted, unaccepted sets
alt Unaccepted issues exist
Run-->>User: Error listing unaccepted issues
else No unaccepted issues
opt Not quiet and there are accepted issues
Run-->>User: Print accepted-issues message
end
Run-->>User: Exit success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/admin/upgrade/recommend/recommend.go (1)
437-441: Bug: double-length slice with empty entries when collecting risks.risks := make([]string, len(update.Risks)) followed by append() yields a slice with len=2n, half empty, leading to extra commas in the message.
Apply this diff:
- risks := make([]string, len(update.Risks)) - for _, risk := range update.Risks { - risks = append(risks, risk.Name) - } + risks := make([]string, 0, len(update.Risks)) + for _, risk := range update.Risks { + risks = append(risks, risk.Name) + }
🧹 Nitpick comments (3)
pkg/cli/admin/upgrade/recommend/recommend.go (3)
65-66: Align flag help with behavior; tighten phrasing.The current help for --quiet implies name-only output, which the code doesn’t yet do (see comment on Lines 344-348). Independently, the strings can be clearer.
Apply this diff to improve clarity:
- flags.BoolVar(&o.quiet, "quiet", o.quiet, "When --quiet is true and --version is set, only print unaccepted issue names.") - flags.StringSliceVar(&o.accept, "accept", o.accept, "Comma-delimited names for issues that you find acceptable. With --version, any unaccepted issues will result in a non-zero exit code.") + flags.BoolVar(&o.quiet, "quiet", o.quiet, "Only print unaccepted issue names (requires --version).") + flags.StringSliceVar(&o.accept, "accept", o.accept, "Names for issues you accept. Comma-delimited or repeat --accept. With --version, any unaccepted issues cause a non-zero exit code.")
137-137: The precheckEnabled knob is now vestigial.Since it’s unconditionally set to true and only used as a gate, consider removing the field and the surrounding branch later. I understand the PR notes favor a minimal pivot/backportable change; treat this as follow-up tech debt.
306-311: Redundant nested quiet check.The inner if !o.quiet is unnecessary because it’s already guarded by the outer condition.
Apply this diff:
- if err := injectUpgradeableAsCondition(cv.Status.Desired.Version, c, majorMinorBuckets); err != nil && !o.quiet { - if !o.quiet { - fmt.Fprintf(o.ErrOut, "warning: Cannot inject %s=%s as a conditional update risk: %s\n\nReason: %s\n Message: %s\n\n", c.Type, c.Status, err, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) - } + if err := injectUpgradeableAsCondition(cv.Status.Desired.Version, c, majorMinorBuckets); err != nil && !o.quiet { + fmt.Fprintf(o.ErrOut, "warning: Cannot inject %s=%s as a conditional update risk: %s\n\nReason: %s\n Message: %s\n\n", c.Type, c.Status, err, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/cli/admin/upgrade/recommend/recommend.go(3 hunks)
|
Serial tests are expected to fail until openshift/origin#30113 merges |
To make that functionality generally available. This could be a more thorough overhaul, e.g. I could drop the precheckEnabled knob entirely. But I'm doing the smallest possible pivot now, in case folks want to backport to older 4.y. And I can do the dev-branch polishing later on. The: error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk output that b74a129 (pkg/cli/admin/upgrade/recommend: Don't error on unaccepted issues when the feature gate is off, 2025-08-02, openshift#2069) had removed from the test fixtures is back, now that the accept gate is enabled by default (and thus newly enabled for the test suite).
wking
force-pushed
the
enable-oc-adm-upgrade-recommend-alert-and-accept
branch
from
4060ec8 to
8bdac16
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-recommended.version-4.12.51-output (1)
19-19: Tiny copyedit for brevity.Consider tightening “but which were” → “but were” to keep CLI output concise.
-error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk +error: issues that apply to this cluster but were not included in --accept: AlertNoTestData,ConditionalUpdateRiskpkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-not-recommended.version-4.12.51-output (2)
19-19: Micro-grammar polish (optional).Same brevity tweak as the other file.
-error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk +error: issues that apply to this cluster but were not included in --accept: AlertNoTestData,ConditionalUpdateRisk
19-19: Optional UX hint mirror.Keep examples consistent by adding the same actionable hint here.
error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk +hint: re-run with --accept=AlertNoTestData,ConditionalUpdateRisk to proceed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-not-recommended.version-4.12.51-output(1 hunks)pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-recommended.version-4.12.51-output(1 hunks)pkg/cli/admin/upgrade/recommend/examples/4.16.27-degraded-monitoring.version-4.16.32-output(1 hunks)pkg/cli/admin/upgrade/recommend/recommend.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/upgrade/recommend/examples/4.16.27-degraded-monitoring.version-4.16.32-output
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/admin/upgrade/recommend/recommend.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: wking
PR: openshift/oc#2088
File: pkg/cli/admin/upgrade/recommend/recommend.go:344-348
Timestamp: 2025-09-03T22:29:02.623Z
Learning: In the `oc adm upgrade recommend` command, the --quiet flag behavior intentionally includes descriptive error context rather than just bare issue names, as human consumers benefit from the additional context. Machine consumers are expected to use a future API (referenced in openshift/api#2360) rather than parsing CLI output.
📚 Learning: 2025-09-03T22:29:02.623Z
Learnt from: wking
PR: openshift/oc#2088
File: pkg/cli/admin/upgrade/recommend/recommend.go:344-348
Timestamp: 2025-09-03T22:29:02.623Z
Learning: In the `oc adm upgrade recommend` command, the --quiet flag behavior intentionally includes descriptive error context rather than just bare issue names, as human consumers benefit from the additional context. Machine consumers are expected to use a future API (referenced in openshift/api#2360) rather than parsing CLI output.
Applied to files:
pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-not-recommended.version-4.12.51-outputpkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-recommended.version-4.12.51-output
🔇 Additional comments (3)
pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-recommended.version-4.12.51-output (2)
18-19: Example updated to reflect new acceptance gating — looks good.The added error line accurately mirrors the new behavior (error when issues remain unaccepted) and lists issue keys in stable, alphabetical order.
19-19: Add optional UX hint
Verified that all example outputs’ issue lists are alphabetical and duplicate-free; the suggested hint to re-run with--accept=…can be applied.pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-not-recommended.version-4.12.51-output (1)
18-19: Consistent with enforced acceptance gating.The appended error line correctly signals unaccepted issues for the not-recommended case; ordering and phrasing match the other example.
|
/lgtm
/hold in case those tests are not required, let us give a chance to exercise them. |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, 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 |
|
/cc |
|
@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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
To make that functionality generally available. This could be a more thorough overhaul, e.g. I could drop the
precheckEnabledknob entirely. But I'm doing the smallest possible pivot now, in case folks want to backport to older 4.y. And I can do the dev-branch polishing later on.Summary by CodeRabbit