LCORE-306: Create e2e test env #289
Conversation
|
""" WalkthroughThis change introduces a new GitHub Actions workflow for end-to-end testing, adds a Docker Compose configuration for orchestrating two services, provides a containerfile for building the llama-stack service, and updates an end-to-end test to reflect changes in the readiness endpoint response schema. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Docker Compose
participant llama-stack
participant lightspeed-stack
participant E2E Tests
GitHub Actions->>Docker Compose: Start llama-stack & lightspeed-stack
Docker Compose->>llama-stack: Build & run container
Docker Compose->>lightspeed-stack: Pull & run container (after llama-stack)
GitHub Actions->>llama-stack: Wait for service to be healthy
GitHub Actions->>lightspeed-stack: Wait for service to be healthy
GitHub Actions->>E2E Tests: Run test commands (curl /v1/models)
E2E Tests->>llama-stack: Test REST API endpoints
E2E Tests->>lightspeed-stack: Test REST API endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
test.containerfile (2)
4-4: Remove unused ARG or implement its usage.The
APP_ROOTargument is defined but never used in the container. Either remove it or use it consistently throughout the Dockerfile.-ARG APP_ROOT=/app-root
8-8: Use COPY instead of ADD for local files.The
ADDinstruction should be replaced withCOPYfor local files as it's more explicit and doesn't have the additional features ofADDthat aren't needed here.-ADD run.yaml ./ +COPY run.yaml ./docker-compose.yaml (3)
32-32: Fix YAML formatting: Add newline at end of file.The file is missing a newline character at the end, which violates YAML best practices and causes linting errors.
networks: lightspeednet: - driver: bridge + driver: bridge +
17-17: Consider pinning the image version for reproducibility.Using the
latesttag can lead to non-reproducible deployments. Consider pinning to a specific version tag.- image: quay.io/lightspeed-core/lightspeed-stack:latest + image: quay.io/lightspeed-core/lightspeed-stack:<specific-version>
1-32: Consider adding health checks and restart policies for robustness.For a more robust e2e testing environment, consider adding health checks and restart policies to ensure services are properly available before tests run.
services: llama-stack: build: context: . dockerfile: test.containerfile container_name: llama-stack ports: - "8321:8321" volumes: - ./run.yaml:/app-root/run.yaml:Z environment: - OPENAI_API_KEY=${OPENAI_API_KEY} networks: - lightspeednet + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8321/readiness"] + interval: 30s + timeout: 10s + retries: 3 + restart: unless-stopped lightspeed-stack: image: quay.io/lightspeed-core/lightspeed-stack:latest container_name: lightspeed-stack ports: - "8080:8080" volumes: - ./lightspeed-stack.yaml:/app-root/lightspeed-stack.yaml:Z environment: - OPENAI_API_KEY=${OPENAI_API_KEY} depends_on: - llama-stack networks: - lightspeednet + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8080/readiness"] + interval: 30s + timeout: 10s + retries: 3 + restart: unless-stopped.github/workflows/e2e_tests.yaml (1)
53-53: Fix YAML formatting: Remove trailing spaces.There are trailing spaces at the end of line 53 that violate YAML formatting standards.
authentication: - module: "noop" - + module: "noop"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e_tests.yaml(1 hunks)docker-compose.yaml(1 hunks)test.containerfile(1 hunks)tests/e2e/features/rest_api.feature(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
docker-compose.yaml
[error] 32-32: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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: e2e_tests
🔇 Additional comments (2)
tests/e2e/features/rest_api.feature (2)
19-21: LGTM: Schema correctly updated for new API response.The addition of the "providers" field to the readiness endpoint schema correctly reflects the API evolution to support multiple providers.
25-25: LGTM: Response expectation matches updated schema.The expected response body correctly includes the new "providers" field with an empty list and updated reason text that better reflects the multi-provider nature of the system.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
Nice work!
Have just one nit about Python version.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test.containerfile (3)
10-11: Python 3.12 upgrade implemented but security concerns remain.Good job implementing the Python 3.12 upgrade as suggested in previous reviews. However, the security recommendations from past reviews haven't been addressed - the container still runs as root without implementing security best practices.
13-13: Security vulnerability remains unaddressed.The security concern raised in previous reviews about downloading and executing the uv installation script without integrity verification has not been addressed. This poses a supply chain security risk.
17-35: Package version pinning still not implemented.The recommendation from previous reviews to pin package versions for reproducible builds has not been addressed. This is particularly important for packages like
torch,llama-stack, and other ML dependencies that frequently release breaking changes.
🧹 Nitpick comments (1)
test.containerfile (1)
4-4: Remove unused ARG or implement its usage.The
APP_ROOTargument is defined but never used in the Dockerfile. Either remove it or implement it consistently throughout the file.-ARG APP_ROOT=/app-root
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test.containerfile(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-pr
🔇 Additional comments (1)
test.containerfile (1)
37-37: Command structure is correct.The final command properly uses uv to run the llama stack with the configuration file.
|
LGTM! Good job @radofuchs ! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/e2e_tests.yaml (3)
194-197: Docker-Compose download still lacks integrity verificationPrevious review already covered this: the workflow downloads the binary every run without checksum/signature verification or using the runner-provided compose. The security concern remains.
200-202: Fixed sleep persists – prefer readiness pollingThe hard-coded
sleep 20is still present; earlier comment proposed an active health-check loop with timeout.
204-207:uvinstall script executed without verificationSame issue noted before: piping an unverified script to
shis risky; please add checksum validation.
🧹 Nitpick comments (1)
.github/workflows/e2e_tests.yaml (1)
52-53: Trim trailing whitespace to satisfy YAMLlintThe YAML-lint warning points at line 53 for trailing spaces. Remove the stray spaces or the empty line to keep the workflow lint-clean.
- module: "noop" - + module: "noop"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/e2e_tests.yaml (1)
200-203: Swap the fixed 20-second sleep for an active readiness loopA hard-coded sleep is brittle – slow CI runners or image pulls will cause flaky failures, while fast runners waste time. Poll the
/readinessendpoints with a timeout instead:- echo "Waiting for services to be healthy..." - sleep 20 # adjust depending on boot time + echo "Waiting for services to be healthy..." + timeout=300 # 5 min hard limit + until curl -sf http://localhost:8080/readiness \ + && curl -sf http://localhost:8321/readiness; do + ((timeout-=5)) || { echo "Timed out"; docker compose logs; exit 1; } + sleep 5 + doneThis removes flakiness and gives deterministic failure information.
🧹 Nitpick comments (1)
.github/workflows/e2e_tests.yaml (1)
51-53: Remove trailing whitespace to satisfy YAML linters
yamllintflags a trailing space at line 53. While harmless, it will keep the workflow non-green if linting is enforced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
121-126: Verify${env.OPENAI_API_KEY}is interpolated as intended insiderun.yamlInside the generated YAML you reference:
api_key: ${env.OPENAI_API_KEY}Unless the downstream application performs
${env.*}expansion itself, the literal string will be written to file (GitHub Actions does not substitute here). Confirm thatllama-stacksupports this syntax, otherwise use plain$OPENAI_API_KEYor inject the value before launching the container.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/e2e_tests.yaml (1)
197-201: Fixed sleep still present – switch to readiness polling
This 20-secondsleepwas flagged earlier and remains unchanged. A hard-coded delay is brittle and slows the workflow unnecessarily. Poll the/readinessendpoints with a timeout instead.
🧹 Nitpick comments (1)
.github/workflows/e2e_tests.yaml (1)
53-53: Trailing whitespace
Line 53 contains superfluous spaces, tripping YAML linters. Remove them to keep the workflow tidy.- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
125-126: Verify env-var interpolation insiderun.yaml
api_key: ${env.OPENAI_API_KEY}depends on the loader supporting${env.*}syntax. If the llama-stack resolver doesn’t substitute this, the key will literally be"${env.OPENAI_API_KEY}", causing auth failures. Confirm the expansion mechanism or inject the secret explicitly at container runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/e2e_tests.yaml (1)
52-54: Strip trailing whitespace to appease YAML lintersYAMLlint flags line 53 for trailing spaces. A tiny clean-up keeps the workflow green.
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
123-126: Confirm that${env.OPENAI_API_KEY}expands insiderun.yaml
create-a-file-actionwrites the literal string; no interpolation happens at file-creation time.
Unless llama-stack’s config loader performs its own${env.*}expansion, the key will remain unresolved at runtime.Please verify and, if needed, switch to a plain
${OPENAI_API_KEY}placeholder (or read from process env in code) to ensure the secret is actually picked up.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/e2e_tests.yaml (1)
200-206:curlnever fails on 4xx / 5xx – health-check may produce false positives
curlreturns exit-code 0 for HTTP errors unless-f/--failis used, so the loop can stop even when the service responds with 503/404 and the stack is still booting.
Add-f(and optionally-sSLfor quiet output) to make the condition reliable.- if curl -X 'GET' 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then + if curl -fsSL -X GET 'http://localhost:8080/v1/readiness' \ + -H 'accept: application/json' ; then
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
217-224: Optional: speed up repeated runs with a pip cache
pip install uv&uv syncdownload dependencies every workflow run, adding latency (~30-60 s).
Using GitHub’s cache action for the pip directory (or~/.cache/pypoetry/uv) can cut cold-start time significantly while still installing the latest version when the lock file changes.Not critical, but worth considering for CI efficiency.
51-53: Trailing whitespace violates YAML-lintLine 53 is blank but contains spaces, triggering the reported
trailing-spaceslint error.
Remove the stray spaces to keep the workflow YAML-lint-clean.- module: "noop" - + module: "noop" +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
121-126: EnsureOPENAI_API_KEYreaches the container
run.yamlexpects the placeholder${env.OPENAI_API_KEY}to be resolved inside the container.
Unless the compose file passes this variable through (environment:orenv_file:), the value will be empty and remote-OpenAI calls will fail.Please verify that:
docker-compose.yamldeclaresenvironment: OPENAI_API_KEY=${{ secrets.OPENAI_API_KEY }}for thellama-stackservice, or- The application resolves
${env.OPENAI_API_KEY}via a different mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/e2e_tests.yaml (4)
1-5: Header comment references wrong filenameThe header says
# .github/workflows/e2e_test.yml, but the actual file ise2e_tests.yaml.
This can confuse readers and tooling that greps for workflow names.-# .github/workflows/e2e_test.yml +# .github/workflows/e2e_tests.yaml
51-53: Trailing whitespace breaks YAML-lintLine 53 violates the repo’s YAML-lint config (
trailing-spaces).
Clean it up to keep the workflow green.- module: "noop" - + module: "noop"
197-210: Health-check only lightspeed-stack – llama-stack is uncheckedThe loop exits as soon as the lightspeed endpoint is up, but the tests also rely
onllama-stack(port 8321). If llama-stack is still booting, the subsequent
e2e run flaps.- if curl -f -X 'GET' 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then + if curl -fsSL http://localhost:8080/v1/readiness \ + && curl -fsSL http://localhost:8321/v1/readiness ; then
217-224: Consider cachinguvinstall
pip install uvexecutes on every run and costs ≈15 s.
Adding the GitHub Actions cache keyed onpipwheels or using
actions/setup-python’s built-in cache saves CI minutes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
.github/workflows/e2e_tests.yaml
Outdated
| - provider_id: openai | ||
| provider_type: remote::openai | ||
| config: | ||
| api_key: ${env.OPENAI_API_KEY} |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
53-53: Strip trailing whitespace to satisfy linters
yamllintis already flagging these three lines.
While harmless at runtime, it keeps the diff-noise low and prevents the CI from failing if stricter lint rules are enabled later.-<blank line that still contains spaces>␠␠ +<truly blank line>Also applies to: 198-198, 207-207
192-201: Consider tearing down the stack after the tests finish
docker compose up -dstarts two long-running containers, but nothing calls
docker compose down.
Cleaning up at the end of the job:
- avoids noise in the job logs (
Container XXX exited with code 137),- frees disk space on the runner, and
- guarantees that volumes/networks are removed even if more jobs are added later.
docker compose up -d + trap 'docker compose down --volumes' EXIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 198-198: trailing spaces
(trailing-spaces)
[error] 207-207: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
- GitHub Check: build-pr
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
200-212: Health-check both services, not just lightspeed-stackThe readiness loop only probes port 8080. If
llama-stack(port 8321) fails to start, the job will still proceed and tests will later error out. Add a second probe inside the loop.- if curl -f -X 'GET' 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then + if curl -sf 'http://localhost:8080/v1/readiness' \ + && curl -sf 'http://localhost:8321/v1/readiness'; then
52-54: Trim trailing whitespaceYAML-lint flags line 53 for trailing spaces. Remove the extra blanks to keep the workflow YAML clean.
- module: "noop" - + module: "noop"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
| - provider_id: openai | ||
| provider_type: remote::openai | ||
| config: | ||
| api_key: ${{ env.OPENAI_API_KEY }} | ||
| post_training: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t embed the OpenAI key into the generated file
Evaluating the expression ${{ env.OPENAI_API_KEY }} at workflow runtime writes the secret into run.yaml, which is then baked into the container image and stored on disk.
Prefer passing the key as an environment variable to the container instead of persisting it in a file.
- api_key: ${{ env.OPENAI_API_KEY }}
+ # Will be supplied at container startup
+ api_key: ${OPENAI_API_KEY}Then ensure docker compose forwards OPENAI_API_KEY (already present in the compose file).
This eliminates the on-disk copy and keeps the secret ephemeral.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - provider_id: openai | |
| provider_type: remote::openai | |
| config: | |
| api_key: ${{ env.OPENAI_API_KEY }} | |
| post_training: | |
| - provider_id: openai | |
| provider_type: remote::openai | |
| config: | |
| # Will be supplied at container startup | |
| api_key: ${OPENAI_API_KEY} | |
| post_training: |
🤖 Prompt for AI Agents
In .github/workflows/e2e_tests.yaml around lines 122 to 126, the OpenAI API key
is embedded directly into the generated file by evaluating the expression `${{
env.OPENAI_API_KEY }}`, which causes the secret to be stored on disk and baked
into the container image. To fix this, remove the direct embedding of the API
key in the config section and instead configure the container to receive the
OPENAI_API_KEY as an environment variable at runtime. Also, verify that the
docker compose setup forwards the OPENAI_API_KEY environment variable to the
container to keep the secret ephemeral and avoid persisting it in files.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/e2e_tests.yaml (2)
122-126: Avoid persisting secrets inrun.yamlThe expression
${{ env.OPENAI_API_KEY }}is evaluated at workflow runtime, writing the raw secret intorun.yaml. That file is then baked into the container image and later printed to the logs, so the secret ends up on disk and (masked-but-present) in logs/artifacts.- api_key: ${{ env.OPENAI_API_KEY }} + # Injected at container start-up – keeps the secret off disk + api_key: ${OPENAI_API_KEY}With this change the compose file already propagates
OPENAI_API_KEY, so the provider still receives the key without persisting it.
186-190: Removecat run.yaml– it leaks the secret to logs
run.yamlcontains the resolved API key, so printing it exposes the key (GitHub masks, but the file is still stored in plain text). Drop the debug print or redact the secret before logging.- cat run.yaml
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
53-53: Trim trailing whitespaceYAML-lint flags trailing spaces on this line. Harmless but noisy in linters.
- +
198-198: Remove trailing whitespaceAnother minor YAML-lint warning – tidy up to keep the workflow linter-clean.
- echo "OPENAI_API_KEY is set: $([ -n "$OPENAI_API_KEY" ] && echo 'YES' || echo 'NO')" + echo "OPENAI_API_KEY is set: $([ -n "$OPENAI_API_KEY" ] && echo 'YES' || echo 'NO')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 198-198: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
- GitHub Check: build-pr
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/e2e_tests.yaml (2)
188-190:cat run.yamlleaks the secret – remove the commandDumping
run.yamlprints the resolved API key to the log. GitHub masks the value, but the file with the secret remains in the workspace and any uploaded artefacts. This exact issue was flagged earlier and is still present.
124-126: Avoid persisting the OpenAI secret inrun.yaml
${{ env.OPENAI_API_KEY }}is rendered at workflow runtime, writing the raw key torun.yamland baking it into the container image. Keep the secret ephemeral by passing it only via environment variables.- api_key: ${{ env.OPENAI_API_KEY }} + # Supplied at container start-up + api_key: ${OPENAI_API_KEY}Then verify
docker composeforwardsOPENAI_API_KEYinto thellama-stackcontainer (the compose file already exposes it for both services).
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
200-213: Health-check probes only one serviceBoth
lightspeed-stack(8080) andllama-stack(8321) are started, yet the readiness loop checks only 8080. Ifllama-stackfails to start, the job proceeds and tests will explode later.- if curl -f -X 'GET' 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then + if curl -f http://localhost:8080/v1/readiness \ + && curl -f http://localhost:8321/v1/readiness; thenOptionally factor the check into a small function to keep the loop tidy.
53-53: Remove trailing whitespace
yamllintreports trailing spaces on lines 53 and 196. Cleaning them keeps the workflow lint-clean and avoids noisy CI warnings.Also applies to: 196-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 196-196: trailing spaces
(trailing-spaces)
⏰ 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: e2e_tests
- GitHub Check: build-pr
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/e2e_tests.yaml (1)
186-190: Still printing the generated file leaks configuration details – please drop this debug step
This exact issue was raised earlier; thecat run.yamlline remains.- cat run.yamlEven if the secret value is not expanded, keeping sensitive config out of logs is a good habit.
🧹 Nitpick comments (2)
.github/workflows/e2e_tests.yaml (2)
51-53: Remove trailing whitespace to keep YAMLlint green
yaml-lintis already reporting trailing-space errors (see static-analysis output). Clean these up to avoid needless CI noise.- authentication: - module: "noop" + authentication: + module: "noop"
204-214: 30-second readiness window is tight – consider a more flexible wait loopCold starts in CI often exceed 30 s. A longer timeout plus exponential back-off keeps the workflow stable without sacrificing speed.
- timeout=30 + timeout=120 # extend to 2 min while [ $timeout -gt 0 ]; do - if curl -f -X 'GET' 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then + if curl -fsS -X GET 'http://localhost:8080/v1/readiness' -H 'accept: application/json'; then echo "Services are ready!" break fi echo "Waiting for services... ($timeout seconds remaining)" - sleep 5 - timeout=$((timeout-5)) + sleep 5 + timeout=$((timeout-5)) done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 196-196: trailing spaces
(trailing-spaces)
⏰ 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: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
.github/workflows/e2e_tests.yaml (1)
146-148: Confirm that the placeholder"******"is acceptable at runtime
braintrust’sopenai_api_keyis hard-coded to six asterisks, unlike the${OPENAI_API_KEY}used for the primary provider.
If the provider genuinely needs a real key during e2e runs, the tests will fail. If the field is ignored in your scenario, please just confirm.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/e2e_tests.yaml (2)
189-191:cat run.yamlstill dumps secrets – please drop the debug print
This was already flagged earlier; it re-exposes the resolvedOPENAI_API_KEYin job logs and artifacts.
Delete the line or redact the sensitive section.
204-207: Replace fixed 20 s sleep with readiness polling
A hard-coded delay is brittle and slows the fast path. Use a loop that polls the/readinessendpoints with a timeout, as suggested previously.
🧹 Nitpick comments (1)
.github/workflows/e2e_tests.yaml (1)
197-199: Leaking secret metadata – avoid printing key length
Revealing the exact length of a secret adds unnecessary side-channel information. Remove theseecholines or replace with a generic “variable present” check.-echo "OPENAI_API_KEY is set: $([ -n "$OPENAI_API_KEY" ] && echo 'YES' || echo 'NO')" -echo "OPENAI_API_KEY length: ${#OPENAI_API_KEY}" +if [ -n "$OPENAI_API_KEY" ]; then + echo "OPENAI_API_KEY detected" +else + echo "OPENAI_API_KEY missing" + exit 1 +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e_tests.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/e2e_tests.yaml
[error] 14-14: trailing spaces
(trailing-spaces)
[warning] 15-15: wrong indentation: expected 10 but found 12
(indentation)
[error] 55-55: trailing spaces
(trailing-spaces)
[warning] 56-56: wrong indentation: expected 10 but found 12
(indentation)
[error] 194-194: trailing spaces
(trailing-spaces)
[warning] 195-195: wrong indentation: expected 10 but found 12
(indentation)
[error] 200-200: trailing spaces
(trailing-spaces)
[error] 212-212: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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-pr
| inference: | ||
| - provider_id: openai | ||
| provider_type: remote::openai | ||
| config: | ||
| api_key: ${{ secrets.OPENAI_API_KEY }} |
There was a problem hiding this comment.
YAML structure broken under providers.inference – indentation & secret-handling need fixing
The config key is not part of the list item because it’s out-dented relative to the preceding - provider_id … entry.
YAML will parse inference as a sequence and a mapping simultaneously, which is invalid in most parsers and will break service start-up.
While you’re there, avoid embedding the secret directly in the file—pass it via env-var instead to keep it ephemeral.
- inference:
- - provider_id: openai
- provider_type: remote::openai
- config:
- api_key: ${{ secrets.OPENAI_API_KEY }}
+ inference:
+ - provider_id: openai
+ provider_type: remote::openai
+ config:
+ api_key: ${OPENAI_API_KEY} # injected at container runtime🤖 Prompt for AI Agents
In .github/workflows/e2e_tests.yaml around lines 122 to 126, the YAML under
providers.inference is invalid because the config key is not properly indented
as part of the list item, causing a mix of sequence and mapping. Fix this by
indenting config and its api_key under the provider_id entry to keep them within
the same list item. Additionally, remove the direct use of the secret in the
YAML and instead pass the OPENAI_API_KEY via an environment variable to avoid
embedding secrets directly in the file.
3 participants
Description
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Tests