fix(alerts): Remove duplicate information in the recommend command

[nbottari9]

Context

---

If the ClusterUpdateAcceptRisks feature gate is enabled on the cluster, the CVO will include alerts in the conditional update risks. The client also checks for alerts, which can cause warnings or info messages to be printed twice.

Description

---

We can detect if the server (CVO) is already including alerts by checking if Recommended=False in the ClusterVersion conditional updates. If this is set, we know that the feature gate is enabled, therefore the server is checking alerts and we don't need to on the client.

Changes

---

• Added an if statement before calling precheck() to check if the server is including alerts
• shouldSkipClientAlertChecking() - loops over the ConditionalUpdates and checks to see if any of them have Recommended=False. If so, return true to indicate the server IS checking for alerts and we don't need to on the client.
• TestServerSideAlertRisks test suite - Added new tests to test new functionality

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • The upgrade recommendation command now avoids displaying duplicate alerts when the server has already flagged risks against updates.

• Tests

  • Added test coverage for server-side alert risk detection scenarios.

[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: 536b7013-0030-4e44-a7b9-82bd333e6d2a

📥 Commits

Reviewing files that changed from the base of the PR and between 9557cf3 and fb98845.

📒 Files selected for processing (2)

• pkg/cli/admin/upgrade/recommend/recommend.go
• pkg/cli/admin/upgrade/recommend/recommend_test.go

---

Walkthrough

The upgrade recommendation command now conditionally skips client-side alert checks when the server has already indicated risk via a False Recommended condition on ConditionalUpdates. A new helper detects this server-side signal to prevent duplicate alert output.

Changes

Conditional client-side alert checking for upgrade recommendations

Layer / File(s)	Summary
Skip guard helper and Run() integration pkg/cli/admin/upgrade/recommend/recommend.go	New shouldSkipClientAlertChecking(cv) helper checks for a Recommended condition set to False in ConditionalUpdates; the Run method wraps its precheck/condition classification and output block to skip entirely when the server is already recommending against updates.
Skip guard test coverage pkg/cli/admin/upgrade/recommend/recommend_test.go	TestServerSideAlertRisks validates the skip helper across table-driven scenarios including nil/empty ConditionalUpdates, absent Recommended conditions, and both True and False Recommended statuses.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.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 accurately describes the main change: preventing duplicate alert information in the recommend command by skipping client-side checks when the server is already handling them.
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	No Ginkgo tests found in PR. Uses standard Go testing with stable, deterministic test names containing no dynamic values.
Test Structure And Quality	✅ Passed	PR contains standard Go unit tests, not Ginkgo tests. Custom check targets Ginkgo patterns (It blocks, BeforeEach/AfterEach) which don't apply to testing.T table-driven tests.
Microshift Test Compatibility	✅ Passed	The new test TestServerSideAlertRisks is a standard Go unit test (not Ginkgo e2e). Custom check scope is limited to Ginkgo e2e tests, so not applicable.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No Ginkgo e2e tests were added in this PR. The new test (TestServerSideAlertRisks) is a standard Go unit test that does not make multi-node cluster assumptions and is not subject to this check.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies CLI code (oc adm upgrade recommend) with no deployment manifests, operator code, controllers, or scheduling constraints; not applicable to topology-aware scheduling check.
Ote Binary Stdout Contract	✅ Passed	New code adds a helper function with no I/O, wraps existing output code conditionally, uses IOStreams for all output, and includes standard Go unit tests with no stdout writes.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	Tests added are not Ginkgo e2e tests; they are standard Go unit tests with no IPv4 assumptions or external connectivity requirements. Check does not apply to this PR.
No-Weak-Crypto	✅ Passed	No weak cryptography detected. PR adds conditional logic to skip client-side alert checking; no crypto algorithms, custom implementations, or unsafe secret comparisons are used.
Container-Privileges	✅ Passed	PR modifies Go source code files for CLI tool only. No container/Kubernetes manifests with privileged settings found. Example YAML files are ClusterVersion resources, not container configs.
No-Sensitive-Data-In-Logs	✅ Passed	PR adds a guard to skip client-side alert checks and a test. No new logging introduced and no sensitive data exposed in logs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nbottari9
Once this PR has been reviewed and has the lgtm label, please assign hongkailiu for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• pkg/cli/admin/upgrade/OWNERS

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

[openshift-ci]

@nbottari9: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-serial-2of2	fb98845	link	true	/test e2e-aws-ovn-serial-2of2
ci/prow/unit	fb98845	link	true	/test unit
ci/prow/e2e-aws-ovn-serial-1of2	fb98845	link	true	/test e2e-aws-ovn-serial-1of2

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.

[wking]

There could be reasons for an update to be Recommended=False that aren't about the alert checks. And we want to be exhaustive in checking, because a cluster-admin should be able to say "oh, I'm ok with those issues" and be able to confidently update anyway. Maybe we can look in conditionalUpdateRisks for risk names that start with Alert? Or maybe the presence of conditionalUpdateRisks at all is enough to mark the relevant feature as enabled, and we don't need to build heuristics based on risk name? Either of those might double-check a cluster where the CVO checked and thought the cluster was clean, and then the client re-checked (and hopefully agreed the cluster was clean), but that seems ok during the transitional period, before we can drop the client-side checking entirely.