fix: add curl fallback in deploy-k8s-metrics-server.sh for macOS comp…#7274
fix: add curl fallback in deploy-k8s-metrics-server.sh for macOS comp…#7274Krishiv-Mahajan wants to merge 3 commits intokarmada-io:masterfrom
Conversation
…atibility Signed-off-by: Krishiv-Mahajan <mahajankrishiv10@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the metrics-server deployment script by addressing a compatibility issue on macOS. By introducing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where the hack/deploy-k8s-metrics-server.sh script would fail on macOS due to wget not being available by default. The addition of curl as a fallback is a good solution that improves the script's portability. I have one suggestion to further enhance maintainability by extracting the repeated URL into a variable.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7274 +/- ##
==========================================
+ Coverage 42.00% 42.02% +0.01%
==========================================
Files 874 874
Lines 53539 53551 +12
==========================================
+ Hits 22490 22504 +14
+ Misses 29364 29355 -9
- Partials 1685 1692 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Krishiv-Mahajan <mahajankrishiv10@gmail.com>
0dd42fa to
cdc40ee
Compare
There was a problem hiding this comment.
Pull request overview
Fixes macOS Quick Start failure by avoiding a hard dependency on wget in hack/deploy-k8s-metrics-server.sh when downloading the metrics-server manifest.
Changes:
- Introduces a
METRICS_SERVER_URLvariable for the metrics-server manifest. - Uses
curlwhen available, otherwise falls back towget, and errors if neither is present.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: Krishiv-Mahajan <mahajankrishiv10@gmail.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
hack/deploy-k8s-metrics-server.shused wget to download the metrics-server manifest, but wget is not available on macOS by default. This caused the Quick Start script to fail immediately on macOS with zsh: command not found: wget. This PR adds curl as a fallback since curl is available on macOS, Linux, and Windows by default.hack/install-cli.shalready follows this pattern of preferring curl.Which issue(s) this PR fixes:
Fixes #7273
Part of #7269
Special notes for your reviewer:
curlandwgetfetch the exact same file from the same URL. The only change is the tool used and the flag syntax (-O for wget → -o for curl). Behavior is identical on Linux where wget is typically pre-installed.Does this PR introduce a user-facing change?:
release
hack/deploy-k8s-metrics-server.sh: Fixed Quick Start failure on macOS caused by missingwgetby addingcurlas a fallback.