feat: Create ODH distro image smoke test for Vertex AI#139
feat: Create ODH distro image smoke test for Vertex AI#139Artemon-line wants to merge 47 commits intomainfrom
Conversation
- Added a step to check for Vertex AI recordings in the integration tests workflow, allowing conditional execution of Vertex AI tests based on the presence of recordings. - Updated the integration tests script to dynamically select the text model based on the environment, supporting both Vertex AI and vllm-inference providers. This improves the testing process by enabling the use of recorded data for Vertex AI, ensuring more reliable and efficient test runs. Signed-off-by: Artemy <ahladenk@redhat.com>
- Removed unnecessary blank lines in the live tests workflow YAML file for better readability. - Updated the condition for the `create-pr` job to use `needs.live-tests.result` instead of `needs.live-tests.outcome`. - Added instructions for setting up Podman for pre-commit hooks in the GitHub Actions testing guide. - Included a section on fixing SELinux context for Fedora/RHEL users in the documentation. - Enhanced the live tests guide to clarify the process of including recordings in pull requests. These changes improve the clarity and functionality of the testing workflows and documentation. Signed-off-by: Artemy <ahladenk@redhat.com>
- Removed redundant environment variable settings in the live tests workflow, replacing them with direct secret references for improved security. - Simplified the Docker run command by using a fixed container name instead of an environment variable. - Updated the local testing script to verify GCP authentication before proceeding, ensuring smoother execution. - Enhanced the live tests guide by clarifying prerequisites and streamlining instructions for running tests with VLLM and Vertex AI. These changes improve the clarity, security, and functionality of the testing workflows and documentation. Signed-off-by: Artemy <ahladenk@redhat.com>
- Removed the optional environment variable setting for VERTEX_AI_LOCATION in the live tests guide to streamline instructions. - Added a log statement in the local testing script to confirm the use of the Vertex AI provider and its model. These changes enhance the clarity and usability of the documentation and scripts for running live tests. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: Artemy <ahladenk@redhat.com>
WalkthroughAdds a composite GitHub Action to start and wait for a Llama Stack container, separates vLLM startup from readiness polling, introduces a "Run integration tests" workflow, refactors the Red Hat container publish workflow to use workflow_run/workflow_dispatch branching, updates tests and docs, and converts the README build badge to a workflow link. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GH as GitHub
participant Runner as Actions Runner
participant Docker as Docker Engine
participant VLLM as vLLM
participant Llama as Llama Stack
participant Vertex as Vertex AI
participant Tests as Test Runner
Dev->>GH: push / PR / workflow_dispatch
GH->>Runner: start workflow
Runner->>Runner: prepare tools (buildx/qemu/etc.)
alt vLLM path
Runner->>Docker: docker run vLLM (detached)
Runner->>VLLM: poll /v1/health (60 tries)
end
alt Vertex creds present
Runner->>Vertex: authenticate / mount creds
end
alt dispatch builds image
Runner->>Docker: build Llama Stack image
Docker-->>Runner: image ready
end
Runner->>Docker: docker run Llama Stack (env, mounts, --net=host, ports)
Runner->>Llama: poll /v1/health (up to 60 attempts)
Llama-->>Runner: health OK
Runner->>Tests: run smoke & integration tests (vLLM)
alt Vertex creds present
Runner->>Tests: run Vertex AI integration tests
end
Runner->>Runner: collect logs, upload artifacts, cleanup
Runner-->>GH: workflow result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Artemon-line
changed the title
Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: Artemy <ahladenk@redhat.com>
Signed-off-by: Artemy <ahladenk@redhat.com>
- Added `shell: bash` to the `live-tests.yml` and `redhat-distro-container.yml` workflows to ensure consistent execution environment for scripts. This change enhances the reliability of the workflows by explicitly defining the shell used for running commands. Signed-off-by: [Your Name] <your.email@example.com> Signed-off-by: Artemy <ahladenk@redhat.com>
Signed-off-by: Artemy <ahladenk@redhat.com>
…tion into RHAIENG-1793-Create-ODH-distro-image-smoke-test-for-Vertex-AI Signed-off-by: Artemy <ahladenk@redhat.com>
This change ensures that Vertex AI tests are only executed when recordings are present, streamlining the testing process. Signed-off-by: Artemy <ahladenk@redhat.com>
Signed-off-by: Artemy <ahladenk@redhat.com>
ktdreyer
left a comment
ktdreyer
left a comment
There was a problem hiding this comment.
Please rebase this on main so that we can try it out!
I've posted some initial comments in the meantime.
|
This pull request has merge conflicts that must be resolved before it can be merged. @Artemon-line please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
- Enhanced the README to include a clickable build badge for better visibility. - Removed the obsolete `record-integration-tests.yml` workflow to streamline CI processes. - Refactored the `redhat-distro-container.yml` workflow to consolidate testing steps and improve clarity. - Updated the live tests guide to reflect changes in running tests with vLLM and Vertex AI, including clearer instructions and prerequisites. These changes aim to simplify the CI/CD pipeline and improve documentation for running integration tests. Signed-off-by: Artemy <ahladenk@redhat.com>
…tion into RHAIENG-1793-Create-ODH-distro-image-smoke-test-for-Vertex-AI
…er builds and add integration tests
|
This pull request has merge conflicts that must be resolved before it can be merged. @Artemon-line please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
…update integration tests workflow to streamline model inference testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
tests/smoke.sh (1)
46-59: The hardcoded "green" expectation intest_model_openai_inferencewill fail for Vertex AI models with different response formats.The function sends "What color is grass?" to
$INFERENCE_MODEL(which includesvertexai/google/gemini-2.0-flashper the CI/CD workflow) and greps for "green". While both Qwen and Gemini will likely mention grass as green, response phrasing differs across models—Gemini might return "green color" or "the colour of grass is green" instead of containing the word "green" directly. For robust multi-model testing, either parameterize the expected response based on the model, or use a more generic success criterion like checking for non-empty completion or absence of error status.
♻️ Duplicate comments (1)
.github/workflows/run-integration-tests.yml (1)
146-158: Smoke test loop runs both vLLM and Vertex AI models, but inference test may fail for Vertex AI.Lines 146–158 loop over both
VLLM_INFERENCE_MODELandVERTEX_AI_INFERENCE_MODEL, runningsmoke.shfor each. However,smoke.sh'stest_model_openai_inferencefunction (line 48 in smoke.sh) hardcodes a "What color is grass?" query and expects "green" in the response. This is specific to vLLM/Qwen and will likely fail for Vertex AI/Gemini. See related issue in smoke.sh review.This issue originates from the smoke.sh file. Both files must be fixed together: smoke.sh must be updated to support multiple model types, and/or this workflow must conditionally run smoke tests only for compatible models.
🧹 Nitpick comments (1)
.github/workflows/run-integration-tests.yml (1)
134-140: Remove redundant health check; setup-llama-stack already validates readiness.The "Verify deployment" step (lines 134–140) repeats the health check already performed by the
setup-llama-stackaction (line 122–132). Removing this duplicate step reduces noise and makes the workflow intent clearer.- - name: Verify deployment - if: github.event_name != 'workflow_dispatch' - shell: bash - run: | - echo "Verifying deployed Llama Stack instance..." - curl -fsS http://127.0.0.1:8321/v1/health || exit 1 - echo "Deployment verified successfully!" -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/setup-vllm/action.yml(2 hunks).github/workflows/run-integration-tests.yml(1 hunks)tests/smoke.sh(3 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). (2)
- GitHub Check: test
- GitHub Check: Summary
🔇 Additional comments (4)
.github/actions/setup-vllm/action.yml (2)
6-22: Verify vLLM health endpoint exists and is reachable.The "Start VLLM" step launches the container non-blocking with
--net=host, enabling access tohttp://localhost:8000/health. Confirm that this health endpoint exists in the vLLM image (quay.io/higginsd/vllm-cpu:65393ee064-qwen3) and responds as expected before merging.
24-38: Good readiness validation pattern.The "Wait for VLLM to be ready" step cleanly separates startup from readiness polling, with appropriate timeout (120 sec), diagnostic output on failure, and exit-code semantics. This is a solid approach for async startup validation.
tests/smoke.sh (1)
7-24: Verify exact health response format.Line 16 performs a strict equality check on the health endpoint response:
[ "$resp" != '{"status":"OK"}' ]. Confirm that the Llama Stack health endpoint consistently returns exactly this JSON format (no whitespace variations, no additional fields) to avoid false failures..github/workflows/run-integration-tests.yml (1)
85-99: Clarify workflow_dispatch build and test execution intent.For
workflow_dispatchevents, the Containerfile is generated (lines 85–99) but the image is not built (line 102 condition isgithub.event_name != 'workflow_dispatch'). As a result, for workflow_dispatch:
- Checkout, setup tools (uv, QEMU, Buildx), and Containerfile generation run.
- No image build, no container startup, no tests run.
This differs from the
scheduleevent, which generates the Containerfile and builds/tests the image. Clarify whether workflow_dispatch is intended to only generate/inspect the Containerfile, or whether the build and test steps should also run.This was noted in a prior review comment: "If this step is triggered for
workflow_dispatchwhen is the image built, given L100 excludes that trigger for the following build step?"
…p, removing unnecessary conditions, and cleaning up container resources post-tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/run-integration-tests.yml(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). (2)
- GitHub Check: test
- GitHub Check: Summary
🔇 Additional comments (3)
.github/workflows/run-integration-tests.yml (3)
81-83: Verify setup-llama-stack handles vLLM readiness when startup is non-blocking.The vLLM container is started non-blocking (line 81-83), then the Llama Stack container is started immediately after Google Cloud authentication (line 121-131). The
setup-llama-stackaction must wait for vLLM to be ready before deploying, since it requires the vLLM endpoint athttp://localhost:8000/v1(line 129).Please confirm that the
setup-llama-stackaction (referenced at line 123) includes proper readiness checks for vLLM before deploying the Llama Stack container. Ensure there are no race conditions between vLLM startup completing and Llama Stack attempting to configure it.Additionally, verify this behavior against the past review concern: "Why is vLLM no longer blocking? Are there scenarios we would want to test even if the vLLM setup fails?"
Also applies to: 121-131
121-131: Verify model parameter format matches setup-llama-stack action expectations.The
inference_modelparameter includes a provider prefix (e.g.,vllm-inference/Qwen/Qwen3-0.6B). Confirm that thesetup-llama-stackaction expects this format and that it's correctly used in the Llama Stack deployment configuration.Additionally, verify that this parameter naming and format is consistent across:
- Line 127: passed to
setup-llama-stackasinference_model- Line 155: used in integration tests as
INFERENCE_MODELenvironment variable- Line 169: used in Vertex AI integration tests as
INFERENCE_MODELenvironment variableIf the formats differ, ensure proper transformation is applied in the action or test scripts.
113-119: Verify credentials file path consistency between auth and Llama Stack action.The
google-github-actions/authaction creates a credentials file whencreate_credentials_file: trueis set, typically at${GITHUB_WORKSPACE}/gha-creds-*.json. Thesetup-llama-stackaction should use theGOOGLE_APPLICATION_CREDENTIALSenvironment variable that the auth action sets, or explicitly handle the credentials file path.Confirm that the
setup-llama-stackaction correctly receives and uses the GCP credentials file. Per the PR objectives, credentials should be mounted at/run/secrets/gcp-credentialsin the container. Verify the action handles this mount correctly and that the path is consistent with local Podman setup mentioned in test plan.
…pt to streamline testing process
…nditionally and conditionally execute Vertex AI tests based on credential availability
…est-for-Vertex-AI
…ining image tags based on event type, improving tagging logic for better version management
…tex-AI' of github.com:opendatahub-io/llama-stack-distribution into RHAIENG-1793-Create-ODH-distro-image-smoke-test-for-Vertex-AI
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/redhat-distro-container.yml(3 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). (2)
- GitHub Check: test
- GitHub Check: Summary
🔇 Additional comments (2)
.github/workflows/redhat-distro-container.yml (2)
93-105: Excellent implementation of the "Determine image tags" step—directly addresses the past review suggestion.The tag determination logic is now cleanly extracted into its own step, making the tagging strategy explicit and maintainable. The regex pattern on line 101 correctly matches
rhoai-v*branches using bash's[[operator with extended pattern matching.For
workflow_dispatchevents, the formatsource-{commit}-{sha}aligns with the IMAGE_TAG computed at line 47 for output purposes. For other events (workflow_run), conditional tagging correctly adds:latestfor main and:{branch}-latestfor release branches.
51-55: Verify checkout ref correctness for workflow_run events.The checkout ref at line 55 uses
github.event.workflow_run.head_shaforworkflow_runevents. Confirm that this ref correctly corresponds to the exact commit that triggered the upstream "Run integration tests" workflow, rather than the current branch HEAD. This ensures the built container matches the version that passed tests.
| if: | | ||
| (github.event_name == 'workflow_run' && github.event.workflow_run.conclusion == 'success') || | ||
| github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "redhat-distro-container.yml" -type fRepository: opendatahub-io/llama-stack-distribution
Length of output: 130
🌐 Web query:
GitHub workflow_run event head_sha availability when conclusion success
💡 Result:
Short answer: github.event.workflow_run.head_sha is usually present for workflow_run events that originate from a commit (push/PR), but it is not guaranteed (can be empty/null) and the event's conclusion field can also sometimes be missing. For reliable values fetch the workflow run via the Actions API (GET /repos/:owner/:repo/actions/runs/:run_id) and use that data (or the commit->pulls API) instead.
Key refs:
- Official docs: workflow_run event payload / behavior. [1]
- Community reports that github.event.workflow_run.conclusion can be empty and the recommended workaround is to call the API to read the run. [2]
- Community examples showing use of github.event.workflow_run.head_sha but noting it’s not always sufficient for finding PR base/head; users query the API or search PRs by commit SHA. [3]
If you want, I can show a minimal workflow step that fetches the canonical head_sha and conclusion from the API (with octokit/request-action or gh).
🏁 Script executed:
cat -n .github/workflows/redhat-distro-container.yml | head -60Repository: opendatahub-io/llama-stack-distribution
Length of output: 3227
Confirm that head_sha fallback behavior is intentional; GitHub does not guarantee head_sha in workflow_run events even when conclusion == 'success'.
The web search confirms that github.event.workflow_run.head_sha is not guaranteed to be present for workflow_run events, and the GitHub Actions API should be used for reliable data. This means the fallback to 'main' at line 44 could be triggered unexpectedly when head_sha is missing, rather than only in edge cases. Additionally, head_sha is used in multiple locations (lines 23, 44, 47, 55), so the fallback behavior should be clearly documented. Consider either:
- Adding an explicit check to verify head_sha presence before proceeding
- Using the GitHub Actions API to fetch the canonical run data
- Documenting why the
'main'fallback is acceptable for your use case
🤖 Prompt for AI Agents
.github/workflows/redhat-distro-container.yml around lines 37 to 39: the
workflow currently assumes github.event.workflow_run.head_sha may be missing and
falls back to 'main' later, which can be triggered unexpectedly; update the job
to explicitly verify head_sha before using it (and fail/skip the job or require
a fallback policy), or call the GitHub Actions API to fetch the canonical
workflow_run.head_sha when github.event_name == 'workflow_run', and ensure every
place that uses head_sha (lines 23, 44, 47, 55) reads from the verified value;
alternatively, if the 'main' fallback is acceptable, add an explicit comment
documenting why that fallback is safe and add a guard that logs a warning when
head_sha is absent.
|
This PR is getting big. It implements multiple things besides vertex testing, and we're 52 comments in. If this is working, I propose we merge it and iterate, so that we make further reviews easier. Smaller changes are easier to review. My suggestions for future improvements:
Again, these are future suggestions for improvement. I'm approving this PR today as-is. |
…est-for-Vertex-AI
…est-for-Vertex-AI
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/redhat-distro-container.yml (1)
23-23: Inconsistent fallback behavior forhead_shaacross env and checkout steps; concurrency group may mis-assign workflow_run events ifhead_shais null.The workflow uses
github.event.workflow_run.head_shain multiple places with different fallbacks:
- Line 23 (concurrency group): Falls back to
github.refifhead_shais missing; if it evaluates to empty, the concurrency group becomes{workflow}-workflow_run-, potentially grouping unrelated workflow_run events.- Line 44 (
LLAMA_STACK_COMMIT_SHA): Falls back to'main'- Line 47 (
IMAGE_TAG): Falls back togithub.sha- Line 55 (checkout ref): Falls back to
github.refThis inconsistency means that if
head_shais missing, the checked-out code, built commit SHA, and concurrency group may not align. The past review flagged thathead_shais not guaranteed by GitHub. Consider:
- Documenting the fallback policy: Add a comment explaining why these different fallbacks are acceptable and what behavior each represents.
- Adding explicit validation: Log a warning or fail-fast if
head_shais missing in workflow_run events, rather than silently falling back.- Unifying fallbacks: If feasible, use consistent fallback values across all uses (e.g., always default to
github.refor always call the GitHub Actions API to fetch canonical run data).Also applies to: 44-44, 47-47, 55-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/redhat-distro-container.yml(3 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). (2)
- GitHub Check: test
- GitHub Check: Summary
🔇 Additional comments (3)
.github/workflows/redhat-distro-container.yml (3)
37-39: Job conditional logic is clear and correct.The conditional correctly gates the build-and-publish job to only run when tests pass (workflow_run + success) or on manual dispatch.
93-105: Tag determination logic is correct and addresses past review suggestion.The step properly extracts tag logic into a separate, readable block as suggested in the past review. The bash logic correctly branches on event type and handles the main/rhoai-v* tagging rules.
107-116: Build configuration is appropriate.The use of
docker/build-push-action, platforms constraint, and GitHub Actions Cache are standard and correct. Thetagsinput referencesIMAGE_TAGSwhich is populated by the "Determine image tags" step.
nathan-weinberg
left a comment
nathan-weinberg
left a comment
There was a problem hiding this comment.
Do you have any evidence these changes are functional?
Frankly, I think this PR is trying to do way too much - I'm not confident this will work on merge, and despite being labeled as a "test" addition you are doing some massive refactors here - while I'm not against it, this needs to be split up.
| LLAMA_STACK_COMMIT_SHA: ${{ github.event.inputs.llama_stack_commit_sha || 'main' }} | ||
| strategy: | ||
| matrix: | ||
| platform: [linux/amd64] # TODO: enable other arch once all pip packages are available. |
There was a problem hiding this comment.
Why is this removed?
| with: | ||
| python-version: 3.12 | ||
| version: 0.7.6 | ||
| enable-cache: false |
| context: . | ||
| file: distribution/Containerfile | ||
| platforms: ${{ matrix.platform }} | ||
| platforms: linux/amd64 |
There was a problem hiding this comment.
Why have you removed the matrix?
| name: Build, test, and publish Red Hat Distribution Containers | ||
| name: Build and publish Red Hat Distribution Containers | ||
|
|
||
| on: |
There was a problem hiding this comment.
There are several mentions of workflow_dispatch later in this file, but it's not exposed as an option here? Unless I am missing something?
|
|
||
| - name: Output custom build information | ||
| if: contains(fromJSON('["workflow_dispatch", "schedule"]'), github.event_name) | ||
| if: github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
Why only for this event?
| with: | ||
| python-version: 3.12 | ||
| version: 0.7.6 | ||
| enable-cache: false |
| echo "Running smoke test for vLLM model: $VLLM_INFERENCE_MODEL" | ||
| INFERENCE_MODEL="$VLLM_INFERENCE_MODEL" ./tests/smoke.sh |
There was a problem hiding this comment.
Can't we just use the same env var names? This seems unnecessarily complex
| # ============================================ | ||
|
|
||
| - name: Gather logs and debugging information | ||
| if: always() && github.event_name != 'workflow_dispatch' |
There was a problem hiding this comment.
You're using always() but also a condition?
| fi | ||
|
|
||
| - name: Upload logs as artifacts | ||
| if: always() && github.event_name != 'workflow_dispatch' |
There was a problem hiding this comment.
always and a condition?
| retention-days: 7 | ||
|
|
||
| - name: Cleanup containers | ||
| if: always() && github.event_name != 'workflow_dispatch' |
There was a problem hiding this comment.
always and a condition?
|
Also, 44 commits is crazy and needs to be squashed |
|
This pull request has merge conflicts that must be resolved before it can be merged. @Artemon-line please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
…tex-AI' of github.com:opendatahub-io/llama-stack-distribution into RHAIENG-1793-Create-ODH-distro-image-smoke-test-for-Vertex-AI
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/live-tests-guide.md (1)
44-44: INFERENCE_MODEL format contradicts prior review feedback: clarify whether provider prefix should be included.The documentation provides examples with provider prefixes (e.g.,
vllm-inference/Qwen/Qwen3-0.6Bat line 44 andvertexai/google/gemini-2.0-flashat line 62), and line 69 explicitly states the script expects this format. However, a prior review comment flagged these exact examples as incorrect and recommended removing the provider prefix. This discrepancy needs resolution.If the prefix should be removed, update all examples at lines 44, 62, 214, and 279 to match the intended format. If the prefix is correct, ensure this aligns with how
distribution/run.yamlstructures provider and model identifiers.Also applies to: 62-62, 69-69, 214-214, 279-279
.github/workflows/redhat-distro-container.yml (1)
37-47: Confirm that head_sha fallback behavior is intentional and acceptable for your workflow reliability.The workflow uses
github.event.workflow_run.head_shain multiple locations (lines 23, 44, 47, 55) with fallbacks to'main'orgithub.sha. A prior review flagged thatworkflow_runevent payloads do not guaranteehead_shais present, even whenconclusion == 'success'.If
head_shais missing and the fallback to'main'is triggered, the workflow will build and publish an image from the main branch instead of the commit that triggered the test workflow. This could result in publishing an untested image to production.Recommended actions:
- Explicit verification: Add a check to verify
head_shais present before proceeding; fail or warn if missing- Use GitHub API: Call the Actions API to fetch the canonical workflow run data including a verified
head_sha- Document fallback policy: If the
'main'fallback is acceptable for your use case, add explicit comments explaining why and document when this fallback may be triggered
🧹 Nitpick comments (2)
docs/live-tests-guide.md (2)
227-227: Hyphenate compound adjective.Line 227 should read
log-gatheringinstead oflog gatheringto form a compound adjective modifying "steps".
169-169: Avoid emphasis in place of headings (markdown style).Lines 169 and 194 use emphasis (
**Standard Pattern**,**Exception**) to mark section-like content. Consider converting these to proper markdown headings (#### Standard Pattern,#### Exception) for better document structure and accessibility.Also applies to: 194-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/redhat-distro-container.yml(4 hunks).github/workflows/run-integration-tests.yml(1 hunks)docs/live-tests-guide.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/live-tests-guide.md
[uncategorized] ~10-~10: The official name of this software platform is spelled with a capital “H”.
Context: ...th providers are tested sequentially in .github/workflows/run-integration-tests.yml. A...
(GITHUB)
[uncategorized] ~73-~73: The official name of this software platform is spelled with a capital “H”.
Context: ...ni-2.0-flash). ## CI/CD Workflow The .github/workflows/run-integration-tests.yml` wo...
(GITHUB)
[uncategorized] ~73-~73: The official name of this software platform is spelled with a capital “H”.
Context: ...ts for both providers sequentially. The .github/workflows/redhat-distro-container.yml ...
(GITHUB)
[uncategorized] ~126-~126: The official name of this software platform is spelled with a capital “H”.
Context: ...dd Environment Variables Location: .github/workflows/run-integration-tests.yml, i...
(GITHUB)
[uncategorized] ~151-~151: The official name of this software platform is spelled with a capital “H”.
Context: ... Configuration Variables Location: .github/workflows/run-integration-tests.yml, i...
(GITHUB)
[uncategorized] ~157-~157: The official name of this software platform is spelled with a capital “H”.
Context: ... Setup Steps (if needed) Location: .github/workflows/run-integration-tests.yml, i...
(GITHUB)
[uncategorized] ~188-~188: The official name of this software platform is spelled with a capital “H”.
Context: ...ication or configuration - Always check github.event_name != 'workflow_dispatch' to s...
(GITHUB)
[uncategorized] ~201-~201: The official name of this software platform is spelled with a capital “H”.
Context: ...anup step See the vLLM implementation (.github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~205-~205: The official name of this software platform is spelled with a capital “H”.
Context: ...d Integration Tests Step Location: .github/workflows/run-integration-tests.yml, a...
(GITHUB)
[grammar] ~227-~227: Use a hyphen to join words.
Context: ...d changes Note: The cleanup and log gathering steps are already configured f...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~294-~294: The official name of this software platform is spelled with a capital “H”.
Context: ...dd test section - Example: Vertex AI (.github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~302-~302: The official name of this software platform is spelled with a capital “H”.
Context: ...running as a sidecar - Example: vLLM (.github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~323-~323: The official name of this software platform is spelled with a capital “H”.
Context: ... - Remote Service):** - Authentication: .github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~324-~324: The official name of this software platform is spelled with a capital “H”.
Context: ...ent section) - Credential verification: .github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~325-~325: The official name of this software platform is spelled with a capital “H”.
Context: ...ymllines 195-203 - Integration tests:.github/workflows/run-integration-tests.yml` li...
(GITHUB)
[uncategorized] ~330-~330: The official name of this software platform is spelled with a capital “H”.
Context: ...on - Local Sidecar):** - Service setup: .github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~331-~331: The official name of this software platform is spelled with a capital “H”.
Context: ...2 (in deployment section) - Validation: .github/workflows/run-integration-tests.yml li...
(GITHUB)
[uncategorized] ~332-~332: The official name of this software platform is spelled with a capital “H”.
Context: ...tests.ymllines 155-167 - Smoke tests:.github/workflows/run-integration-tests.yml` li...
(GITHUB)
[uncategorized] ~333-~333: The official name of this software platform is spelled with a capital “H”.
Context: ...ymllines 169-174 - Integration tests:.github/workflows/run-integration-tests.yml` li...
(GITHUB)
[uncategorized] ~334-~334: The official name of this software platform is spelled with a capital “H”.
Context: ...ion-tests.ymllines 176-182 - Cleanup:.github/workflows/run-integration-tests.yml` li...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
docs/live-tests-guide.md
169-169: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
194-194: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: Summary
🔇 Additional comments (3)
.github/workflows/run-integration-tests.yml (2)
144-193: Sequential testing with conditional guards is well-structured.The workflow properly implements:
- Conditional vLLM startup (line 89) only for PR/push events
- Smoke tests with separate guards for vLLM (always) and Vertex AI (only when credentials present)
- Sequential integration tests with appropriate skip logic
- Clear separation between provider test phases
The prior critical issue about Vertex AI smoke test failures when credentials are missing has been properly resolved via conditional checks at lines 158–163.
199-244: Cleanup and logging strategy is sound.The use of
always()combined with thegithub.event_name != 'workflow_dispatch'condition correctly ensures logs are gathered and containers cleaned up even on test failure, while avoiding unnecessary steps when containers were never started..github/workflows/redhat-distro-container.yml (1)
92-104: Image tag determination logic is clear and well-scoped.The dynamic tag determination correctly differentiates between
workflow_dispatch(source format) andworkflow_run(commit-based tags with branch variants). Logic is readable and easy to maintain.
…est-for-Vertex-AI
|
|
Agreed, closing this one and splitting everything into smaller PRs. Thanks for the review. |
leseb
deleted the
RHAIENG-1793-Create-ODH-distro-image-smoke-test-for-Vertex-AI
branch
4 participants
What does this PR do?
Adds Vertex AI support to integration tests alongside vLLM. Tests run sequentially using a single Llama Stack instance configured with both providers.
Key Changes
/run/secrets/gcp-credentials(matching local Podman setup)Architecture
Test Plan
Local Testing
./tests/run_integration_tests.shwithINFERENCE_MODEL="vllm-inference/Qwen/Qwen3-0.6B"VERTEX_AI_PROJECT, rungcloud auth application-default login, then./tests/run_integration_tests.shwithINFERENCE_MODEL="vertexai/google/gemini-2.0-flash"CI Testing
Required Secrets
For Vertex AI tests to run in CI:
VERTEX_AI_PROJECT– Target GCP projectGCP_WORKLOAD_IDENTITY_PROVIDER– Used for OIDC authentication via Workload Identity FederationIf Vertex AI secrets are not configured, Vertex AI tests are skipped with a warning.
Files Changed
.github/workflows/run-integration-tests.yml- Added Vertex AI test section, sequential execution.github/actions/setup-llama-stack/action.yml- Updated to mount GCP credentials fromGOOGLE_APPLICATION_CREDENTIALS.github/actions/setup-vllm/action.yml- Made non-blocking (removed wait loop)docs/live-tests-guide.md- Updated to reflect sequential execution and adding new providersSummary by CodeRabbit
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.