Add CI smoke test for container image#167
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new ChangesSmoke Test Addition
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Podman
participant Make
GitHubActions->>GitHubActions: download claudio-amd64 artifact
GitHubActions->>Podman: load image and retag as smoke-test:latest
GitHubActions->>Make: run make smoke-test IMAGE_NAME=smoke-test:latest
Make->>Podman: run container with bash test harness
Podman-->>Make: PASS/FAIL results
Make-->>GitHubActions: exit status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
rishupk
force-pushed
the
aipcc-17957/smoke-test
branch
from
073c9aa to
d6ac50b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
Makefile (2)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailure output is fully suppressed, hindering CI debugging.
check()redirects the evaluated command's stdout/stderr to/dev/null, so aFAIL: claude --versiongives no clue why (missing binary vs. crash vs. wrong PATH). This also means the actualclaude/gcloudversions the PR description says are being "checked" never appear in the CI logs.♻️ Suggested fix to capture and print output on failure
check() { \ - if eval "$$2" > /dev/null 2>&1; then \ - echo "PASS: $$1"; \ + out=$$(eval "$$2" 2>&1); \ + if [ $$? -eq 0 ]; then \ + echo "PASS: $$1"; \ else \ echo "FAIL: $$1"; \ + echo "$$out"; \ FAIL=1; \ fi; \ }; \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 80 - 87, The check() helper in the Makefile is swallowing all stdout/stderr from the evaluated command, so failed checks like claude or gcloud provide no diagnostic output and successful version checks never appear in CI logs. Update check() so it captures command output, prints it when the command fails, and still reports PASS/FAIL clearly; use the existing check() shell function and its eval-based command execution as the place to fix this.
76-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the inline harness into a standalone script.
checkmake flags the body length (23 lines vs. the 5-line guideline). Embedding a multi-check Bash harness as a single-quoted
-cone-liner inside a Makefile recipe is hard to read, diff, and lint (e.g., shellcheck can't easily analyze it in this form).♻️ Suggested approach
Move the harness to e.g.
scripts/smoke-test.sh, copy/mount it into the container (or bake it in), and have the Makefile target just invoke it:-smoke-test: - `@echo` "=== Smoke testing $(IMAGE_NAME) ===" - ${CONTAINER_MANAGER} run --rm --entrypoint bash $(IMAGE_NAME) -c '\ - FAIL=0; \ - ... - ' +smoke-test: + `@echo` "=== Smoke testing $(IMAGE_NAME) ===" + ${CONTAINER_MANAGER} run --rm --entrypoint bash -v $(CURDIR)/scripts/smoke-test.sh:/tmp/smoke-test.sh:ro $(IMAGE_NAME) -c 'bash /tmp/smoke-test.sh'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 76 - 100, The smoke-test target in Makefile embeds a long inline Bash harness inside the container run command, which makes it hard to read and maintain. Extract the multi-check logic from the smoke-test recipe into a standalone script (for example a dedicated smoke-test shell script), keep the Makefile target as a thin wrapper that invokes that script through $(CONTAINER_MANAGER) run, and preserve the existing checks for claude, gcloud, required binaries, entrypoint.sh, the plugin list, and executable scripts.Source: Linters/SAST tools
.github/workflows/build.yml (3)
80-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile image-tag detection via
podman images | head -1.Relying on output ordering of
podman imagesto find "the" loaded image is indirect and could mis-tag if any pre-existing/base image is present in local storage.podman loadalready prints the loaded image reference directly to stdout.♻️ Suggested fix
- name: Load image run: | - podman load -i claudio-amd64.tar - LOADED=$(podman images --format '{{.Repository}}:{{.Tag}}' | head -1) + LOADED=$(podman load -i claudio-amd64.tar | sed -n 's/^Loaded image: //p') podman tag "${LOADED}" smoke-test:latest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 80 - 84, The image tagging step in the build workflow is using fragile `podman images | head -1` detection, which can pick the wrong local image. Update the “Load image” step to use the image reference emitted directly by `podman load` instead, then tag that returned reference as `smoke-test:latest`. Keep the change localized to the workflow step that currently shells out with `podman load`, `podman images`, and `podman tag`.
64-88: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an explicit
permissionsblock to the job.Static analysis flags this job as running with default (broad) permissions. Since it only checks out code, downloads an artifact, and runs local commands, it should be scoped to read-only.
🔒 Suggested fix
smoke-test: name: Smoke test needs: build-and-upload runs-on: ubuntu-24.04 + permissions: + contents: read steps:As per static analysis hints, zizmor flagged
overly broad permissions (excessive-permissions): default permissions used due to no permissions: blockfor lines 64-87.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 64 - 88, Add an explicit job-level permissions block to the smoke-test job in build.yml so it does not inherit the default broad GitHub Actions permissions. Keep the scope read-only for the actions used here (checkout, artifact download, and local smoke-test commands), and place the new permissions stanza alongside the smoke-test job definition so it is easy to find and maintain.Source: Linters/SAST tools
64-78: 🧹 Nitpick | 🔵 TrivialSmoke test only covers the amd64 image.
needs: build-and-uploadwaits on both matrix legs, but only theclaudio-amd64artifact is downloaded and validated; thearm64build is never smoke-tested end-to-end. Likely an accepted limitation of amd64-only hosted runners, worth confirming this is intentional and, if so, a short comment noting why would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 64 - 78, The smoke-test job only validates the claudio-amd64 artifact even though build-and-upload runs both matrix legs, so please clarify the intentional amd64-only coverage in the workflow. Update the smoke-test job in build.yml around the smoke-test and Download amd64 image steps to add a short comment explaining why arm64 is not end-to-end smoke-tested on hosted runners, or adjust the job if broader coverage is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 80-84: The image tagging step in the build workflow is using
fragile `podman images | head -1` detection, which can pick the wrong local
image. Update the “Load image” step to use the image reference emitted directly
by `podman load` instead, then tag that returned reference as
`smoke-test:latest`. Keep the change localized to the workflow step that
currently shells out with `podman load`, `podman images`, and `podman tag`.
- Around line 64-88: Add an explicit job-level permissions block to the
smoke-test job in build.yml so it does not inherit the default broad GitHub
Actions permissions. Keep the scope read-only for the actions used here
(checkout, artifact download, and local smoke-test commands), and place the new
permissions stanza alongside the smoke-test job definition so it is easy to find
and maintain.
- Around line 64-78: The smoke-test job only validates the claudio-amd64
artifact even though build-and-upload runs both matrix legs, so please clarify
the intentional amd64-only coverage in the workflow. Update the smoke-test job
in build.yml around the smoke-test and Download amd64 image steps to add a short
comment explaining why arm64 is not end-to-end smoke-tested on hosted runners,
or adjust the job if broader coverage is intended.
In `@Makefile`:
- Around line 80-87: The check() helper in the Makefile is swallowing all
stdout/stderr from the evaluated command, so failed checks like claude or gcloud
provide no diagnostic output and successful version checks never appear in CI
logs. Update check() so it captures command output, prints it when the command
fails, and still reports PASS/FAIL clearly; use the existing check() shell
function and its eval-based command execution as the place to fix this.
- Around line 76-100: The smoke-test target in Makefile embeds a long inline
Bash harness inside the container run command, which makes it hard to read and
maintain. Extract the multi-check logic from the smoke-test recipe into a
standalone script (for example a dedicated smoke-test shell script), keep the
Makefile target as a thin wrapper that invokes that script through
$(CONTAINER_MANAGER) run, and preserve the existing checks for claude, gcloud,
required binaries, entrypoint.sh, the plugin list, and executable scripts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79ff50c1-1ebc-4273-9d83-be1162777dc5
📒 Files selected for processing (2)
.github/workflows/build.ymlMakefile
- Add smoke-test job to build.yml that runs after image build - Add make smoke-test target for local use - Checks: claude, gcloud, key binaries, entrypoint syntax, plugin, helper scripts Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Rishabh Kothari <rkothari@redhat.com>
rishupk
force-pushed
the
aipcc-17957/smoke-test
branch
from
d6ac50b to
df61385
Compare
2 participants
Summary
smoke-testjob to the Build workflow that runs after image build on every PR and push to mainmake smoke-testMakefile target for local useCloses AIPCC-17957
Co-Authored-By: Claude noreply@anthropic.com
Signed-off-by: Rishabh Kothari rkothari@redhat.com
Summary by CodeRabbit