Repository navigation
fix(docker): fix image build failures in engine Dockerfile and release workflows - #656
Conversation
github-actions
Bot
added
ci
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 significantly enhances the reliability of Docker image builds and improves the observability of the CI/CD pipeline. By addressing critical issues in the Dockerfile and refining the GitHub Actions workflows, it ensures more stable image creation and provides clearer, real-time feedback on build processes. Highlights
Changelog
Ignored Files
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
|
📝 WalkthroughWalkthroughUpdates GitHub Actions release workflows for SGLang, TRT-LLM, and vLLM Docker image builds with refined tag resolution logic, improved summary logging, default SMG commit version bump to 'v1.1.0', and cleanup steps. Dockerfile.engine refactored with explicit branching for clone/checkout operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a typo and improves the build logic in Dockerfile.engine to handle specific commit checkouts more reliably. However, the direct use of build arguments in shell commands within the Dockerfile introduces a potential command injection vulnerability if these arguments are sourced from untrusted user input. I recommend implementing strict validation for these build arguments in the CI/CD pipeline. Additionally, I've provided suggestions to further improve the shell script logic within the Dockerfile for better robustness and readability by using git -C and adding protective checks for environment variables.
| if [ "${ENGINE_COMMIT}" = "latest" ]; then \ | ||
| git clone --depth 1 "${ENGINE_REPO}" /opt/engine-src; \ | ||
| else \ | ||
| git clone "${ENGINE_REPO}" /opt/engine-src \ | ||
| && ( cd /opt/engine-src && git checkout "${ENGINE_COMMIT}" ); \ | ||
| fi; \ | ||
| else mkdir -p /opt/engine-src; fi \ | ||
| && git clone --depth 1 "${SMG_REPO}" /tmp/smg-src \ | ||
| && ( cd /tmp/smg-src && ( [ "${SMG_COMMIT}" = "latest" ] || git checkout "${SMG_COMMIT}" ) ) | ||
| && if [ "${SMG_COMMIT}" = "latest" ]; then \ | ||
| git clone --depth 1 "${SMG_REPO}" /tmp/smg-src; \ | ||
| else \ | ||
| git clone "${SMG_REPO}" /tmp/smg-src \ | ||
| && ( cd /tmp/smg-src && git checkout "${SMG_COMMIT}" ); \ | ||
| fi |
There was a problem hiding this comment.
The RUN instruction uses several build arguments (ENGINE_REPO, ENGINE_COMMIT, SMG_REPO, SMG_COMMIT) directly within a shell command. This introduces a potential command injection vulnerability, as an attacker controlling these arguments could inject arbitrary shell commands. For example, setting ENGINE_COMMIT to latest" ]; touch /tmp/pwned; if [ "1" = "1 could lead to arbitrary code execution during the build. To mitigate this, ensure strict validation of these build arguments in the CI/CD pipeline. Additionally, for improved robustness and clarity, consider using git -C instead of ( cd ... && git ... ) and adding protective checks for environment variables like SMG_REPO and SMG_COMMIT before use.
&& if [ -n "${ENGINE_REPO}" ] && [ -n "${ENGINE_COMMIT}" ]; then \
if [ "${ENGINE_COMMIT}" = "latest" ]; then \
git clone --depth 1 "${ENGINE_REPO}" /opt/engine-src; \
else \
git clone "${ENGINE_REPO}" /opt/engine-src \
&& git -C /opt/engine-src checkout "${ENGINE_COMMIT}"; \
fi; \
else mkdir -p /opt/engine-src; fi \
&& if [ -n "${SMG_REPO}" ] && [ -n "${SMG_COMMIT}" ]; then \
if [ "${SMG_COMMIT}" = "latest" ]; then \
git clone --depth 1 "${SMG_REPO}" /tmp/smg-src; \
else \
git clone "${SMG_REPO}" /tmp/smg-src \
&& git -C /tmp/smg-src checkout "${SMG_COMMIT}"; \
fi; \
else mkdir -p /tmp/smg-src; fi
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-vllm-docker.yml:
- Line 142: The workflow uses the wrong step output for the image tag: change
the reference from steps.push-ghcr.outputs.image_tag to the step that actually
exposes image_tag (steps.resolve-tag.outputs.image_tag); update the summary echo
to read the image_tag from resolve-tag so it prints the resolved image tag
instead of an empty value, keeping the push-ghcr step as-is which only exposes
image_name.
In `@docker/Dockerfile.engine`:
- Around line 11-25: The SMG clone block lacks a guard for empty
SMG_REPO/SMG_COMMIT and can run git clone "" causing build failures; update the
SMG handling to mirror the ENGINE logic: check if [ -n "${SMG_REPO}" ] && [ -n
"${SMG_COMMIT}" ] before attempting any git operations, and if not present
create the destination directory (e.g., /tmp/smg-src); when present, keep the
existing behavior of using depth=1 for SMG_COMMIT="latest" and full clone +
checkout otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cdb10a2-74c5-41a4-b7d7-91731ba5dd43
📒 Files selected for processing (4)
.github/workflows/release-sglang-docker.yml.github/workflows/release-trtllm-docker.yml.github/workflows/release-vllm-docker.ymldocker/Dockerfile.engine
| run: | | ||
| echo "## Image" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "**Image tag:** \`${{ steps.push-ghcr.outputs.image_tag }}\`" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
Bug: Incorrect step output reference will produce empty image tag.
Line 142 references steps.push-ghcr.outputs.image_tag, but the push-ghcr step (lines 113-124) only sets image_name as an output (line 121), not image_tag.
This should use steps.resolve-tag.outputs.image_tag like the sglang and trtllm workflows do.
🐛 Proposed fix
- echo "**Image tag:** \`${{ steps.push-ghcr.outputs.image_tag }}\`" >> $GITHUB_STEP_SUMMARY
+ echo "**Image tag:** \`${{ steps.resolve-tag.outputs.image_tag }}\`" >> $GITHUB_STEP_SUMMARY📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "**Image tag:** \`${{ steps.push-ghcr.outputs.image_tag }}\`" >> $GITHUB_STEP_SUMMARY | |
| echo "**Image tag:** \`${{ steps.resolve-tag.outputs.image_tag }}\`" >> $GITHUB_STEP_SUMMARY |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-vllm-docker.yml at line 142, The workflow uses the
wrong step output for the image tag: change the reference from
steps.push-ghcr.outputs.image_tag to the step that actually exposes image_tag
(steps.resolve-tag.outputs.image_tag); update the summary echo to read the
image_tag from resolve-tag so it prints the resolved image tag instead of an
empty value, keeping the push-ghcr step as-is which only exposes image_name.
| RUN apk add --no-cache git \ | ||
| && if [ -n "${ENGINE_REPO}" ] && [ -n "${ENGINE_COMMIT}" ]; then \ | ||
| git clone --depth 1 "${ENGINE_REPO}" /opt/engine-sr \ | ||
| && ( cd /opt/engine-src && ( [ "${ENGINE_COMMIT}" = "latest" ] || git checkout "${ENGINE_COMMIT}" ) ); \ | ||
| if [ "${ENGINE_COMMIT}" = "latest" ]; then \ | ||
| git clone --depth 1 "${ENGINE_REPO}" /opt/engine-src; \ | ||
| else \ | ||
| git clone "${ENGINE_REPO}" /opt/engine-src \ | ||
| && ( cd /opt/engine-src && git checkout "${ENGINE_COMMIT}" ); \ | ||
| fi; \ | ||
| else mkdir -p /opt/engine-src; fi \ | ||
| && git clone --depth 1 "${SMG_REPO}" /tmp/smg-src \ | ||
| && ( cd /tmp/smg-src && ( [ "${SMG_COMMIT}" = "latest" ] || git checkout "${SMG_COMMIT}" ) ) | ||
| && if [ "${SMG_COMMIT}" = "latest" ]; then \ | ||
| git clone --depth 1 "${SMG_REPO}" /tmp/smg-src; \ | ||
| else \ | ||
| git clone "${SMG_REPO}" /tmp/smg-src \ | ||
| && ( cd /tmp/smg-src && git checkout "${SMG_COMMIT}" ); \ | ||
| fi |
There was a problem hiding this comment.
Missing guard for empty SMG_REPO could cause build failures.
The ENGINE_REPO block (lines 12-19) properly guards against empty values with if [ -n "${ENGINE_REPO}" ] && [ -n "${ENGINE_COMMIT}" ]. However, the SMG_REPO block (lines 20-25) directly attempts to clone without verifying that SMG_REPO is non-empty.
Although workflows define a default for smg_repo, the input is required: false, meaning an empty value could be passed. If SMG_REPO is empty, git clone "" /tmp/smg-src will fail.
🐛 Proposed fix to guard SMG_REPO
- && if [ "${SMG_COMMIT}" = "latest" ]; then \
- git clone --depth 1 "${SMG_REPO}" /tmp/smg-src; \
- else \
- git clone "${SMG_REPO}" /tmp/smg-src \
- && ( cd /tmp/smg-src && git checkout "${SMG_COMMIT}" ); \
- fi
+ && if [ -n "${SMG_REPO}" ]; then \
+ if [ "${SMG_COMMIT}" = "latest" ]; then \
+ git clone --depth 1 "${SMG_REPO}" /tmp/smg-src; \
+ else \
+ git clone "${SMG_REPO}" /tmp/smg-src \
+ && ( cd /tmp/smg-src && git checkout "${SMG_COMMIT}" ); \
+ fi; \
+ else \
+ echo "SMG_REPO is required" && exit 1; \
+ fi🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 11-11: Use WORKDIR to switch to a directory
(DL3003)
[warning] 11-11: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/Dockerfile.engine` around lines 11 - 25, The SMG clone block lacks a
guard for empty SMG_REPO/SMG_COMMIT and can run git clone "" causing build
failures; update the SMG handling to mirror the ENGINE logic: check if [ -n
"${SMG_REPO}" ] && [ -n "${SMG_COMMIT}" ] before attempting any git operations,
and if not present create the destination directory (e.g., /tmp/smg-src); when
present, keep the existing behavior of using depth=1 for SMG_COMMIT="latest" and
full clone + checkout otherwise.
2 participants
Summary
engine-sr→engine-srccausedcdto fail after clone--depth 1checkout failure: shallow clone cannot checkout a specific commit SHA; now uses full clone when a specific commit is given,--depth 1only forlatestChanges
docker/Dockerfile.engineengine-sr→engine-src)latestuses--depth 1, specific commit/ref uses full clone +git checkout.github/workflows/release-{vllm,sglang,trtllm}-docker.ymlBuild image: doneandPush image: <name>milestones to summary as steps completeResolve image tagstep (was embedded in the build+push step), align with sglang/trtllm structureBuild base image: donesummary line after optional base image build stepif: always()to Summary step and addClean up local imagesstep🤖 Generated with Claude Code
Summary by CodeRabbit