Conversation
Reviewer's GuideThis PR introduces a dedicated bash installer script for managing ca-certificates and Vulkan libraries based on LLAMA_SERVER_VARIANT, integrates it into the Dockerfile to streamline image setup, and adds a Makefile target to run shellcheck on all shell scripts to catch issues early. Class diagram for the new apt-install.sh scriptclassDiagram
class apt-install.sh {
+main(args)
-set -eux -o pipefail
-apt-get update
-packages: array
-conditional libvulkan1 addition
-apt-get install
-rm -rf /var/lib/apt/lists/*
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, 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 focuses on improving the robustness and maintainability of the project's scripting and build processes. It introduces static analysis for bash scripts using Highlights
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.
Pull Request Overview
This PR adds shellcheck validation to improve bash script quality and introduces a new apt installation script. The changes focus on adding linting validation and modernizing the package installation process in the Docker build.
- Adds shellcheck validation target to the Makefile for bash script linting
- Introduces a new bash script for apt package installation with conditional Vulkan support
- Updates Dockerfile to use the new apt installation script
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/apt-install.sh | New bash script for installing apt packages with conditional Vulkan support |
| Makefile | Adds shellcheck validation target to lint bash scripts |
| Dockerfile | Integrates the new apt installation script into the build process |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Trying to avoid things like: happening again |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The Dockerfile contains a redundant apt-get update/install for ca-certificates/vulkan after calling apt-install.sh; consolidate these into one step to avoid duplicate layers and reduce image size.
- Consider integrating the new validate (shellcheck) target into your CI pipeline or default Make workflow so Bash linting runs automatically rather than as a manual step.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Dockerfile contains a redundant apt-get update/install for ca-certificates/vulkan after calling apt-install.sh; consolidate these into one step to avoid duplicate layers and reduce image size.
- Consider integrating the new validate (shellcheck) target into your CI pipeline or default Make workflow so Bash linting runs automatically rather than as a manual step.
## Individual Comments
### Comment 1
<location> `Dockerfile:43-46` </location>
<code_context>
# Create non-root user
RUN groupadd --system modelrunner && useradd --system --gid modelrunner --create-home --home-dir /home/modelrunner modelrunner
+COPY scripts/apt-install.sh apt-install.sh
+
+# Install ca-certificates for HTTPS and vulkan
+RUN ./apt-install.sh
+
# Install ca-certificates for HTTPS and vulkan
</code_context>
<issue_to_address>
**suggestion:** Consider removing duplicate package installation logic.
The Dockerfile repeats apt-get update and installs ca-certificates after these are already handled in apt-install.sh. Removing the duplicate steps will simplify the build process.
</issue_to_address>
### Comment 2
<location> `Makefile:37-38` </location>
<code_context>
test:
go test -v ./...
+validate:
+ shellcheck */*.sh
+
# Build Docker image
</code_context>
<issue_to_address>
**suggestion:** Shellcheck invocation may miss scripts in nested directories.
Consider replacing the current pattern with a recursive find command to ensure shellcheck validates scripts in all subdirectories.
```suggestion
validate:
find . -type f -name "*.sh" -print0 | xargs -0 shellcheck
```
</issue_to_address>
### Comment 3
<location> `scripts/apt-install.sh:8-10` </location>
<code_context>
+
+ apt-get update
+ local packages=("ca-certificates")
+ if [ "${LLAMA_SERVER_VARIANT}" = "generic" ] || [ "${LLAMA_SERVER_VARIANT}" = "cpu" ]; then
+ packages+=("libvulkan1")
+ fi
+
</code_context>
<issue_to_address>
**suggestion:** LLAMA_SERVER_VARIANT may be unset, leading to unexpected behavior.
Consider explicitly handling or documenting the case when LLAMA_SERVER_VARIANT is unset to clarify expected behavior.
```suggestion
# If LLAMA_SERVER_VARIANT is unset, default behavior is to not add libvulkan1.
if [ -n "${LLAMA_SERVER_VARIANT:-}" ] && { [ "${LLAMA_SERVER_VARIANT}" = "generic" ] || [ "${LLAMA_SERVER_VARIANT}" = "cpu" ]; }; then
packages+=("libvulkan1")
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 introduces shellcheck to validate bash scripts, which is a great addition for maintaining script quality. A new validate target is added to the Makefile, and the package installation logic in the Dockerfile is refactored into a separate script. My review includes suggestions to improve this refactoring by removing redundant code and applying Docker best practices, as well as making the new validate target more robust.
ericcurtin
force-pushed
the
shellcheck
branch
from
68f8d84 to
e6df5ce
Compare
ericcurtin
force-pushed
the
shellcheck
branch
from
e6df5ce to
a081e15
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ericcurtin
force-pushed
the
shellcheck
branch
from
a081e15 to
a770487
Compare
Help to avoid bugs Signed-off-by: Eric Curtin <eric.curtin@docker.com>
ericcurtin
force-pushed
the
shellcheck
branch
from
a770487 to
e2cc266
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Help to avoid bugs
Summary by Sourcery
Centralize apt package installation into a dedicated script and integrate shellcheck validation into the Makefile and Docker build
New Features:
Enhancements: