-
Notifications
You must be signed in to change notification settings - Fork 22
add ssh output to check eval test failure #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eliajahshan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe changes enhance diagnostic capabilities in cluster creation and validation test scripts. They add detailed diagnostic output when cluster configuration mismatches are detected and when cluster creation times out, while expanding validation parameters for Single Node OpenStack test verification. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 @eliajahshan. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/scripts/common.sh (1)
166-173: LGTM! Enhanced timeout diagnostics are very helpful.The debug block that lists matching clusters will significantly improve troubleshooting when cluster creation times out. The
|| trueensures the script doesn't fail if the debug query encounters issues, which is appropriate.Minor style consistency suggestions
For consistency with the rest of the file:
- Line 170 uses
curl -fsSwhile lines 36 and 45 usecurl -sSf(same flags, different order)- Lines 168-169 could follow the inline style used on lines 35 and 44:
- local service_url - service_url=$(get_assisted_service_url) + local service_url=$(get_assisted_service_url)These are purely stylistic and don't affect functionality.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/scripts/common.shtest/scripts/verify_create_eval_test_sno.sh
🔇 Additional comments (3)
test/scripts/common.sh (1)
147-150: LGTM! Helpful diagnostic additions.These additional diagnostic fields (cluster ID, status, high_availability_mode, platform.type) will significantly aid troubleshooting when configuration mismatches occur.
test/scripts/verify_create_eval_test_sno.sh (2)
17-17: LGTM! Clear diagnostic message.This informational message helps users understand what's being validated and sets expectations about failure output.
19-26: LGTM! SSH key validation properly integrated.The function call correctly passes all required parameters including the SSH key for validation. This aligns with the PR objective to "add ssh output to check eval test failure."
The hardcoded SSH key on line 15 appears to be a test fixture (based on the example@example.com domain), which is appropriate for a validation test script.
Draft
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.