NO-JIRA: Fix issues collection when CVO handles the risks

[hongkailiu]

Follow up #2349 (comment)

The issues are used only for printing and attached to a specific version whose value is taken from --to. The version was not used when the issues are collected in the case of cvoChecking. This change fixes that.

/hold

I will rebase after #2349 gets in.

Summary by CodeRabbit

• Bug Fixes
  • Upgrade recommendations now evaluate risks only for the explicitly requested version when CVO checking is enabled.
  • Risks are reported only when their conditions apply, improving the accuracy of upgrade issue detection.
  • When no version is specified, unrelated CVO risk issues are no longer added to recommendations.
  • Corrected the displayed list of unaccepted cluster issues by removing SyntheticRiskC.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci-robot]

@hongkailiu: This pull request explicitly references no jira issue.

Details

In response to this:

The issues are used only for printing and attached to a specific version whose value is taken from --to. The version was not used when the issues are collected in the case of cvoChecking. This change fixes that.

/hold

I will rebase after #2349 gets in.

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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: e6189839-4434-46c0-b22e-5b5f7f04d929

📥 Commits

Reviewing files that changed from the base of the PR and between e2cda1c and c4a856c.

📒 Files selected for processing (2)

• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.version-5.0.0-ec.5-output
• pkg/cli/admin/upgrade/recommend/recommend.go

🚧 Files skipped from review as they are similar to previous changes (2)

• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.version-5.0.0-ec.5-output
• pkg/cli/admin/upgrade/recommend/recommend.go

---

Walkthrough

CVO risk detection now evaluates only risks named by the requested conditional update. The recommendation logic adds a risk lookup helper. The related expected output no longer lists SyntheticRiskC.

Changes

CVO risk filtering

Layer / File(s)	Summary
Scope CVO risks to the requested version pkg/cli/admin/upgrade/recommend/recommend.go	When a version is requested, the code matches its conditional update, resolves named risks, and records risks whose Applies condition is not false. Without a requested version, it does not add CVO risk issues.
Update expected risk output pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.version-5.0.0-ec.5-output	The expected unaccepted-issues list removes SyntheticRiskC.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• openshift/oc#2349: Updates the same graph-and-alert-risks expected-output fixture.
• openshift/oc#2355: Extends the same oc adm upgrade recommend --version risk-filtering behavior.

Suggested reviewers: ingvagabund, atiratree

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing issue collection when CVO handles risks.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The PR changes recommend.go and one output fixture only. The patch adds no Ginkgo It, Describe, Context, or When test title and introduces no dynamic test name.
Test Structure And Quality	✅ Passed	The PR changes no test source or Ginkgo code. It updates one expected-output fixture; existing standard Go fixture tests use bounded local execution and diagnostic failures.
Microshift Test Compatibility	✅ Passed	The pull request adds no Ginkgo e2e tests; the only changed file is production Go code, so MicroShift test compatibility is not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The commit changes only recommend.go; it adds no Ginkgo e2e tests or test files, so no SNO multi-node compatibility issue applies.
Topology-Aware Scheduling Compatibility	✅ Passed	The change only updates upgrade-risk collection in a CLI and one example output; it adds no manifests, operators, controllers, or scheduling constraints.
Ote Binary Stdout Contract	✅ Passed	The PR changes recommendation logic and buffered fixture tests only; no OTE process-level setup or stdout writes were added.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds no Ginkgo e2e test. Changes are in recommend.go, and the existing fixture test uses standard Go testing with local mock files, not external or IPv4-only network access.
No-Weak-Crypto	✅ Passed	The PR diff only changes conditional-risk lookup and example output. The changed Go file has no crypto imports, weak algorithms, custom crypto, or secret comparisons.
Container-Privileges	✅ Passed	The PR diff changes only upgrade recommendation logic in Go; it adds no container or Kubernetes manifest privilege settings, host namespaces, SYS_ADMIN capability, or root execution.
No-Sensitive-Data-In-Logs	✅ Passed	The change adds only CVO risk names to issue output; inspected code and fixtures contain identifiers such as SyntheticRiskA and TestAlert, with no passwords, tokens, keys, PII, or customer data.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

pkg/cli/admin/upgrade/recommend/recommend.go (1)

460-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a comment for findRiskByName.

Line 460 adds an unexported function without a descriptive comment. Add a comment that states that the function returns the matching conditional-update risk, or nil.

Proposed fix

+// findRiskByName returns the conditional-update risk with name, or nil.
 func findRiskByName(risks []configv1.ConditionalUpdateRisk, name string) *configv1.ConditionalUpdateRisk {

As per coding guidelines, "Add descriptive comments to all exported and unexported Go types, functions, and methods."

🤖 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 `@pkg/cli/admin/upgrade/recommend/recommend.go` around lines 460 - 467, Add a
descriptive Go comment immediately before findRiskByName stating that it returns
the matching conditional-update risk by name, or nil when no match exists.

Source: Coding guidelines

🤖 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 `@pkg/cli/admin/upgrade/recommend/recommend.go`:
- Around line 460-467: Add a descriptive Go comment immediately before
findRiskByName stating that it returns the matching conditional-update risk by
name, or nil when no match exists.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: c1bbdf65-def3-4c9d-a33a-978e192e45de

📥 Commits

Reviewing files that changed from the base of the PR and between 345ffe9 and 84f9c98.

📒 Files selected for processing (9)

• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-cv.yaml
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-featuregate.yaml
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks-infrastructure.yaml
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.json
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.output
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.show-outdated-releases-output
• pkg/cli/admin/upgrade/recommend/examples/5.0.0-includes-graph-and-alert-risks.version-5.0.0-ec.5-output
• pkg/cli/admin/upgrade/recommend/examples_test.go
• pkg/cli/admin/upgrade/recommend/recommend.go

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[openshift-ci]

@hongkailiu: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[nbottari9]

/lgtm

[openshift-merge-bot]

Scheduling required tests:
/test e2e-agnostic-ovn-cmd
/test e2e-aws-ovn
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-aws-ovn-upgrade

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongkailiu, nbottari9

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/cli/admin/upgrade/OWNERS [hongkailiu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hongkailiu]

/hold cancel

[hongkailiu]

/payload-job periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

[openshift-ci]

@hongkailiu: trigger 3 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

• periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-1of3
• periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-2of3
• periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/44a95850-91d0-11f1-90f5-45624aa54c3d-0