Skip to content

Conversation

@radofuchs
Copy link
Contributor

@radofuchs radofuchs commented Oct 6, 2025

Description

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • New Features

    • Added a CI matrix option and a reusable environment selection flow for e2e runs.
    • Introduced a comprehensive end-to-end test configuration covering APIs, providers, models, vector stores, and tool groups.
  • Tests

    • CI now dynamically selects and validates environment-specific e2e configurations, normalizes paths, substitutes secrets securely, and shows a final preview of providers/models before running.
    • Retains service startup, health checks, connectivity, and e2e execution using the selected configuration.
  • Chores

    • Removed legacy debug/config-generation steps and streamlined the CI workflow.

@radofuchs radofuchs requested a review from tisnik October 6, 2025 10:41
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 6, 2025

Walkthrough

Replaces legacy run.yaml generation with a matrix-driven GitHub Actions workflow that scans tests/e2e/configs, selects run-<ENV>.yaml (default ci), copies and normalizes it to run.yaml, injects secrets, and then starts services and runs e2e tests. Adds tests/e2e/configs/run-ci.yaml as a CI configuration.

Changes

Cohort / File(s) Summary of Changes
CI workflow: dynamic config selection
.github/workflows/e2e_tests.yaml
Adds matrix strategy (environment: ["ci"]). Removes legacy debug/diagnostic and static run.yaml generation. Adds steps to scan tests/e2e/configs, select run-<ENV>.yaml, copy to run.yaml, convert relative paths to absolute container paths, substitute OPENAI_API_KEY, log selection, and show a configuration summary/preview. Retains service startup, health checks, connectivity tests, and e2e execution using the prepared run.yaml.
E2E configuration: CI preset
tests/e2e/configs/run-ci.yaml
Adds a comprehensive CI config defining APIs, providers (localfs, meta-reference, huggingface, openai, etc.), sqlite-backed stores/metadata, faiss vector DB and sentence-transformers embeddings, an OpenAI GPT-4 Turbo model entry, tool groups (RAG), server settings, and related persistence/kvstore entries.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant GH as GitHub Actions Runner
  participant WF as e2e_tests Workflow
  participant FS as Repo Filesystem
  participant C as Containers/Services
  participant T as E2E Tests

  GH->>WF: Start job (matrix: environment=ci)
  WF->>FS: Scan tests/e2e/configs for run-<ENV>.yaml
  alt config found
    WF->>FS: Copy run-<ENV>.yaml -> run.yaml
    WF->>WF: Normalize paths & substitute OPENAI_API_KEY
    WF->>GH: Emit config summary & preview
    WF->>C: Start containers/services per run.yaml
    C-->>WF: Health checks OK
    WF->>T: Run E2E tests
    T-->>WF: Test results
  else config missing
    WF-->>GH: Fail job (configuration not found)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • tisnik
  • matysek

Poem

In CI burrows I hop and pry, 🐇
I find the run that fits the sky—
I copy, patch, and hide the key,
Then wake the services, one, two, three.
Tests march forth and carrots gleam—green CI! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title references a typographically incorrect folder path and includes unnecessary folder details, which undermines its clarity and precision even though it attempts to summarize the main change of generic loading of run.yaml. Please correct the folder name from “teste” to “tests” or simplify the title to something like “LCORE-492: implement generic run.yaml loader for e2e configs” to improve clarity and accuracy.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • 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
Contributor

@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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d754a32 and a370fae.

📒 Files selected for processing (3)
  • .github/workflows/e2e_tests.yaml (2 hunks)
  • docker-compose.yaml (1 hunks)
  • tests/e2e/configs/run-ci.yaml (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: E2E Tests
tests/e2e/configs/run-ci.yaml

[error] 1-1: Container startup failed due to mount error: cannot mount host path '/home/runner/work/lightspeed-stack/lightspeed-stack/run-ci.yaml' to '/app-root/run.yaml' because target is not a directory (likely a file) or mismatch between file/directory types.

🪛 YAMLlint (1.37.1)
tests/e2e/configs/run-ci.yaml

[error] 97-97: duplication of key "provider_id" in mapping

(key-duplicates)


[error] 98-98: duplication of key "provider_type" in mapping

(key-duplicates)

⏰ 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

- "8321:8321" # Expose llama-stack on 8321 (adjust if needed)
volumes:
- ./run.yaml:/app-root/run.yaml:Z
- ./run-ci.yaml:/app-root/run.yaml:Z
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Restore the bind mount to a file that actually exists before docker compose up.

This change is breaking the job: the latest E2E run failed with cannot mount host path '/home/runner/.../run-ci.yaml' ... because target is not a directory, which happens when the host file is absent and Docker auto-creates a directory. We no longer ship a run-ci.yaml at the repo root, so docker compose up (locally or in CI) regresses immediately. Please keep the mount pointed at the generated run.yaml (and ensure the workflow writes that file). Suggested fix:

-      - ./run-ci.yaml:/app-root/run.yaml:Z
+      - ./run.yaml:/app-root/run.yaml:Z
🤖 Prompt for AI Agents
In docker-compose.yaml around line 10, the bind mount was changed to
./run-ci.yaml which doesn't exist and causes Docker to create a directory and
fail mounting; restore the mount to the generated file by changing the host path
back to ./run.yaml (keep the container path /app-root/run.yaml:Z) and update the
CI/workflow step that generates the file to write ./run.yaml at the repo root
before docker compose up so the file exists when mounting.

Copy link
Contributor

@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)
.github/workflows/e2e_tests.yaml (1)

121-124: Make the config summary environment-aware

The summary still hard-codes run-ci.yaml, so it will go stale as soon as we add another environment. Echo the actual matrix environment instead.

-          echo "Source config: tests/e2e/configs/run-ci.yaml"
+          echo "Source config: tests/e2e/configs/run-${{ matrix.environment }}.yaml"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a370fae and 6387ae4.

📒 Files selected for processing (1)
  • .github/workflows/e2e_tests.yaml (2 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: build-pr
  • GitHub Check: e2e_tests

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

@radofuchs radofuchs force-pushed the generic_loading_configs_e2e branch from 116d8fc to c05cb1a Compare October 6, 2025 11:12
Copy link
Contributor

@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)
.github/workflows/e2e_tests.yaml (1)

76-128: Make the summary reflect the selected config name.

Select and configure run.yaml already resolves the correct run-<env>.yaml, but the follow-up summary still hardcodes run-ci.yaml, so the log becomes misleading once we add more environments. Export the chosen filename while templating and reuse it later.

           if [ -f "$CONFIG_FILE" ]; then
             echo "Found config for environment: $ENVIRONMENT"
             cp "$CONFIG_FILE" run.yaml
+            echo "SELECTED_CONFIG=$(basename "$CONFIG_FILE")" >> "$GITHUB_ENV"
           else
             echo "Configuration file not found: $CONFIG_FILE"
             echo "Available files in $CONFIGS_DIR:"
             ls -la "$CONFIGS_DIR/"
             exit 1
           fi
@@
       - name: Show final configuration
         run: |
           echo "=== Configuration Summary ==="
-          echo "Source config: tests/e2e/configs/run-ci.yaml"
+          echo "Source config: tests/e2e/configs/${SELECTED_CONFIG:-unknown}"
           echo "Final file: run.yaml"
           echo "Container mount: /app-root/run.yaml"
           echo ""
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6387ae4 and c05cb1a.

📒 Files selected for processing (2)
  • .github/workflows/e2e_tests.yaml (2 hunks)
  • tests/e2e/configs/run-ci.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). (2)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests

@tisnik tisnik merged commit f74a8be into lightspeed-core:main Oct 6, 2025
18 of 19 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Oct 9, 2025
15 tasks
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.

2 participants