fix: Fix setup script for minikube #2136
Conversation
atchernych
requested review from
biswapanda,
hhzhang16,
hutm,
ishandhanani,
julienmancuso,
mohammedabdulwahhab and
nnshah1
as code owners
WalkthroughThe changes refactor a shell script for improved host discovery logic in Istio VirtualServices, separating JSON retrieval and processing steps for clarity and error handling. Additionally, a documentation update aligns Docker image push commands with variable-based tagging for consistency in the deployment guide. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
WalkthroughThe updates include a refactor of the Istio VirtualService host extraction logic in a shell script to improve clarity and robustness, and a documentation correction to ensure Docker images are pushed using the correct tag variable. No exported or public entities were altered. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/cloud/helm/network-config-wizard.sh (1)
103-109: Usereadarrayto avoid word-splitting warning (SC2207)
while readin a here-string works, butreadarray -t HOST_PATTERNS <<<"$HOST_PATTERNS_RAW"is shorter, avoids a subshell and silences shellcheck.Low priority, but worth considering.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/cloud/helm/network-config-wizard.sh(1 hunks)docs/guides/dynamo_deploy/quickstart.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
deploy/cloud/helm/network-config-wizard.sh (1)
Learnt from: julienmancuso
PR: #2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
🪛 Shellcheck (0.10.0)
deploy/cloud/helm/network-config-wizard.sh
[warning] 109-109: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
docs/guides/dynamo_deploy/quickstart.md (1)
90-96: Consistency fix looks goodSwapping the hard-coded tag for
$IMAGE_TAGkeeps thetag/pushsteps in sync and prevents accidental pushes under the wrong name. Nice catch.
WalkthroughThe changes update a shell script to improve the robustness and clarity of extracting host patterns from Istio VirtualServices, adding explicit handling for empty results. Additionally, the documentation is updated to use a variable for the Docker image tag in the push command, ensuring consistency with previous tagging steps. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/cloud/helm/network-config-wizard.sh (1)
103-110: Usereadarray -tinstead of command substitution to avoid word-splitting (SC2207)ShellCheck warns because command substitution splits on IFS. Using
readarraypreserves each host as one element and avoids subshell overhead.- HOST_PATTERNS=($(printf "%s\n" "${HOST_PATTERNS[@]}" | sort | uniq)) + readarray -t HOST_PATTERNS < <(printf '%s\n' "${HOST_PATTERNS[@]}" | sort -u)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/cloud/helm/network-config-wizard.sh(1 hunks)docs/guides/dynamo_deploy/quickstart.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
deploy/cloud/helm/network-config-wizard.sh (1)
Learnt from: julienmancuso
PR: #2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
🪛 Shellcheck (0.10.0)
deploy/cloud/helm/network-config-wizard.sh
[warning] 109-109: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
docs/guides/dynamo_deploy/quickstart.md (1)
90-96: Push command now consistent – double–check registry variable usageGood catch switching to
$IMAGE_TAG; tagging (line 93) and pushing (line 95) are now aligned.
Just ensure<your-registry>is the exact same value you used forDOCKER_SERVERearlier to avoid pushing to an unexpected repo.
No further action required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/cloud/helm/network-config-wizard.sh (1)
99-103: Guard against “no resources found” & jq parse errors
kubectl get virtualservice …prints “No resources found” (non-JSON) and exits with0when no VirtualServices exist.
Feeding that string intojqstill triggers a parse error (stderr suppressed, but noisy ifset -o pipefailis ever enabled).Add a fallback to valid empty JSON so the pipeline remains silent & future-proof:
-VIRTUALSVC_JSON=$(kubectl get virtualservice --all-namespaces -o json 2>/dev/null) +VIRTUALSVC_JSON="$(kubectl get virtualservice --all-namespaces -o json 2>/dev/null || echo '{}')"This also sidesteps the need for
2>/dev/nullafterjqbecause the input is always valid JSON.
🧹 Nitpick comments (1)
deploy/cloud/helm/network-config-wizard.sh (1)
104-112: Usemapfilefor safer array construction & silence SC2207The current
while read …; HOST_PATTERNS+=()loop works but:
- Triggers SC2207 (“prefer mapfile or read -a to split command output”).
- Re-sorts/uniqs in a subshell only to re-split into words, losing hostnames that contain spaces (unlikely but possible).
A compact, shellcheck-friendly alternative:
-if [[ -n "$HOST_PATTERNS_RAW" ]]; then - while read -r host; do - suffix=$(extract_domain_suffix "$host") - HOST_PATTERNS+=("$suffix") - done <<< "$HOST_PATTERNS_RAW" - - HOST_PATTERNS=($(printf "%s\n" "${HOST_PATTERNS[@]}" | sort | uniq)) +if [[ -n "$HOST_PATTERNS_RAW" ]]; then + # Deduplicate, sort and load into an array in one go + mapfile -t HOST_PATTERNS < <( + printf '%s\n' "$HOST_PATTERNS_RAW" | + while read -r host; do + extract_domain_suffix "$host" + done | sort -u + )Results are identical, but the code is clearer and static-analysis-clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/cloud/helm/network-config-wizard.sh(1 hunks)docs/guides/dynamo_deploy/quickstart.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
deploy/cloud/helm/network-config-wizard.sh (1)
Learnt from: julienmancuso
PR: #2012
File: deploy/cloud/helm/crds/templates/nvidia.com_dynamocomponentdeployments.yaml:92-98
Timestamp: 2025-07-18T16:04:31.771Z
Learning: CRD schemas in files like deploy/cloud/helm/crds/templates/*.yaml are auto-generated from Kubernetes library upgrades and should not be manually modified as changes would be overwritten during regeneration.
🪛 Shellcheck (0.10.0)
deploy/cloud/helm/network-config-wizard.sh
[warning] 111-111: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (1)
docs/guides/dynamo_deploy/quickstart.md (1)
90-96: Consistent image-tag propagation – looks goodThe push command now uses
$IMAGE_TAG, matching the earlierdocker tagstep, so users no longer have to edit two places.
Nice catch.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
Overview:
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Documentation
Refactor