Conversation
WalkthroughThe workflows for CI end-to-end and static checks were updated to set the Node.js version dynamically based on the branch, specifically using Node 20.x for branch "1.14" and Node 24.x otherwise. Steps involving legacy PostgreSQL setup were removed. PHPUnit test steps were expanded to run two suites, and a conditional PHPSpec step was added for branch "1.14". Additionally, the Panther JS Behat job now dynamically selects the test app version based on the branch. Changes
Suggested reviewers
Poem
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 (
|
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci_e2e-mariadb.yaml (1)
112-112: Normalize hyphen spacing
YAML lint flags excessive spaces after the hyphen (- name:). Standardize to- name:for cleaner, more consistent formatting.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 112-112: too many spaces after hyphen
(hyphens)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci_e2e-mariadb.yaml(3 hunks).github/workflows/ci_e2e-mysql.yaml(9 hunks).github/workflows/ci_e2e-pgsql.yaml(3 hunks).github/workflows/ci_e2e-unstable.yaml(7 hunks).github/workflows/ci_static-checks.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci_e2e-mariadb.yaml
[warning] 112-112: too many spaces after hyphen
(hyphens)
🪛 actionlint (1.7.7)
.github/workflows/ci_e2e-unstable.yaml
39-39: property "branch" is not defined in object type {ignore-failure: bool}
(expression)
55-55: property "branch" is not defined in object type {ignore-failure: bool}
(expression)
125-125: property "branch" is not defined in object type {ignore-failure: bool}
(expression)
217-217: property "branch" is not defined in object type {ignore-failure: bool}
(expression)
🔇 Additional comments (15)
.github/workflows/ci_static-checks.yaml (1)
124-127: Conditional PHPSpec step
The new PHPSpec step runs only for branch “1.14” and complements the existing static checks. Ensure PHPSpec is available in your dev dependencies to avoid failures..github/workflows/ci_e2e-unstable.yaml (2)
76-76: Use dynamic Node.js version
Switchingnode_versionto${{ env.NODE_VERSION }}ensures the runtime matches the branch logic.
80-83: Expand PHPUnit test suites
Running both--testsuite alland--testsuite="Sylius Test Suite"aligns this workflow with others and improves test coverage..github/workflows/ci_e2e-mariadb.yaml (4)
64-68: Conditional Node.js version setting
TheSet variablesstep correctly initializesNODE_VERSIONbased oninputs.branch.
87-87: Skip manifest prep on branch “1.14”
The conditionif: ${{ inputs.branch != '1.14' }}prevents overwriting prebuilt assets when targeting that release.
109-109: Inject dynamic Node.js version
Replacing the hardcoded value with${{ env.NODE_VERSION }}makes the build step branch-aware.
113-116: Include the Sylius Test Suite
Addingvendor/bin/phpunit --testsuite="Sylius Test Suite"enhances your end-to-end coverage..github/workflows/ci_e2e-pgsql.yaml (4)
63-68: Conditional Node.js version setting
TheSet variablesstep correctly setsNODE_VERSIONbased oninputs.branch.
80-81: Skip manifest prep on branch “1.14”
The addedifcondition prevents manifest overrides when assets are prebuilt for that branch.
102-102: Inject dynamic Node.js version
Switching to${{ env.NODE_VERSION }}aligns the build action with the branch-based logic.
106-109: Expand PHPUnit test suites
Running both--testsuite alland--testsuite="Sylius Test Suite"matches other e2e workflows and boosts coverage..github/workflows/ci_e2e-mysql.yaml (4)
63-68: Conditional Node.js version setting
TheSet variablesstep correctly mapsNODE_VERSIONfrominputs.branch.
84-84: Skip manifest prep on branch “1.14”
Theif: ${{ inputs.branch != '1.14' }}guard prevents unnecessary manifest file creation.
105-105: Inject dynamic Node.js version
Using${{ env.NODE_VERSION }}instead of a fixed value makes the build condition-aware.
117-120: Expand PHPUnit test suites
Adding the “Sylius Test Suite” run alongside theallsuite standardizes coverage across all end-to-end workflows.
118bbfd to
340b9eb
Compare
340b9eb to
5991cf2
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🛑 Comments failed to post (1)
.github/workflows/ci_e2e-mysql.yaml (1)
281-281:
⚠️ Potential issueInvalid dynamic
usesreference
GitHub Actions does not support theenvcontext inside theusesfield. Replace this step with separate conditional steps for each version:- uses: "SyliusLabs/BuildTestAppAction@${{ env.TEST_APP_VERSION }}" + # Use explicit refs since dynamic `uses` with env context isn’t supported + - name: Build application (v2.3) + if: ${{ inputs.branch == '1.14' }} + uses: SyliusLabs/BuildTestAppAction@v2.3 + with: + build_type: "sylius" + cache_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" + cache_restore_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" + e2e: "yes" + e2e_js: "yes" + database_version: ${{ matrix.mysql }} + php_version: ${{ matrix.php }} + symfony_version: ${{ matrix.symfony }} + node_version: 20.x + chrome_version: stable + - name: Build application (v2.4) + if: ${{ inputs.branch != '1.14' }} + uses: SyliusLabs/BuildTestAppAction@v2.4 + with: + build_type: "sylius" + cache_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" + cache_restore_key: "${{ github.run_id }}-${{ runner.os }}-${{ hashFiles('composer.json') }}-symfony-${{ matrix.symfony }}" + e2e: "yes" + e2e_js: "yes" + database_version: ${{ matrix.mysql }} + php_version: ${{ matrix.php }} + symfony_version: ${{ matrix.symfony }} + node_version: 24.x + chrome_version: stableCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.7)
281-281: context "env" is not allowed here. no context is available here. see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 Prompt for AI Agents
In .github/workflows/ci_e2e-mysql.yaml at line 281, the `uses` field incorrectly references an environment variable using `${{ env.TEST_APP_VERSION }}`, which is not supported by GitHub Actions. To fix this, replace the single step with multiple conditional steps, each specifying a fixed version in the `uses` field and guarded by an `if` condition that checks the value of `env.TEST_APP_VERSION`. This ensures the correct action version is used without dynamic interpolation in `uses`.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/ci_e2e-mysql.yaml (4)
160-164: Duplicate: Dynamic Node.js version selection
This block repeats lines 63–67. The same refactoring suggestion applies.
202-202: Duplicate: Dynamicnode_versioninjection
Matches line 105; ensures consistency across jobs.
247-251: Duplicate: Dynamic Node.js version selection
Repeated in the Panther job; consider consolidating the logic.
294-297: Duplicate: Non-1.14build step and dynamicnode_version
This mirrors the earlier conditional build logic for other branches.Also applies to: 305-306
🧹 Nitpick comments (2)
.github/workflows/ci_e2e-mysql.yaml (2)
63-67: Dynamic Node.js version selection
The shell logic correctly setsNODE_VERSIONbased on the1.14branch flag.Consider extracting this repeated snippet into a reusable step or composite action to DRY up the workflow.
278-292: Conditional build step for1.14using v2.3 action
Switching toSyliusLabs/BuildTestAppAction@v2.3for full1.14builds appears correct.Nit: The indentation of
if:undername:could be aligned for clarity.
Please confirm that v2.3 supports all required parameters (e.g.,node_versionandchrome_version).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci_e2e-mysql.yaml(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.2, Symfony ^6.4, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.4, Symfony ^7.2, MariaDB 11.4.7, State Machine Adapter symfony_workflow
- GitHub Check: Frontend / NodeJS 24.x
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.4, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.4, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.4, Symfony ^7.2 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: Packages / PHP 8.4, Symfony ^7.2
- GitHub Check: Packages / PHP 8.3, Symfony ^7.2, ORM ^3.3
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.4, Symfony ^7.2, PostgreSQL 17.5
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.13
🔇 Additional comments (3)
.github/workflows/ci_e2e-mysql.yaml (3)
84-84: Skip manifest generation on 1.14 branch
The addedifcondition prevents manifest.json creation on1.14, matching the intended JS build behavior.Verify downstream steps don’t assume the presence of these files in
1.14runs.
105-105: Inject dynamicnode_versioninto build action
Replacing the hardcoded value with${{ env.NODE_VERSION }}aligns with the new versioning logic.
117-120: Run both PHPUnit test suites
Expanding to run theallsuite and theSylius Test Suiteimproves coverage.
Tweaked the workflows a bit to be compatible with 2.x and 1.14 in full builds since these always use the workflows from the default branch:
The full tests on 1.14 ran from this branch are green.
Summary by CodeRabbit