Skip to content

Conversation

@nathan-weinberg
Copy link
Collaborator

@nathan-weinberg nathan-weinberg commented Sep 3, 2025

Summary by CodeRabbit

  • New Features

    • CI-ready VLLM setup action to start a local inference service for validation.
  • Tests

    • New smoke test that launches the container, checks health, lists models, and validates a chat completion.
  • Chores

    • Added new Red Hat distribution build/test/publish workflow and removed the legacy multi-arch workflow.
    • Improved workflow concurrency grouping for PR/non-PR runs.
    • Added actionlint and shellcheck to pre-commit; removed a deprecated local hook.
    • Bumped distribution provider dependency versions.

@coderabbitai
Copy link

coderabbitai bot commented Sep 3, 2025

Warning

Rate limit exceeded

@nathan-weinberg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 59 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4a2eefe and 44d6819.

📒 Files selected for processing (6)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)

Walkthrough

Adds a composite GitHub Action to start a VLLM Docker service, replaces a legacy Red Hat multi-arch publish workflow with a new build-test-publish workflow that uses the action and a smoke test, updates workflow concurrency expressions, adds actionlint/shellcheck pre-commit hooks, bumps two provider package pins, and adds a container smoke-test script.

Changes

Cohort / File(s) Summary
VLLM setup action
.github/actions/setup-vllm/action.yml
New composite action that starts a detached VLLM Docker container (host networking / port 8000), sets VLLM args (model, parser, served-model-name, etc.), and polls /health up to 900s for readiness.
Red Hat distro workflow (new)
.github/workflows/redhat-distro-container.yml
New workflow: builds image from distribution/Containerfile (load=true), sets up QEMU/Buildx, invokes the local setup-vllm action, runs tests/smoke.sh, and conditionally logs into Quay and pushes the image (SHA and latest on main).
Red Hat distro workflow (removed)
.github/workflows/redhat-distro-container-build.yml
Deleted previous multi-arch build-and-publish workflow (matrix build, QEMU/Buildx, conditional Quay login/push and triggers).
Workflow concurrency tweaks
.github/workflows/pre-commit.yml, .github/workflows/semantic-pr.yml
Updated concurrency.group expressions to use the PR number with a fallback to github.ref to avoid empty groups; cancel-in-progress remains enabled.
Pre-commit config
.pre-commit-config.yaml
Added actionlint (rhysd/[email protected]) and shellcheck ([email protected]) hooks; removed the local Distribution Build (pkg-gen) hook.
Containerfile dependency pins
distribution/Containerfile
Bumped pip pins: llama_stack_provider_lmeval 0.1.7 → 0.2.4 and llama_stack_provider_trustyai_fms 0.1.2 → 0.2.1.
Provider configs
distribution/providers.d/remote/eval/trustyai_lmeval.yaml, distribution/providers.d/remote/safety/trustyai_fms.yaml
Updated provider package versions to match Containerfile bumps (0.2.4 and 0.2.1 respectively).
Smoke test script
tests/smoke.sh
New bash smoke-test: runs built image (env: INFERENCE_MODEL, VLLM_URL), waits for /v1/health, verifies /v1/models includes INFERENCE_MODEL, performs an OpenAI-style chat completion request, and prints container logs on failures.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Runner as GitHub Runner
  participant Buildx as Docker Buildx
  participant SetupVLLM as setup-vllm Action
  participant VLLM as VLLM Container (:8000)
  participant Image as Built Image (loaded)
  participant LS as Llama Stack Container (:8321)
  participant Smoke as tests/smoke.sh

  Runner->>Buildx: Build image from distribution/Containerfile (load=true, tag=SHA)
  Runner->>SetupVLLM: Start VLLM action (detached) and wait for /health
  SetupVLLM->>VLLM: Run container (host network, :8000), poll /health
  Note over VLLM: VLLM serves inference model on :8000

  Runner->>Image: Run built image (env: INFERENCE_MODEL, VLLM_URL)
  Runner->>LS: Start Llama Stack container (:8321)
  Smoke->>LS: Poll GET /v1/health until {"status":"OK"}
  Smoke->>LS: GET /v1/models (assert INFERENCE_MODEL present)
  Smoke->>LS: POST /v1/openai/v1/chat/completions (validate response)

  alt push event
    Runner->>Quay: Login & push image (SHA)
    alt branch == main
      Runner->>Quay: Tag and push `latest`
    end
  end

  Smoke->>LS: Tear down container on EXIT
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
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.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately and concisely summarizes the primary change — updating the distribution container CI workflow to add a smoke test — using a conventional "ci:" prefix and no extraneous details. It directly reflects the added smoke test script and workflow modifications in the changeset.

Poem

I nudged a VLLM awake at dawn,
spun up containers on a dew-fresh lawn.
Tests hopped through health and models with glee,
pins updated, old workflow set free.
Quiet rabbit, CI green — off I’m gone. 🐇

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
.github/workflows/redhat-distro-container.yml (1)

59-59: Credentials OK; consider OIDC later.
Login step is correct. For future hardening, consider Quay OIDC tokens (workload identity) to drop long‑lived passwords.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between bc39404 and 9ca29da.

📒 Files selected for processing (1)
  • .github/workflows/redhat-distro-container.yml (3 hunks)
🔇 Additional comments (2)
.github/workflows/redhat-distro-container.yml (2)

1-1: Workflow rename is clear and accurate.
Naming reflects the new build–test–publish flow.


24-24: Job restructuring LGTM.
Separating build, test, and publish is a good move for CI clarity and gatekeeping.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
.github/workflows/redhat-distro-container.yml (3)

40-50: Add Buildx cache to avoid double-build cost and speed up CI.
Mirror cache config in both build (load) and publish steps to share layers.

       - name: Build image
         id: build
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: false
           tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}
           load: true  # needed to load for smoke test
+          cache-from: type=gha
+          cache-to: type=gha,mode=max

Note: when you later enable multiple platforms, load: true will fail; gate the smoke-test build to a single platform (e.g., linux/amd64) and keep the multi-arch publish build separate.


51-57: Prevent hangs; add timeouts and harden docker run.
Add a step-level timeout and ensure the run exits if the image’s CMD is long-lived.

-      - name: Test image
+      - name: Test image
+        timeout-minutes: 2
         id: test
         run: |
-          docker run --rm \
+          set -euo pipefail
+          timeout 120s docker run --rm --pull=never \
+            --platform ${{ matrix.platform }} \
             --env "INFERENCE_MODEL=dummy" \
             ${{ env.IMAGE_NAME }}:${{ github.sha }}

67-76: Don’t push latest on non-main; reuse Docker metadata and enable cache on publish.
Gate the latest tag to main and let metadata-action manage tags/labels; also add cache here.

+      - name: Docker meta
+        id: meta
+        uses: docker/metadata-action@8e5442c4ef9f78752691e2e8b5b5f42f53b8b3c0 # v5.5.1
+        with:
+          images: ${{ env.IMAGE_NAME }}
+          tags: |
+            type=sha
+            type=ref,event=branch
+            type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+
       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
-          tags: ${{ env.IMAGE_NAME }}:${{ github.sha }},${{ env.IMAGE_NAME }}:latest
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
+          cache-from: type=gha
+          cache-to: type=gha,mode=max
🧹 Nitpick comments (3)
.pre-commit-config.yaml (1)

38-42: Good addition; scope actionlint to workflows and avoid external tool noise.
Actionlint runs fine, but by default it will scan any YAML you pass it and try to call external linters if present. Scoping to workflow files and disabling optional integrations keeps pre-commit fast and deterministic.

 -   repo: https://github.com/rhysd/actionlint
     rev: v1.7.7
     hooks:
-      - id: actionlint
+      - id: actionlint
+        files: ^\.github/workflows/.*\.ya?ml$
+        args:
+          - -shellcheck=   # disable external ShellCheck invocation if not installed
+          - -pyflakes=     # disable external Pyflakes invocation if not installed
.github/workflows/redhat-distro-container.yml (2)

24-24: Job key rename is fine; consider adding concurrency to avoid duplicate runs.
Optional hardening to cancel superseded runs per ref.

 jobs:
   build-test-push:
     runs-on: ubuntu-latest
+    concurrency:
+      group: redhat-distro-container-${{ github.ref }}
+      cancel-in-progress: true

34-36: QEMU setup is unnecessary for single-arch builds.
Slight speedup: skip QEMU until you include non-amd64 in the matrix.

-      - name: Set up QEMU
-        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
+      - name: Set up QEMU
+        if: contains(matrix.platform, 'arm64') || contains(matrix.platform, 'ppc64le') || contains(matrix.platform, 's390x')
+        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9ca29da and f384e7e.

📒 Files selected for processing (2)
  • .github/workflows/redhat-distro-container.yml (3 hunks)
  • .pre-commit-config.yaml (1 hunks)
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)
🔇 Additional comments (2)
.github/workflows/redhat-distro-container.yml (2)

1-1: Name change LGTM.
Clearer workflow title; matches the new build→test→publish flow.


59-59: Adding an id for login is fine; verify secrets exist.
Ensure QUAY_USERNAME and QUAY_PASSWORD are configured in repo/org secrets to avoid push-time failures.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
.github/workflows/redhat-distro-container.yml (2)

53-55: Add a step-level timeout to prevent hung jobs.

Guard the smoke test with a hard timeout so the runner can recover from non-terminating runs.

       - name: Test image
+        timeout-minutes: 2
         id: test
         run: |

91-102: Prefer docker/metadata-action for robust, conditional tagging (latest only on main).

Removes brittle inline expressions and gives you branch/sha tags automatically.

+      - name: Docker meta
+        id: meta
+        uses: docker/metadata-action@8e5442c4ef9f78752691e2e8b5b5f42f53b8b3c0 # v5.5.1
+        with:
+          images: ${{ env.IMAGE_NAME }}
+          tags: |
+            type=sha
+            type=ref,event=branch
+            type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+
       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
-          tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}${{ github.ref == 'refs/heads/main' && format(',{0}:latest', env.IMAGE_NAME) || '' }}
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
           cache-from: type=gha
           cache-to: type=gha,mode=max
🧹 Nitpick comments (2)
.github/workflows/redhat-distro-container.yml (2)

34-36: Skip QEMU when building only linux/amd64 to shave CI time.

       - name: Set up QEMU
+        if: ${{ matrix.platform != 'linux/amd64' }}
         uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0

1-22: Lock down default GITHUB_TOKEN permissions.

This workflow doesn’t need elevated permissions; reduce the attack surface.

 name: Build, test, and publish Red Hat Distribution Containers
 
 on:
@@
   push:
     branches:
       - main
       - rhoai-v*
 
+permissions:
+  contents: read
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between f384e7e and fc5ea17.

📒 Files selected for processing (3)
  • .github/workflows/redhat-distro-container-build.yml (0 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/redhat-distro-container-build.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .pre-commit-config.yaml
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
.github/workflows/redhat-distro-container.yml (1)

53-79: Add a step-level timeout and align platform in docker run.

Your custom loop is good. Add a guardrail timeout and pin platform for consistency.

-      - name: Test image
+      - name: Test image
+        timeout-minutes: 5
         id: test
         run: |
           set -euo pipefail
           # Start llama stack
-          CID="$(docker run -d --pull=never \
+          CID="$(docker run -d --pull=never \
+            --platform ${{ matrix.platform }} \
             -p 8321:8321 \
             --env INFERENCE_MODEL=dummy \
-            --name llama-stack \
+            --name llama-stack-${{ github.sha }} \
             ${{ env.IMAGE_NAME }}:${{ github.sha }})"
           trap 'docker rm -f "$CID" >/dev/null 2>&1 || true' EXIT

Optional hardening (if image tolerates it): add --init --security-opt no-new-privileges.

🧹 Nitpick comments (6)
.github/workflows/redhat-distro-container.yml (6)

3-15: Consider adding workflow_dispatch and reopen triggers.

Optional quality-of-life: allow manual runs and handle reopened PRs.

 on:
   pull_request:
     branches:
       - main
       - rhoai-v*
     types:
-      - opened
+      - opened
+      - reopened
       - synchronize
   push:
     branches:
       - main
       - rhoai-v*
+  workflow_dispatch:

16-22: Tighten default permissions at workflow or job level.

Least-privilege GITHUB_TOKEN is recommended.

 jobs:
   build-test-push:
+    permissions:
+      contents: read

Alternatively at the top-level:

 name: Build, test, and publish Red Hat Distribution Containers
+permissions:
+  contents: read

27-36: Skip QEMU when building native amd64 to save minutes.

Gate the QEMU step by platform until multi-arch lands.

 - name: Set up QEMU
   uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
+  if: matrix.platform != 'linux/amd64'

40-52: Make cache resilient on forked PRs.

GHA cache writes can be unavailable on forks; avoid hard failures.

       with:
         context: .
         file: distribution/Containerfile
         platforms: ${{ matrix.platform }}
         push: false
         tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}
         load: true  # needed to load for smoke test
-        cache-from: type=gha
-        cache-to: type=gha,mode=max
+        cache-from: type=gha
+        cache-to: type=gha,mode=max,ignore-error=true

Mirror the same change in the publish step below.


89-101: Nice fix to gate :latest to main; consider metadata-action for richer tags/labels.

For discoverability, add branch tags for rhoai-v* and OCI labels via docker/metadata-action.

+      - name: Docker meta
+        if: github.event_name == 'push'
+        id: meta
+        uses: docker/metadata-action@8e5442c4ef9f78752691e2e8b5b5f42f53b8b3c0 # v5.5.1
+        with:
+          images: ${{ env.IMAGE_NAME }}
+          tags: |
+            type=sha
+            type=ref,event=branch
+            type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+
       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
-          tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}${{ github.ref == 'refs/heads/main' && format(',{0}:latest', env.IMAGE_NAME) || '' }}
-          cache-from: type=gha
-          cache-to: type=gha,mode=max
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
+          cache-from: type=gha
+          cache-to: type=gha,mode=max,ignore-error=true

Bonus: consider attests: type=sbom or provenance: mode=max if you want SBOM/SLSA.


23-29: Avoid duplicate work on rapid pushes with concurrency.

Cancel in-flight runs per ref to save CI minutes.

 jobs:
   build-test-push:
     runs-on: ubuntu-latest
+    concurrency:
+      group: build-test-push-${{ github.ref }}
+      cancel-in-progress: true
     strategy:
       matrix:
         platform: [linux/amd64] # TODO: enable other arch once all pip packages are available.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fc5ea17 and 20e6e90.

📒 Files selected for processing (3)
  • .github/workflows/redhat-distro-container-build.yml (0 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/redhat-distro-container-build.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .pre-commit-config.yaml
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)
🔇 Additional comments (2)
.github/workflows/redhat-distro-container.yml (2)

40-52: Buildx cache usage looks good.

Local build is cached with GHA and loads for smoke test. Nice.


80-88: Good gating of registry login.

Secrets are only used on push. Looks correct.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
distribution/providers.d/remote/safety/trustyai_fms.yaml (1)

3-3: Optional: strengthen supply-chain pinning.

If your installer honors extra pip args in this list, consider adding a hash to the pin or installing via a constraints/lock file referenced by both this YAML and the Containerfile to avoid drift.

-  pip_packages: ["llama_stack_provider_trustyai_fms==0.2.1"]
+  # If supported by your installer:
+  pip_packages: ["llama_stack_provider_trustyai_fms==0.2.1 --require-hashes --hash=sha256:<publish-hash>"]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 20e6e90 and fdb082f.

📒 Files selected for processing (2)
  • distribution/Containerfile (1 hunks)
  • distribution/providers.d/remote/safety/trustyai_fms.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • distribution/Containerfile
🔇 Additional comments (2)
distribution/providers.d/remote/safety/trustyai_fms.yaml (2)

3-3: Version bump pin looks good.

Pinned to 0.2.1 for reproducibility; matches the stated Containerfile update in this PR.


3-3: All trustyai_fms pins updated to 0.2.1. Verified no lingering 0.1.2 references; both distribution/Containerfile and providers.d/remote/safety/trustyai_fms.yaml now pin version 0.2.1.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1)

3-3: Version bump to 0.2.1 confirmed; no legacy 0.1.7 references found. Optional: pin kubernetes to a stable major/minor range for reproducible builds (e.g., "kubernetes>=30,<31").

distribution/Containerfile (1)

17-18: Versions are consistent; consider centralizing them
Verified that both distribution/Containerfile (lines 17–18) and the provider YAMLs under distribution/providers.d pin llama_stack_provider_lmeval and trustyai_fms to 0.2.1. To avoid future drift, drive these versions from a single source of truth (e.g., a versions map in distribution/build.py).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fdb082f and dac9825.

📒 Files selected for processing (2)
  • distribution/Containerfile (1 hunks)
  • distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1 hunks)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
.github/workflows/pre-commit.yml (1)

9-10: Fix concurrency grouping for push events (fallback to ref).

Using only github.event.pull_request.number yields an empty suffix on push events, causing cross-branch cancellation. Fall back to github.ref.

-concurrency:
-  group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
   cancel-in-progress: true
.github/workflows/redhat-distro-container.yml (4)

16-18: Make concurrency robust for both PRs and pushes.

Same rationale as the pre-commit workflow: add a ref fallback to avoid cross-branch cancellations on pushes.

-concurrency:
-  group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
   cancel-in-progress: true

38-40: Skip QEMU when building only amd64.

Saves seconds per run; keep if multi-arch is imminent.

-      - name: Set up QEMU
-        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
+      - name: Set up QEMU
+        if: ${{ contains(matrix.platform, 'arm64') || contains(matrix.platform, 'ppc64le') || contains(matrix.platform, 's390x') }}
+        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0

61-86: Harden the smoke test and give slower boots a bit more headroom.

Minor robustness: longer wait, faster fail per attempt, and a quick functional probe after health.

       - name: Test image
         id: test
         run: |
           set -euo pipefail
           # Start llama stack
           CID="$(docker run -d --pull=never \
             -p 8321:8321 \
             --env INFERENCE_MODEL=meta-llama/Llama-3.2-1B-Instruct \
             --name llama-stack \
             ${{ env.IMAGE_NAME }}:${{ github.sha }})"
           trap 'docker rm -f "$CID" >/dev/null 2>&1 || true' EXIT
           echo "Started Llama Stack container: $CID"

           echo "Waiting for Llama Stack server..."
-          for i in {1..60}; do
+          for i in {1..180}; do
             echo "Attempt $i to connect to Llama Stack..."
-            if curl -fsS --max-time 2 http://127.0.0.1:8321/v1/health | grep -q '"status":"OK"'; then
+            if curl -fsS --max-time 2 http://127.0.0.1:8321/v1/health | grep -q '"status":"OK"'; then
               echo "Llama Stack server is up :)"
-              exit 0
+              # Quick functional probe
+              curl -fsS --max-time 3 http://127.0.0.1:8321/v1/models >/dev/null || true
+              exit 0
             fi
             sleep 1
           done
           echo "Llama Stack server failed to start :("
           echo "Container logs:"
-          docker logs "$CID" || true
+          docker logs --tail=200 "$CID" || true
           exit 1

97-108: Mirror build reliability in publish step.

Add pull: true for fresh bases and keep cache symmetry.

       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
           tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}${{ github.ref == 'refs/heads/main' && format(',{0}:latest', env.IMAGE_NAME) || '' }}
+          pull: true
           cache-from: type=gha
           cache-to: type=gha,mode=max
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 11cb7d3 and 70d6473.

📒 Files selected for processing (3)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/actions/setup-vllm/action.yml
🔇 Additional comments (1)
.github/workflows/redhat-distro-container.yml (1)

44-56: Drop explicit provenance: false—it’s redundant and can break on older BuildKit versions; add pull: true to always fetch fresh base images.

Likely an incorrect or invalid review comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
.github/workflows/semantic-pr.yml (2)

12-12: Nit: prefer run_id as the fallback to avoid cross-cancel on the same branch if new triggers are added

On pull_request_target, number is always present, so the fallback won’t fire today. If you later add workflow_dispatch/schedule, using github.ref will cause runs on the same branch to cancel each other. If that’s not desired, use run_id (or ref_name for per-branch intent).

Apply:

-  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.run_id }}

15-17: Verify minimal permissions—action may need statuses: write

amannn/action-semantic-pull-request typically reports a status/check. With only contents: read, it might not be able to publish the result. Consider least-privilege but sufficient perms.

Proposed:

 permissions:
-  contents: read
+  contents: read
+  pull-requests: read
+  statuses: write
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 70d6473 and 9fc099c.

📒 Files selected for processing (4)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/workflows/pre-commit.yml
  • .github/actions/setup-vllm/action.yml
  • .github/workflows/redhat-distro-container.yml
🔇 Additional comments (2)
.github/workflows/semantic-pr.yml (2)

11-13: Concurrency key change looks good

Using the PR number with a ref fallback is a sane way to scope cancellations per PR and avoid duplicate runs. Matches the pattern adopted elsewhere.


23-23: Good: action pinned to commit SHA

Pinned to a specific SHA for supply-chain safety.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.pre-commit-config.yaml (1)

48-60: Local “Distribution Build” hook likely obsolete and heavy; remove or gate it

This always_run local hook will execute on every commit (pass_filenames: false), which can slow contributors and duplicates CI responsibilities now covered by the updated workflows. Also, the PR summary implies this block was removed, but it’s still present.

Apply:

- -   repo: local
-     hooks:
-       - id: pkg-gen
-         name: Distribution Build
-         entry: ./distribution/build.py
-         language: python
-         pass_filenames: false
-         require_serial: true
-         always_run: true
-         files: ^distribution/.*$
-         additional_dependencies:
-           - llama-stack==0.2.18

If you still need a local helper, consider making it a tox/nox task or a Makefile target invoked manually, not a pre-commit hook.

🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)

38-42: Good add: actionlint for workflow hygiene

Adding actionlint is on point for CI reliability. Consider whether you want stricter failure modes later (e.g., custom rules), but this is solid as-is.

If you want non-color output in CI logs, add:

-      - id: actionlint
+      - id: actionlint
+        args: ["-color=never"]

43-47: Shellcheck hook is useful; consider enabling -x and tuning ignores

To reduce false positives with sourced files and improve coverage:

-    -   id: shellcheck
+    -   id: shellcheck
+        args: ["-x", "--severity=style"]
+        # Example: ignore known external-sourced files in CI, tweak as needed
+        exclude: '(^|/)(vendor|third_party)/|/etc/os-release'

If tests/smoke.sh sources non-repo files, annotate them with targeted disables (e.g., # shellcheck disable=SC1091) rather than broad excludes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e9b945 and 99d906e.

📒 Files selected for processing (3)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/smoke.sh
  • .github/workflows/redhat-distro-container.yml
🔇 Additional comments (1)
.pre-commit-config.yaml (1)

7-37: Yes — please open and paste the exact rev: strings for each hook so we can update .pre-commit-config.yaml precisely:

  • pre-commit/pre-commit-hooks v6.0.0
  • astral-sh/ruff-pre-commit v0.12.12 (and confirm updated hook IDs: ruff-check/ruff-format)
  • rhysd/actionlint (latest released tag)
  • koalaman/shellcheck-precommit v0.11.0

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (4)
tests/smoke.sh (4)

3-4: Enable fail-fast and inherit ERR traps.

Use -e (and -E for functions) so unexpected command failures stop the run.

-set -uo pipefail
+set -Eeuo pipefail

19-27: Health check is brittle on exact JSON equality.

A single added field breaks equality. Match a stable substring or parse.

-    resp=$(curl -fsS --max-time 2 http://127.0.0.1:8321/v1/health)
-    if [ "$resp" == '{"status":"OK"}' ]; then
+    resp="$(curl -fsS --max-time 2 http://127.0.0.1:8321/v1/health || true)"
+    if printf '%s' "$resp" | grep -Fq '"status":"OK"'; then

35-45: Treat model name as a fixed string.

Avoid regex surprises for models with special chars; also keep quoting consistent.

-  resp=$(curl -fsS --max-time 4 http://127.0.0.1:8321/v1/models)
-  if echo "$resp" | grep -q "$INFERENCE_MODEL"; then
+  resp="$(curl -fsS --max-time 4 http://127.0.0.1:8321/v1/models)"
+  if printf '%s' "$resp" | grep -Fq -- "$INFERENCE_MODEL"; then

48-55: Make inference assertion more tolerant and safe.

  • Quote command substitution;
  • Use case-insensitive fixed search to avoid regex pitfalls;
  • Allow "Green" variants.
-  resp=$(curl -fsS --max-time 6 http://127.0.0.1:8321/v1/openai/v1/chat/completions -H "Content-Type: application/json" -d "{\"model\": \"$INFERENCE_MODEL\",\"messages\": [{\"role\": \"user\", \"content\": \"What color is grass?\"}], \"max_tokens\": 10, \"temperature\": 0.0}")
-  if echo "$resp" | grep -q "green"; then
+  resp="$(curl -fsS --max-time 6 http://127.0.0.1:8321/v1/openai/v1/chat/completions \
+           -H "Content-Type: application/json" \
+           -d "{\"model\": \"$INFERENCE_MODEL\",\"messages\": [{\"role\": \"user\", \"content\": \"What color is grass?\"}], \"max_tokens\": 10, \"temperature\": 0.0}")"
+  if printf '%s' "$resp" | grep -iqF "green"; then
🧹 Nitpick comments (2)
distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1)

3-3: Pin kubernetes to a safe range; verified provider version availability.

-  pip_packages: ["kubernetes", "llama_stack_provider_lmeval==0.2.3"]
+  pip_packages: ["kubernetes>=33.0.0,<34.0.0", "llama_stack_provider_lmeval==0.2.3"]

Confirmed kubernetes client 33.1.0 and llama_stack_provider_lmeval 0.2.3 are available on PyPI.

tests/smoke.sh (1)

70-72: Avoid masking non-zero returns from main.

If a future refactor switches exit 1 to return 1, the trailing exit 0 would hide failures.

-main "$@"
-exit 0
+main "$@"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99d906e and f5b96fb.

📒 Files selected for processing (8)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • distribution/Containerfile (1 hunks)
  • distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • .github/workflows/pre-commit.yml
  • .github/workflows/semantic-pr.yml
  • .pre-commit-config.yaml
  • distribution/Containerfile
  • .github/actions/setup-vllm/action.yml
  • .github/workflows/redhat-distro-container.yml
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)

also add CI linter to pre-commit config

Assisted-by: coderabbitai
Signed-off-by: Nathan Weinberg <[email protected]>
to mirror d/s change and unblock CI

Signed-off-by: Nathan Weinberg <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
.github/workflows/redhat-distro-container.yml (1)

35-37: Skip QEMU for amd64-only builds.

QEMU adds overhead without benefit here; gate it on non-amd64.

-      - name: Set up QEMU
-        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
+      - name: Set up QEMU
+        if: matrix.platform != 'linux/amd64'
+        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0
🧹 Nitpick comments (4)
.github/workflows/redhat-distro-container.yml (4)

3-11: Broaden PR triggers to avoid missed runs.

Include reopened/ready_for_review so CI re-runs when PRs are revived or leave draft.

 on:
   pull_request:
     branches:
       - main
       - rhoai-v*
     types:
-      - opened
-      - synchronize
+      - opened
+      - synchronize
+      - reopened
+      - ready_for_review

25-30: Add a job-level timeout to prevent hung runners.

Set an upper bound for the whole job (network pulls, smoke test).

 jobs:
   build-test-push:
     runs-on: ubuntu-latest
+    timeout-minutes: 45
     strategy:
       matrix:
         platform: [linux/amd64]

74-85: Use docker/metadata-action for cleaner tags and OCI labels.

It keeps tag logic declarative and adds labels (source, ref, revision).

+      - name: Docker meta
+        id: meta
+        uses: docker/metadata-action@8e5442c4ef9f78752691e2e8b5b5f42f53b8b3c0 # v5.5.1
+        with:
+          images: ${{ env.IMAGE_NAME }}
+          tags: |
+            type=sha
+            type=ref,event=branch
+            type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+
       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
-          tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}${{ github.ref == 'refs/heads/main' && format(',{0}:latest', env.IMAGE_NAME) || '' }}  # only update 'latest' tag if push is to the 'main' branch
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
           cache-from: type=gha
           cache-to: type=gha,mode=max

1-2: Set minimal GitHub token permissions.

Helps harden the workflow; Quay creds are separate.

 name: Build, test, and publish Red Hat Distribution Containers
+
+permissions:
+  contents: read
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f5b96fb and 1365d8e.

📒 Files selected for processing (10)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container-build.yml (0 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • distribution/Containerfile (1 hunks)
  • distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1 hunks)
  • distribution/providers.d/remote/safety/trustyai_fms.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/redhat-distro-container-build.yml
🚧 Files skipped from review as they are similar to previous changes (8)
  • .github/workflows/semantic-pr.yml
  • .github/workflows/pre-commit.yml
  • distribution/providers.d/remote/eval/trustyai_lmeval.yaml
  • distribution/providers.d/remote/safety/trustyai_fms.yaml
  • .pre-commit-config.yaml
  • distribution/Containerfile
  • tests/smoke.sh
  • .github/actions/setup-vllm/action.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T17:17:31.486Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#16
File: .github/workflows/redhat-distro-container.yml:79-81
Timestamp: 2025-09-08T17:17:31.486Z
Learning: The llama-stack server expects OpenAI-compatible endpoints at the path /v1/openai/v1/chat/completions (not /v1/openai/chat/completions). This double /v1 pattern is intentional for the llama-stack implementation.

Applied to files:

  • .github/workflows/redhat-distro-container.yml
🔇 Additional comments (4)
.github/workflows/redhat-distro-container.yml (4)

16-18: Good concurrency key.

Group by PR number or ref with cancel-in-progress is appropriate here.


20-23: Image name/env looks fine.

Clear, and tags are handled later.


41-53: Build step looks solid (cache + load).

Buildx cache and load:true for local smoke test are correctly configured.


65-73: Login is properly scoped to push.

No change needed.

to mirror d/s change and unblock CI

Signed-off-by: Nathan Weinberg <[email protected]>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
distribution/providers.d/remote/eval/trustyai_lmeval.yaml (2)

3-3: Pin the Kubernetes client to a compatible range to avoid upstream breakages.

Unpinned kubernetes can introduce CI flakiness and supply-chain risk. Prefer a compatible pin.

-  pip_packages: ["kubernetes", "llama_stack_provider_lmeval==0.2.4"]
+  pip_packages: ["kubernetes~=29.0", "llama_stack_provider_lmeval==0.2.4"]

3-3: If Kubernetes isn’t required at runtime for this eval provider, drop it to slim the image.

If only the operator/K8s repo needs it, consider removing or making it optional to reduce image size and CVE surface.

-  pip_packages: ["kubernetes", "llama_stack_provider_lmeval==0.2.4"]
+  pip_packages: ["llama_stack_provider_lmeval==0.2.4"]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1365d8e and 12fb251.

📒 Files selected for processing (8)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • distribution/Containerfile (1 hunks)
  • distribution/providers.d/remote/eval/trustyai_lmeval.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • .github/workflows/pre-commit.yml
  • distribution/Containerfile
  • .github/actions/setup-vllm/action.yml
  • tests/smoke.sh
  • .pre-commit-config.yaml
  • .github/workflows/redhat-distro-container.yml
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)
🔇 Additional comments (3)
.github/workflows/semantic-pr.yml (1)

12-13: Concurrency key LGTM; correct PR-number-or-ref fallback.

This prevents cross-PR cancellations and aligns with the pattern used elsewhere. No change needed.

distribution/providers.d/remote/eval/trustyai_lmeval.yaml (2)

3-5: Verify LMEvalEvalProviderConfig in llama_stack_provider_lmeval v0.2.4
Docker isn’t available to smoke-test the import—please manually confirm that llama_stack_provider_lmeval.config.LMEvalEvalProviderConfig still exists in version 0.2.4.


3-3: Version consistency confirmed: All references to llama_stack_provider_lmeval are pinned to 0.2.4 in the Containerfile and trustyai_lmeval.yaml; no other occurrences found in workflows or docs.

@nathan-weinberg nathan-weinberg force-pushed the big-smoke branch 4 times, most recently from 3e8df72 to 345e1f3 Compare September 11, 2025 12:46
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
.github/workflows/semantic-pr.yml (1)

15-16: Action can’t report status without extra permissions. Add statuses: write (+ pull-requests: read).

amannn/action-semantic-pull-request needs to write commit statuses and read PRs. Without these, status updates fail.

Apply:

 permissions:
-  contents: read
+  contents: read
+  pull-requests: read
+  statuses: write
🧹 Nitpick comments (5)
.pre-commit-config.yaml (1)

43-47: Tighten ShellCheck: enable source-path resolution and stricter severity

Recommend passing -x so sourced files are resolved and raising severity to surface style issues early.

 -   repo: https://github.com/koalaman/shellcheck-precommit
     rev: v0.11.0
     hooks:
-    -   id: shellcheck
+    -   id: shellcheck
+        args: ["-x", "-S", "style"]
.github/workflows/redhat-distro-container.yml (4)

35-37: Drop QEMU until multi-arch is enabled.

You’re only building linux/amd64; QEMU adds overhead without benefit.

-      - name: Set up QEMU
-        uses: docker/setup-qemu-action@29109295f81e9208d7d86ff1c6c12d2833863392 # v3.6.0

54-57: Consider a step-level timeout for vLLM setup.

If the composite action waits too long on readiness, guard the step.

-      - name: Setup vllm for image test
+      - name: Setup vllm for image test
+        timeout-minutes: 15
         id: vllm
         uses: ./.github/actions/setup-vllm

58-65: Add a step timeout to prevent hangs in the smoke test.

Even with script traps, enforce a ceiling here.

-      - name: Smoke test image
+      - name: Smoke test image
+        timeout-minutes: 10
         id: smoke-test
         shell: bash
         env:
           INFERENCE_MODEL: meta-llama/Llama-3.2-1B-Instruct
           VLLM_URL: http://host.docker.internal:8000/v1
         run: ./tests/smoke.sh

75-86: Use docker/metadata-action for cleaner tags and OCI labels.

You already gate latest; metadata-action will also add labels and handle branch/sha tags neatly.

+      - name: Docker meta
+        id: meta
+        uses: docker/metadata-action@8e5442c4ef9f78752691e2e8b5b5f42f53b8b3c0 # v5.5.1
+        with:
+          images: ${{ env.IMAGE_NAME }}
+          tags: |
+            type=sha
+            type=ref,event=branch
+            type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+
       - name: Publish image to Quay.io
         id: publish
         if: github.event_name == 'push'
         uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0
         with:
           context: .
           file: distribution/Containerfile
           platforms: ${{ matrix.platform }}
           push: true
-          tags: ${{ env.IMAGE_NAME }}:${{ github.sha }}${{ github.ref == 'refs/heads/main' && format(',{0}:latest', env.IMAGE_NAME) || '' }}  # only update 'latest' tag if push is to the 'main' branch
+          tags: ${{ steps.meta.outputs.tags }}
+          labels: ${{ steps.meta.outputs.labels }}
           cache-from: type=gha
           cache-to: type=gha,mode=max
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e8df72 and 345e1f3.

📒 Files selected for processing (6)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/pre-commit.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .github/workflows/semantic-pr.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • .github/workflows/pre-commit.yml
  • tests/smoke.sh
  • .github/actions/setup-vllm/action.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T17:17:31.486Z
Learnt from: nathan-weinberg
PR: opendatahub-io/llama-stack-distribution#16
File: .github/workflows/redhat-distro-container.yml:79-81
Timestamp: 2025-09-08T17:17:31.486Z
Learning: The llama-stack server expects OpenAI-compatible endpoints at the path /v1/openai/v1/chat/completions (not /v1/openai/chat/completions). This double /v1 pattern is intentional for the llama-stack implementation.

Applied to files:

  • .github/workflows/redhat-distro-container.yml
🔇 Additional comments (5)
.pre-commit-config.yaml (1)

38-42: Actionlint added — confirm expectations for shellcheck on workflow run: blocks

Actionlint won’t run ShellCheck on YAML run: steps in the pre-commit context unless ShellCheck is available in the same environment (pre-commit hooks run in isolated envs). Your separate ShellCheck hook won’t satisfy that for actionlint. If you want run: blocks linted, run actionlint in CI where shellcheck is installed, or accept that those checks are skipped locally.

.github/workflows/semantic-pr.yml (2)

12-12: Concurrency key fallback looks good.

The ${{ github.event.pull_request.number || github.ref }} fallback avoids empty groups on non-PR events. LGTM.


23-23: Good: action pinned by commit SHA.

Using a pinned SHA for amannn/action-semantic-pull-request is the right supply‑chain hardening.

.github/workflows/redhat-distro-container.yml (2)

41-53: Build caching + load flow looks solid.

Cache config and load=true for the smoke test are correct. Nice.


58-65: Verify host.docker.internal resolution from inside the llama-stack container.

On Linux runners this can fail unless the container run adds --add-host=host.docker.internal:host-gateway. Ensure tests/smoke.sh sets this (or equivalent) so LLS can reach vLLM at the provided VLLM_URL.

@nathan-weinberg nathan-weinberg force-pushed the big-smoke branch 3 times, most recently from 12032de to 4a2eefe Compare September 11, 2025 13:48
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
.pre-commit-config.yaml (3)

38-42: Scope actionlint to workflow files to avoid unnecessary runs

Limit this hook to GitHub Actions workflow paths so it only triggers when relevant.

   hooks:
-      - id: actionlint
+      - id: actionlint
+        files: ^\.github/workflows/.*\.ya?ml$

43-47: Enable ShellCheck’s source-following for more accurate results

Add -x so sourced files are resolved; without it you’ll get SC1091 noise and false negatives.

   hooks:
-    -   id: shellcheck
+    -   id: shellcheck
+        args: ["-x"]

48-60: Confirm intent: heavy local “Distribution Build” hook still present and always runs

AI summary suggested this block was removed, but it’s still here with always_run: true. If that’s unintentional—or if the build is heavy—gate it to a manual stage to keep local commits snappy while retaining the tool.

       - id: pkg-gen
         name: Distribution Build
         entry: ./distribution/build.py
         language: python
-        pass_filenames: false
-        require_serial: true
-        always_run: true
+        pass_filenames: false
+        require_serial: true
+        always_run: false
+        stages: [manual]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12032de and 4a2eefe.

📒 Files selected for processing (4)
  • .github/actions/setup-vllm/action.yml (1 hunks)
  • .github/workflows/redhat-distro-container.yml (1 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • tests/smoke.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/smoke.sh
  • .github/actions/setup-vllm/action.yml
  • .github/workflows/redhat-distro-container.yml
⏰ 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). (1)
  • GitHub Check: build-test-push (linux/amd64)

Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, good job!

nathan-weinberg and others added 5 commits September 11, 2025 10:05
Co-authored-by: Derek Higgins <[email protected]>
Signed-off-by: Nathan Weinberg <[email protected]>
Signed-off-by: Nathan Weinberg <[email protected]>
also add shell linter to pre-commit config

Assisted-by: coderabbitai
Signed-off-by: Nathan Weinberg <[email protected]>
@nathan-weinberg nathan-weinberg merged commit 4d50ebe into opendatahub-io:main Sep 11, 2025
4 checks passed
@nathan-weinberg nathan-weinberg deleted the big-smoke branch September 11, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants