ci: base container image change

[ranrubin]

Overview:

This PR adds the ability to use out CI registry (ECR) instead of the default nvcr.io registry for base images during container builds. This should translate to a cost reduction.

Changes

• container/build.sh: Added --registry flag to override the default nvcr.io registry in all base image URLs (TRTLLM, vLLM, SGLANG, NONE)
• .github/actions/docker-build/action.yml: Added use_ci_registry input parameter that sets the registry to ECR when enabled
• .github/workflows/container-validation-backends.yml: Enabled use_ci_registry: 'true' for backend validation builds

Where should the reviewer start?

container/build.sh

Summary by CodeRabbit

• New Features
  • Added optional CI registry support for container image builds, allowing selection between the default registry and an alternative CI registry during the build process.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The changes introduce optional CI registry support for container builds. A new use_ci_registry input parameter is added to the Docker build action, which conditionally selects either an ECR hostname or the default nvcr.io registry. This flag is passed through the workflow to the build script via a new --registry command-line option.

Changes

Cohort / File(s)	Summary
Docker build action input \.github/actions/docker-build/action\.yml	Adds new optional input use_ci_registry to enable registry selection between CI registry (ECR) and default registry (nvcr.io), conditionally setting REGISTRY variable and passing it to build.sh via --registry argument
Workflow validation steps \.github/workflows/container-validation-backends\.yml	Passes use_ci_registry: 'true' to docker-build action in three validation build steps (vllm, sglang, trtllm)
Build script registry option container/build\.sh	Implements new --registry command-line option in get_options to override default registry; replaces nvcr.io occurrences in TRTLLM_BASE_IMAGE, VLLM_BASE_IMAGE, NONE_BASE_IMAGE, SGLANG_BASE_IMAGE, and SGLANG_FRAMEWORK_IMAGE with provided registry value; updates help documentation

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Registry substitution pattern in build.sh applied across five base image variables — verify string replacement logic handles all intended image references
• Conditional registry selection flow from workflow input through action to build script — confirm flag propagation is correct

Poem

🐰 With whiskers twitched and ears held high,
We hop through registries, registry by registry!
ECR or nvcr, take your pick,
This flexible build makes the pipeline tick! 🚀

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title is vague and generic, using a non-descriptive term 'base container image change' that doesn't clearly convey the specific change of enabling CI registry support for cost reduction.	Consider a more specific title like 'ci: add support for using ECR registry for base images' or 'ci: allow configurable container registry for cost reduction'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description covers all required template sections (Overview, Details/Changes, Where should the reviewer start) with clear, relevant content about the ECR registry support implementation.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

.github/actions/docker-build/action.yml (1)

150-150: Consider quoting the registry variable.

While the current usage should work (registry hostnames typically don't contain spaces), it's a best practice to quote shell variables to prevent word splitting.

-          --registry $REGISTRY \
+          --registry "$REGISTRY" \

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c26e6fb and ecef83b.

📒 Files selected for processing (3)

• .github/actions/docker-build/action.yml (2 hunks)
• .github/workflows/container-validation-backends.yml (3 hunks)
• container/build.sh (4 hunks)

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-12-02T18:13:40.065Z

Learnt from: PeaBrane
Repo: ai-dynamo/dynamo PR: 4698
File: .github/workflows/container-validation-dynamo.yml:68-68
Timestamp: 2025-12-02T18:13:40.065Z
Learning: In the ai-dynamo/dynamo repository, backend-specific tests (vllm, sglang, trtllm) are intentionally excluded from the container-validation-dynamo.yml workflow using "not (vllm or sglang or trtllm)" because they run in a separate container-validation-backends.yml workflow that has dedicated jobs for each backend. This separation keeps framework-agnostic tests separate from backend-specific tests.

Applied to files:

• .github/workflows/container-validation-backends.yml

📚 Learning: 2025-06-24T21:58:17.757Z

Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 1623
File: deploy/cloud/helm/platform/values.yaml:48-52
Timestamp: 2025-06-24T21:58:17.757Z
Learning: In published Helm charts, it's a best practice to set `useKubernetesSecret: true` as the default for docker registry configuration while leaving server, username, and password fields empty. This ensures users must explicitly provide registry credentials at deployment time via `--set` flags or custom values files, rather than having sensitive data hardcoded in the chart repository.

Applied to files:

• .github/workflows/container-validation-backends.yml

🧬 Code graph analysis (1)

container/build.sh (1)

container/run.sh (1)

• missing_requirement (361-363)

⏰ 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). (5)

• GitHub Check: sglang (arm64)
• GitHub Check: operator (amd64)
• GitHub Check: vllm (arm64)
• GitHub Check: trtllm (arm64)
• GitHub Check: Build and Test - dynamo

🔇 Additional comments (7)

.github/workflows/container-validation-backends.yml (3)

172-172: LGTM: Consistent CI registry enablement across backends.

The addition of use_ci_registry: 'true' for vllm, sglang, and trtllm builds is consistent and aligns with the PR objective to reduce costs by using ECR instead of nvcr.io.

---

241-241: LGTM: Consistent with other backends.

---

303-303: LGTM: Consistent with other backends.

container/build.sh (3)

331-339: LGTM: Standard option parsing pattern.

The --registry option follows the established pattern used by other options in this script.

---

484-484: LGTM: Help text properly updated.

---

356-363: Registry substitution is correct.

The string substitution logic properly replaces nvcr.io in all base image variables when --registry is provided. The conditional check ensures it only runs when REGISTRY is set. The logic handles ECR hostname formats correctly.

.github/actions/docker-build/action.yml (1)

55-57: LGTM: Input definition is appropriate.

The optional use_ci_registry input follows the pattern of other optional inputs in this action.