Merged
Conversation
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/571/index.html |
ecolternv
force-pushed
the
ecolter/fix-deploy-scripts-aws
branch
from
9776e78 to
776aa32
Compare
ecolternv
force-pushed
the
ecolter/fix-deploy-scripts-aws
branch
from
3d68146 to
058d908
Compare
📝 WalkthroughWalkthroughAdds optional NGC API key support across deployment scripts: new Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as "deploy-k8s.sh"
participant Helm as "Helm / Chart Repo"
participant K8s as "Kubernetes API"
User->>Script: run with --ngc-api-key <KEY>
Script->>Script: parse args, set NGC_API_KEY
alt NGC_API_KEY present
Script->>K8s: create `docker-registry` secret `nvcr-secret` in namespaces (3)
K8s-->>Script: secret created/updated
Script->>Helm: helm repo add <repo> user="$oauthtoken" password=<NGC_API_KEY>
Helm-->>Script: repo authenticated
Script->>Script: inject imagePullSecret into Helm values
end
Script->>Helm: helm upgrade --install (with values referencing imagePullSecret)
Helm->>K8s: apply manifests
K8s-->>Script: deployment complete
Note over Script,K8s: On cleanup -> delete `nvcr-secret` from namespaces
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
deployments/scripts/deploy-k8s.sh (1)
336-341: Consider usinghelm repo updateafter add to refresh credentials.If the helm repo was previously added without credentials,
helm repo addwith|| truewill silently skip updating the authentication. Thehelm repo updateon line 342 fetches new charts but doesn't update repo credentials.💡 Alternative: force repo update with credentials
if [[ -n "$NGC_API_KEY" ]]; then - helm repo add osmo https://helm.ngc.nvidia.com/nvidia/osmo \ - --username='$oauthtoken' --password="$NGC_API_KEY" || true + helm repo add osmo https://helm.ngc.nvidia.com/nvidia/osmo \ + --username='$oauthtoken' --password="$NGC_API_KEY" --force-update || true else helm repo add osmo https://helm.ngc.nvidia.com/nvidia/osmo || true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/scripts/deploy-k8s.sh` around lines 336 - 341, The helm repo add call for "osmo" can leave old unauthenticated credentials in place due to the "|| true" and a later generic "helm repo update"; update the deploy-k8s.sh logic so that when NGC_API_KEY is set you either (a) call helm repo add osmo ... --username='$oauthtoken' --password="$NGC_API_KEY" --force-update to immediately replace credentials, or (b) run a targeted helm repo update immediately after the add to refresh auth for the "osmo" repo; adjust the branch that checks NGC_API_KEY and the subsequent helm repo update invocation (references: NGC_API_KEY, helm repo add osmo, helm repo update) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/scripts/deploy-k8s.sh`:
- Around line 309-314: Replace the raw kubectl invocation in the secret-creation
block with the provider wrapper RUN_KUBECTL to ensure provider-specific
execution (reference: NG C_SECRET_NAME variable and the kubectl create secret
docker-registry ... --dry-run=client -o yaml | kubectl apply -f - pipeline). Use
RUN_KUBECTL for both the create (--dry-run=client -o yaml) and the apply steps;
if the provider wrapper does not support piping, write the create output to a
temporary YAML file and then call RUN_KUBECTL apply -f <tempfile>, cleaning up
the temp file afterward so the behavior matches other uses of RUN_KUBECTL in the
script.
In `@deployments/scripts/deploy-osmo-minimal.sh`:
- Around line 386-397: The branch that runs when has_all_required is true still
calls azure_configure_interactively/aws_configure_interactively which can prompt
interactively; change the logic so when has_all_required is true you skip the
interactive calls and only invoke the provider-specific generate functions
(azure_generate_tfvars or aws_generate_tfvars) based on PROVIDER, ensuring flags
win and no interactive prompts are triggered.
---
Nitpick comments:
In `@deployments/scripts/deploy-k8s.sh`:
- Around line 336-341: The helm repo add call for "osmo" can leave old
unauthenticated credentials in place due to the "|| true" and a later generic
"helm repo update"; update the deploy-k8s.sh logic so that when NGC_API_KEY is
set you either (a) call helm repo add osmo ... --username='$oauthtoken'
--password="$NGC_API_KEY" --force-update to immediately replace credentials, or
(b) run a targeted helm repo update immediately after the add to refresh auth
for the "osmo" repo; adjust the branch that checks NGC_API_KEY and the
subsequent helm repo update invocation (references: NGC_API_KEY, helm repo add
osmo, helm repo update) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb071e63-382a-47b4-bd29-10c40a3f92f0
📒 Files selected for processing (4)
deployments/scripts/README.mddeployments/scripts/aws/terraform.shdeployments/scripts/deploy-k8s.shdeployments/scripts/deploy-osmo-minimal.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/scripts/deploy-osmo-minimal.sh`:
- Around line 369-384: The script errors under set -u because
TF_POSTGRES_PASSWORD and TF_REDIS_PASSWORD may be unset when flags aren’t
provided; initialize them to empty defaults near the other globals (before they
are referenced) so checks in the has_all_required logic and the case on PROVIDER
won’t trigger unbound variable errors—add default assignments for
TF_POSTGRES_PASSWORD and TF_REDIS_PASSWORD alongside the other global vars (so
later references in the has_all_required logic, case "$PROVIDER", and any usage
in functions are safe).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 760ac743-7b32-456f-b356-4ad0554e81a0
📒 Files selected for processing (2)
deployments/scripts/deploy-k8s.shdeployments/scripts/deploy-osmo-minimal.sh
RyaliNvidia
approved these changes
Mar 11, 2026
fernandol-nvidia
approved these changes
Mar 11, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployments/scripts/deploy-osmo-minimal.sh (1)
371-423:⚠️ Potential issue | 🟠 MajorFlag overrides are still dropped when
terraform.tfvarsalready exists.This branch only regenerates
terraform.tfvarswhen the secrets were passed on the current invocation. If the passwords live in the existing file and the user only changes--aws-region,--cluster-name,--environment, etc., the code falls intoUsing existing terraform.tfvarsand the new CLI values never reach Terraform. Backfill the missing secrets from the existing file before evaluatinghas_all_required, then regenerate whenever anyTF_*override is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/scripts/deploy-osmo-minimal.sh` around lines 371 - 423, The current logic only checks has_all_required based on environment/flag vars and skips regenerating terraform.tfvars when an existing tfvars file is present, dropping other flag overrides; fix by first loading/backfilling TF_POSTGRES_PASSWORD and TF_REDIS_PASSWORD from the existing tfvars_file (using the same grep logic already used) before computing has_all_required, then change the regeneration condition to regenerate (call azure_generate_tfvars or aws_generate_tfvars) whenever any TF_* override is present (e.g., TF_POSTGRES_PASSWORD or TF_REDIS_PASSWORD or other TF_* flags) or when has_all_required is true so that CLI flag overrides (region, cluster name, environment) are never ignored for the PROVIDER-specific generators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/scripts/deploy-osmo-minimal.sh`:
- Around line 206-207: The built-in usage text in show_help does not mention the
new --redis-password flag, so update the help/usage output (the show_help
function or the help string printed by the script) to include the
--redis-password option and note that it sets TF_REDIS_PASSWORD; ensure the flag
description mirrors the existing --postgres-password entry format and includes
any expected value format or security note so users can discover and set
TF_REDIS_PASSWORD from the script's usage text.
- Around line 559-563: The DRY_RUN early-exit unconditionally prevents later
Kubernetes dry-run logic (including deploy_osmo and the call to deploy-k8s.sh)
from running; update the if [[ "$DRY_RUN" == true ]] block so it only exits when
Terraform was actually executed (e.g., when --skip-terraform is false or a
Terraform plan/run path was taken), or move this exit into the
Terraform-specific block so that DRY_RUN still allows the subsequent
deploy_osmo/deploy-k8s.sh dry-run path to run. Ensure you reference and check
the same flag/condition used when invoking Terraform (the --skip-terraform path)
before calling exit 0 so Kubernetes dry-run logic is reached when appropriate.
---
Outside diff comments:
In `@deployments/scripts/deploy-osmo-minimal.sh`:
- Around line 371-423: The current logic only checks has_all_required based on
environment/flag vars and skips regenerating terraform.tfvars when an existing
tfvars file is present, dropping other flag overrides; fix by first
loading/backfilling TF_POSTGRES_PASSWORD and TF_REDIS_PASSWORD from the existing
tfvars_file (using the same grep logic already used) before computing
has_all_required, then change the regeneration condition to regenerate (call
azure_generate_tfvars or aws_generate_tfvars) whenever any TF_* override is
present (e.g., TF_POSTGRES_PASSWORD or TF_REDIS_PASSWORD or other TF_* flags) or
when has_all_required is true so that CLI flag overrides (region, cluster name,
environment) are never ignored for the PROVIDER-specific generators.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f6cd965-9d3c-491a-93ce-69c81937b543
📒 Files selected for processing (1)
deployments/scripts/deploy-osmo-minimal.sh
vvnpn-nv
approved these changes
Mar 11, 2026
RyaliNvidia
approved these changes
Mar 11, 2026
fernandol-nvidia
approved these changes
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix minor bugs in aws deployment script
Issue - None
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores