Fix console plugin failing when hosted clusters are defined without SSH keys - #6636
Conversation
openshift-ci
Bot
added
do-not-merge/work-in-progress
dco-signoff: yes
needs-ok-to-test
labels
|
Hi @dror1212. Thanks for your PR. I'm waiting for a stolostron 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. |
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesHostedCluster secret handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
dror1212
changed the title
dror1212
changed the title
|
/ok-to-test |
|
/retest |
|
The MCE enterprise-contract checks are still failing after /retest, but I don’t have permission to view the detailed Konflux logs. Could someone with access take a look and let me know if I need to change anything in this PR? |
|
Hi @dror1212 we have opened a konflux issue regarding the failure https://redhat.atlassian.net/browse/KFLUXSPRT-8665. @KevinFCormier do you think we can manually merge this one? |
Signed-off-by: dror1212 <drortal12@gmail.com>
Signed-off-by: dror1212 <drortal12@gmail.com>
98c3271 to
5676fdf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@frontend/src/resources/hosted-cluster.ts`:
- Line 1: Add the required Open Cluster Management copyright comment as the
first line of hosted-cluster.ts, before the import of IResource and
IResourceDefinition.
- Around line 1-3: Update the imports in hosted-cluster.ts to use type-only
imports for IResource, IResourceDefinition, and Cluster, since they are only
referenced as types; preserve their existing module sources and usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 77898ef3-a837-4d02-855e-2592eb48feac
📒 Files selected for processing (2)
frontend/src/resources/hosted-cluster.tsfrontend/src/resources/utils/get-cluster.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/resources/utils/get-cluster.ts
| @@ -1,8 +1,9 @@ | |||
| import { IResource, IResourceDefinition } from './resource' | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required source header.
Line 1 starts with an import. Add /* Copyright Contributors to the Open Cluster Management project */ as the first line.
🤖 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/src/resources/hosted-cluster.ts` at line 1, Add the required Open
Cluster Management copyright comment as the first line of hosted-cluster.ts,
before the import of IResource and IResourceDefinition.
Source: Coding guidelines
| import { IResource, IResourceDefinition } from './resource' | ||
|
|
||
| import { Cluster } from './utils/get-cluster' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file existence =="
git ls-files | grep -E '(^|/)frontend/src/resources/hosted-cluster\.ts$' || true
if [ -f frontend/src/resources/hosted-cluster.ts ]; then
echo "== first 80 lines =="
cat -n frontend/src/resources/hosted-cluster.ts | sed -n '1,80p'
echo "== import/type mentions in hosted-cluster.ts =="
rg -n "^[[:space:]]*import |import type|IResource|IResourceDefinition|Cluster" frontend/src/resources/hosted-cluster.ts || true
echo "== helper exports at candidate files =="
for f in frontend/src/resources/resource.ts frontend/src/resources/utils/get-cluster.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
grep -n -E "^(export (interface|type|const|class)|export {)" "$f" | sed -n '1,120p'
fi
done
fi
echo "== deterministic check: runtime export usages of the imported symbols in hosted-cluster.ts =="
python3 - <<'PY'
from pathlib import Path
p = Path('frontend/src/resources/hosted-cluster.ts')
if not p.exists():
print('hosted-cluster.ts missing')
raise SystemExit(0)
text=p.read_text()
for symbol in ['IResource', 'IResourceDefinition', 'Cluster']:
# occurrences outside comment/title-ish first line
occurrences=[]
for i,line in enumerate(text.splitlines(),1):
if line.strip().startswith('/* Copyright'):
continue
if symbol in line:
occurrences.append((i,line.strip()))
import_re=re=re import re
for i,line in occurrences:
print(f'{symbol}@{i}: {line}')
PYRepository: stolostron/console
Length of output: 5867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== referenced file sizes =="
wc -l frontend/src/resources/utils/get-cluster.ts frontend/src/resources/hosted-cluster.ts frontend/src/resources/resource.ts
echo "== get-cluster export/type context =="
sed -n '230,255p;330,370p' frontend/src/resources/utils/get-cluster.ts | cat -n
echo "== hosted-cluster imports and type usages =="
python3 - <<'PY'
from pathlib import Path
text = Path('frontend/src/resources/hosted-cluster.ts').read_text()
for line_no, line in enumerate(text.splitlines(), 1):
for symbol in ['IResource', 'IResourceDefinition', 'Cluster']:
if symbol in line and not line.strip().startswith('/* Copyright'):
print(f"{symbol}`@line`{line_no}: {line.strip()}")
PYRepository: stolostron/console
Length of output: 4235
Use type-only imports for type symbols.
IResource, IResourceDefinition, and Cluster are only used for types in this file, so import them with import type to keep runtime imports aligned with the guideline.
Proposed change
-import { IResource, IResourceDefinition } from './resource'
+import type { IResource, IResourceDefinition } from './resource'
-import { Cluster } from './utils/get-cluster'
+import type { Cluster } from './utils/get-cluster'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { IResource, IResourceDefinition } from './resource' | |
| import { Cluster } from './utils/get-cluster' | |
| import type { IResource, IResourceDefinition } from './resource' | |
| import type { Cluster } from './utils/get-cluster' |
🤖 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/src/resources/hosted-cluster.ts` around lines 1 - 3, Update the
imports in hosted-cluster.ts to use type-only imports for IResource,
IResourceDefinition, and Cluster, since they are only referenced as types;
preserve their existing module sources and usage.
Source: Coding guidelines
Signed-off-by: Kevin Cormier <kcormier@redhat.com>
5676fdf to
757dca1
Compare
@fxiang1 I'm not seeing this issue on the post-merge builds, nor on other PRs. I agree that it is probably a false-positive, but if we merge this code, we might start failing the EC check on post-merge builds. I have tweaked the code a little bit and rebased to see if that will change the build output enough to avoid this problem. |
@KevinFCormier Thanks! I saw this on Kike's PR #6591 as well so I thought it was a common issue. |
|
/ok-to-test |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dror1212, KevinFCormier 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 |
|
|
/cherry-pick release-2.17 |
|
/cherry-pick release-2.16 |
|
/cherry-pick release-2.15 |
|
@KevinFCormier: new pull request created: #6674 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 kubernetes-sigs/prow repository. |
|
@KevinFCormier: new pull request created: #6675 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 kubernetes-sigs/prow repository. |
|
@KevinFCormier: new pull request created: #6676 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 kubernetes-sigs/prow repository. |
4 participants
📝 Summary
Ticket Summary (Title):
Handle HostedClusters without an SSH key
Ticket Link:
N/A — this fix does not have an ACM Jira ticket, and GitHub Issues are disabled for this repository.
Type of Change:
✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
HyperShift defines
HostedCluster.spec.sshKeyas optional. The cluster resource mapping guardedspec, but accessedsshKey.namewithout guarding the optionalsshKey. As a result, listing a valid HostedCluster without an SSH key threwCannot read properties of undefined (reading 'name')and broke the managed clusters page.This change safely accesses the optional SSH key reference and updates the console-owned HostedCluster interface to reflect the HyperShift API contract. A regression test verifies that a HostedCluster without an SSH key is mapped successfully.
Validation:
npm run check— passednpm test— 532 frontend suites and all backend tests passed; one unrelated Policy wizard test failed from a missing parallel HTTP mocknpm test -- --runInBand src/wizards/Governance/Policy/policyWizard.test.tsx— passed (9/9) when rerun in isolationnpm test -- --runInBand src/resources/utils/get-cluster.test.ts— passed, including the new regression caseSummary by CodeRabbit