Improve container workflow and build script#457
Conversation
Update containers workflow to only push images when triggered manually or on schedule, not on workflow file changes. - add push trigger for workflow file changes in containers.yml - update push condition to skip on regular push events - update CLAUDE.md commit message guidelines
WalkthroughModifies the container build workflow and script to add ROCm support, make build script behavior and base image/tag resolution configurable with logging, and updates contribution guidance in CLAUDE.md about including Co‑Authored‑By attribution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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
🤖 Fix all issues with AI agents
In @docker/build-container.sh:
- Around line 26-27: The current ARCH membership check uses unsafe array pattern
matching and concatenates array elements wrongly in the log; replace the [[ ! "
${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]] test with a proper loop that iterates
over ALLOWED_ARCHS and breaks on match (or use [[ ! " ${ALLOWED_ARCHS[*]} " ==
*" ${ARCH} "* ]] with careful quoting), and update the log_info call to print a
joined, space-separated list using "${ALLOWED_ARCHS[*]}" (not mixing array and
string) so membership testing and the error message are shellcheck-compliant;
adjust code around the conditional that references ALLOWED_ARCHS, ARCH and
log_info accordingly.
🧹 Nitpick comments (1)
docker/build-container.sh (1)
5-7: Consider using-ffor regular file check.Using
-echecks for any file type (including directories, symlinks, etc.). If you only want to source regular files, use-f:-if [ -e .env ]; then +if [ -f .env ]; then
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/containers.ymlCLAUDE.mddocker/build-container.sh
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-09T00:13:24.352Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T00:13:24.352Z
Learning: Do not mention 'created by claude' in commit messages
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-09T00:13:24.352Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T00:13:24.352Z
Learning: When summarizing changes only include details that require further action; just say 'Done.' when there is no further action
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-08T16:56:35.626Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 391
File: docker/build-container.sh:29-32
Timestamp: 2025-11-08T16:56:35.626Z
Learning: In the mostlygeek/llama-swap repository, llama.cpp container tags from ghcr.io/ggml-org/llama.cpp use build identifiers (e.g., `b6981`) rather than semantic versions. The tag format is `server-<something>-<build-id>` for cpu and `server-<arch>-<something>-<build-id>` for other architectures, where extracting the third field with `awk -F '-' '{print $3}'` correctly retrieves the build identifier.
Applied to files:
docker/build-container.sh
📚 Learning: 2025-10-27T16:04:23.381Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 368
File: docker/llama-swap.Containerfile:14-20
Timestamp: 2025-10-27T16:04:23.381Z
Learning: In docker/llama-swap.Containerfile, the user/group creation logic intentionally supports adding a non-root user (non-zero UID) to the root group (GID=0) as a valid configuration. This allows flexible UID/GID customization where users can specify only a UID or both UID and GID.
Applied to files:
docker/build-container.sh
🪛 Shellcheck (0.11.0)
docker/build-container.sh
[error] 26-26: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 26-26: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 27-27: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (7)
docker/build-container.sh (3)
9-17: LGTM!Clean logging helpers with conditional debug output. This improves observability without cluttering normal output.
23-23: LGTM!Adding
rocmto the allowed architectures aligns with the workflow matrix expansion.
37-43: Good configurability improvements.Making
BASE_IMAGEandLS_REPOconfigurable via environment variables enables testing with forks and alternative base images. The defaults are sensible..github/workflows/containers.yml (3)
13-16: LGTM!Triggering the workflow on changes to itself allows CI validation of workflow modifications without pushing images (controlled by line 39).
23-23: LGTM!Adding
rocmto the matrix is consistent with theALLOWED_ARCHSupdate inbuild-container.sh.
39-39: Verify the push behavior logic.The expression
${{ github.event_name != 'push' }}means:
- schedule/workflow_dispatch →
PUSH_IMAGES=true→ images are pushed- push (workflow file changes) →
PUSH_IMAGES=false→ images are not pushedThis aligns with the PR objective to skip image pushes when only workflow files change. Just confirming this is the intended behavior.
CLAUDE.md (1)
15-15: Confirm intentional policy reversal.Based on learnings, the previous policy was to not mention "created by claude" in commit messages. This change reverses that policy to now require attribution.
If this is intentional, the change is clear and actionable.
| if [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]]; then | ||
| echo "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}" | ||
| log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}" |
There was a problem hiding this comment.
Address shellcheck warnings for array handling.
Static analysis flags several issues:
- SC2199/SC2076: Array pattern matching in
[[ ]]with=~should use*or a loop. - SC2145: Mixing string and array in the log message concatenates elements without a separator.
🔧 Proposed fix using a loop
-if [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]]; then
- log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}"
+is_valid_arch=false
+for valid_arch in "${ALLOWED_ARCHS[@]}"; do
+ if [[ "$ARCH" == "$valid_arch" ]]; then
+ is_valid_arch=true
+ break
+ fi
+done
+if [[ "$is_valid_arch" == "false" ]]; then
+ log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[*]}"Alternatively, if you prefer the concise pattern-matching approach, use * instead of @ and unquote the regex:
-if [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]]; then
- log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}"
+if [[ ! " ${ALLOWED_ARCHS[*]} " =~ " ${ARCH} " ]]; then
+ log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[*]}"📝 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.
| if [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]]; then | |
| echo "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}" | |
| log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}" | |
| if [[ ! " ${ALLOWED_ARCHS[*]} " =~ " ${ARCH} " ]]; then | |
| log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[*]}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 26-26: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 26-26: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 27-27: Argument mixes string and array. Use * or separate argument.
(SC2145)
🤖 Prompt for AI Agents
In @docker/build-container.sh around lines 26 - 27, The current ARCH membership
check uses unsafe array pattern matching and concatenates array elements wrongly
in the log; replace the [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]] test with
a proper loop that iterates over ALLOWED_ARCHS and breaks on match (or use [[ !
" ${ALLOWED_ARCHS[*]} " == *" ${ARCH} "* ]] with careful quoting), and update
the log_info call to print a joined, space-separated list using
"${ALLOWED_ARCHS[*]}" (not mixing array and string) so membership testing and
the error message are shellcheck-compliant; adjust code around the conditional
that references ALLOWED_ARCHS, ARCH and log_info accordingly.
Add pagination support for fetching llama.cpp tags and improve debugging. - add build-container.sh to workflow trigger paths - implement fetch_llama_tag() with pagination support - replace .env with local testing instructions - add DEBUG_ABORT_BUILD flag for testing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docker/build-container.sh (4)
28-29: Address shellcheck warnings for array pattern matching.The current implementation works, but shellcheck flags several issues:
- SC2199/SC2076: Array expansion and quoted regex pattern
- SC2145: Mixing string and array in the log message
♻️ Proposed fix
# Check if ARCH is in the allowed list -if [[ ! " ${ALLOWED_ARCHS[@]} " =~ " ${ARCH} " ]]; then - log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[@]}" +if [[ ! " ${ALLOWED_ARCHS[*]} " =~ " ${ARCH} " ]]; then + log_info "Error: ARCH must be one of the following: ${ALLOWED_ARCHS[*]}" exit 1 fiUsing
[*]instead of[@]expands the array as a single string (elements joined by the first character of IFS), which is the intended behavior here and silences the shellcheck warnings.
63-71: Separate variable declaration and assignment to avoid masking errors.SC2155: When using
local var=$(command), the exit status ofcommandis masked bylocal's success. Ifcurlfails due to a network error,$?will be0(fromlocal), and the error won't be detected.While the code does check for API error messages in the response body, network-level failures could result in an empty or malformed response that might not trigger the existing error handling.
♻️ Proposed fix for lines 63-71
- local response=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \ + local response + response=$(curl -s -H "Authorization: Bearer $GITHUB_TOKEN" \ "https://api.github.com/users/ggml-org/packages/container/llama.cpp/versions?per_page=${per_page}&page=${page}") # Check for API errors if echo "$response" | jq -e '.message' > /dev/null 2>&1; then - local error_msg=$(echo "$response" | jq -r '.message') + local error_msg + error_msg=$(echo "$response" | jq -r '.message') log_info "GitHub API error: $error_msg" return 1 fi
80-82: Apply the same pattern forfound_tagassignment.SC2155: Same issue with masking return values.
♻️ Proposed fix
# Extract matching tag from this page - local found_tag=$(echo "$response" | jq -r \ + local found_tag + found_tag=$(echo "$response" | jq -r \ ".[] | select(.metadata.container.tags[]? | startswith(\"$tag_prefix\")) | .metadata.container.tags[] | select(startswith(\"$tag_prefix\"))" \ | sort -r | head -n1)
100-119: LGTM!The tag fetching logic correctly differentiates between CPU and other architectures. The
DEBUG_ABORT_BUILDfeature is useful for testing the tag resolution without triggering a full build.Minor nit:
[[ ! -z "$DEBUG_ABORT_BUILD" ]]could be[[ -n "$DEBUG_ABORT_BUILD" ]]for slightly more idiomatic bash, but this is purely stylistic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/containers.ymldocker/build-container.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/containers.yml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-08T16:56:35.626Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 391
File: docker/build-container.sh:29-32
Timestamp: 2025-11-08T16:56:35.626Z
Learning: In the mostlygeek/llama-swap repository, llama.cpp container tags from ghcr.io/ggml-org/llama.cpp use build identifiers (e.g., `b6981`) rather than semantic versions. The tag format is `server-<something>-<build-id>` for cpu and `server-<arch>-<something>-<build-id>` for other architectures, where extracting the third field with `awk -F '-' '{print $3}'` correctly retrieves the build identifier.
Applied to files:
docker/build-container.sh
📚 Learning: 2025-10-27T16:04:23.381Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 368
File: docker/llama-swap.Containerfile:14-20
Timestamp: 2025-10-27T16:04:23.381Z
Learning: In docker/llama-swap.Containerfile, the user/group creation logic intentionally supports adding a non-root user (non-zero UID) to the root group (GID=0) as a valid configuration. This allows flexible UID/GID customization where users can specify only a UID or both UID and GID.
Applied to files:
docker/build-container.sh
🪛 Shellcheck (0.11.0)
docker/build-container.sh
[error] 28-28: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 28-28: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 29-29: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 68-68: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 80-80: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (2)
- GitHub Check: run-tests
- GitHub Check: build-and-push (rocm)
🔇 Additional comments (4)
docker/build-container.sh (4)
11-19: LGTM!The logging helper functions are clean and follow a consistent pattern. Using
$*correctly passes all arguments as a single string for logging.
39-49: LGTM!Good configurability for testing with forked llama.cpp repositories and automatic repository detection via
GITHUB_REPOSITORY.
84-97: LGTM!Good use of
$NFto extract the build identifier from the last field, which robustly handles variable-length tag prefixes. The 50-page safety limit is a sensible safeguard against infinite loops.
121-144: LGTM!The build loop correctly handles both root and non-root container variants with appropriate UID/GID settings. The conditional push based on
PUSH_IMAGESaligns well with the workflow changes that pass this parameter.
* docker: add .env usage in build-container.sh * .github,docker: add rocm, improve logging * .github,CLAUDE.md: fix workflow and update guidelines Update containers workflow to only push images when triggered manually or on schedule, not on workflow file changes. - add push trigger for workflow file changes in containers.yml - update push condition to skip on regular push events - update CLAUDE.md commit message guidelines * docker: remove comma in build-container.sh * .github,docker: improve container build workflow Add pagination support for fetching llama.cpp tags and improve debugging. - add build-container.sh to workflow trigger paths - implement fetch_llama_tag() with pagination support - replace .env with local testing instructions - add DEBUG_ABORT_BUILD flag for testing
1 participant
Add ROCm support, improve build logging, and fix workflow push behavior.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.