OCPBUGS-81511: Remove unscoped CSV watch from ClusterNotUpgradeableAlert - #16904
OCPBUGS-81511: Remove unscoped CSV watch from ClusterNotUpgradeableAlert#16904rsacherer wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@rsacherer: This pull request references Jira Issue OCPBUGS-81511, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe cluster settings alert no longer checks resource upgradeability. It always displays ClusterOperator and installed Operator links and uses ChangesCluster settings alert
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Hi @rsacherer. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/public/components/cluster-settings/cluster-settings.tsx (1)
1131-1143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the link behavior.
Render
ClusterNotUpgradeableAlertwithUpgradeable=Falseand verify both link labels. Also verify that the installed Operator link uses the all-namespaces path generated fromClusterServiceVersionModel. This protects both user-visible changes from regression.🤖 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 `@frontend/public/components/cluster-settings/cluster-settings.tsx` around lines 1131 - 1143, Add a regression test for ClusterNotUpgradeableAlert with Upgradeable set to False, asserting both “View ClusterOperators” and “View installed Operators” labels and verifying the installed Operators link uses the all-namespaces path generated by resourceListPathFromModel(ClusterServiceVersionModel).
🤖 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 `@frontend/public/components/cluster-settings/cluster-settings.tsx`:
- Around line 1131-1143: Add a regression test for ClusterNotUpgradeableAlert
with Upgradeable set to False, asserting both “View ClusterOperators” and “View
installed Operators” labels and verifying the installed Operators link uses the
all-namespaces path generated by
resourceListPathFromModel(ClusterServiceVersionModel).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a051259a-6ca4-4bc2-a0a2-a24a3832e5b1
📒 Files selected for processing (1)
frontend/public/components/cluster-settings/cluster-settings.tsx
|
/ok-to-test |
|
Scheduling tests matching the |
|
/hold valid frontend test failure |
The ClusterNotUpgradeableAlert component fetched all ClusterServiceVersions cluster-wide (resulting in up to 600MB object sizes in the browser on clusters with 350+ namespaces and lot's of operators) on every visit to Cluster Settings when Upgradeable=False was set. The only use of CSVs was to decide whether or not to show navigation links. For installed operators this was ineffective: CSV status.conditions use phase/reason fields, not type/status, so getConditionUpgradeableFalse() always returned undefined on CSVs — meaning notUpgradeableCSVsPresent was permanently false and the 'View installed Operators' link was never shown. Additionally the link URL used /k8s/ns/all-namespaces/ instead of /k8s/all-namespaces/, causing 'No Operators found' on navigation. Fix: remove both the ClusterOperator and CSV watches. Both navigation links are now always shown when the alert renders — correct since the alert only mounts when Upgradeable=False is already confirmed on ClusterVersion. Use resourceListPathFromModel() for the correct all-namespaces URL. https://issues.redhat.com/browse/OCPBUGS-81511
b578bbd to
46125c6
Compare
|
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. |
|
Fixed the unused export, and included a test-case to check that the alert is printed with both URLs. |
|
/retest-required |
|
/unhold |
|
/jira refresh |
|
@logonoff: This pull request references Jira Issue OCPBUGS-81511, which is valid. 3 validation(s) were run on this bug
DetailsIn 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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rsacherer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
@rsacherer: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
PR needs rebase. DetailsInstructions 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. |
The ClusterNotUpgradeableAlert component fetched all ClusterServiceVersions cluster-wide (resulting in up to 600MB object sizes in the browser on clusters with 350+ namespaces and lot's of operators) on every visit to Cluster Settings when Upgradeable=False was set.
The only use of CSVs was to decide whether or not to show navigation links.
For installed operators this was ineffective: CSV status.conditions use phase/reason fields, not type/status, so getConditionUpgradeableFalse() always returned undefined on CSVs — meaning notUpgradeableCSVsPresent was permanently false and the 'View installed Operators' link was never shown.
Additionally the link URL used /k8s/ns/all-namespaces/ instead of /k8s/all-namespaces/, causing 'No Operators found' on navigation.
Fix: remove both the ClusterOperator and CSV watches. Both navigation links are now always shown when the alert renders — correct since the alert only mounts when Upgradeable=False is already confirmed on ClusterVersion. Use resourceListPathFromModel() for the correct all-namespaces URL.
https://issues.redhat.com/browse/OCPBUGS-81511
Analysis / Root cause:
The ClusterNotUpgradeableAlert component fetched all ClusterServiceVersions
cluster-wide (resulting in up to 600MB object sizes in the browser on clusters
with 350+ namespaces and lots of operators) on every visit to Cluster Settings
when Upgradeable=False was set.
The only use of CSVs was to decide whether or not to show navigation links.
For installed operators this was ineffective: CSV status.conditions use
phase/reason fields, not type/status, so getConditionUpgradeableFalse() always
returned undefined on CSVs — meaning notUpgradeableCSVsPresent was permanently
false and the 'View installed Operators' link was never shown.
Additionally the link URL used /k8s/ns/all-namespaces/ instead of
/k8s/all-namespaces/, causing 'No Operators found' on navigation.
Solution description:
Remove both the ClusterOperator and CSV watches. Both navigation links are now
always shown when the alert renders — correct since the alert only mounts when
Upgradeable=False is already confirmed on ClusterVersion. Use
resourceListPathFromModel() for the correct all-namespaces URL.
https://issues.redhat.com/browse/OCPBUGS-81511
Test setup:
oc apply -f - <<EOF
apiVersion: config.openshift.io/v1
kind: ClusterOperator
metadata:
name: test-upgrade-blocker
spec: {}
EOF
oc patch co test-upgrade-blocker --subresource=status --type=merge -p '{"status":{"conditions":[{"type":"Upgradeable","status":"False","reason":"Test","message":"Test block","lastTransitionTime":"2026-08-04T10:00:00Z"},{"type":"Available","
status":"True","lastTransitionTime":"2026-08-04T10:00:00Z"},{"type":"Degraded","status":"False","lastTransitionTime":"2026-08-04T10:00:00Z"},{"type":"Progressing","status":"False","lastTransitionTime":"2026-08-04T10:00:00Z"}]}}'
Test cases:
Browser conformance:
Summary by CodeRabbit