Skip to content

Conversation

@rmartinoscar
Copy link
Member

We don't need to bloat github workers with full matrix tests when either of them failed.
Also used a common env and overwritting required keys instead of duplicating them.

@coderabbitai
Copy link

coderabbitai bot commented Sep 20, 2025

📝 Walkthrough

Walkthrough

Updates GitHub Actions workflows: switches main CI tests to SQLite with file creation, reconfigures MySQL/MariaDB/PG blocks, and changes multiple matrices’ fail-fast behavior to true or default. Removes explicit matrix strategy in Docker publish jobs. No application code changes or exported/public entity modifications.

Changes

Cohort / File(s) Summary
CI database reconfiguration
.github/workflows/ci.yaml
Main test matrix uses SQLite (DB_CONNECTION=sqlite, DB_DATABASE testing.sqlite). Adds step to create SQLite file. Removes embedded MySQL service from main matrix. Reorganizes MySQL/MariaDB as separate service matrices with updated env and health checks. Keeps PostgreSQL block with adjusted env and fail-fast.
Fail-fast policy updates across workflows
.github/workflows/build.yaml, .github/workflows/lint.yaml, .github/workflows/docker-publish.yml
Sets strategy.fail-fast: true in Build and Lint matrices. Removes explicit strategy (including fail-fast: false) from Docker publish jobs, reverting to default fail-fast behavior for matrices.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant GH as GitHub Actions
  participant Job as CI Job (Matrix)
  participant Env as DB Env
  participant SQL as SQLite File
  participant MySQL as MySQL Service
  participant Maria as MariaDB Service
  participant PG as PostgreSQL Service

  Dev->>GH: Push/PR triggers workflows
  GH->>Job: Start matrix jobs (fail-fast enabled)
  alt Main matrix (SQLite)
    Job->>Env: Set DB_CONNECTION=sqlite, DB_DATABASE=testing.sqlite
    Job->>SQL: Create testing.sqlite file
    Job->>Job: Run unit/integration tests against SQLite
  else MySQL matrix
    Job->>MySQL: Start service with health checks
    Job->>Env: Set mysql env (host/port/user/db)
    Job->>Job: Run tests against MySQL
  else MariaDB matrix
    Job->>Maria: Start service with health checks
    Job->>Env: Set mariadb env (host/port/user/db)
    Job->>Job: Run tests against MariaDB
  else PostgreSQL matrix
    Job->>PG: Start service with health checks
    Job->>Env: Set pgsql env (host/port/user/db)
    Job->>Job: Run tests against PostgreSQL
  end
  note over GH,Job: With fail-fast, remaining matrix jobs cancel on first failure
Loading

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title is concise and directly describes the two primary changes introduced by the PR: enabling fail-fast for tests and consolidating common environment variables, which matches the workflow edits in the changeset. It is specific enough for a reviewer scanning history to understand the main intent and contains no extraneous noise.
Description Check ✅ Passed The PR description states the intent to stop running full CI matrices when a job fails and to use a shared environment with per-job overrides, which aligns with the workflow changes in the diff. It is brief and on-topic, though it contains a minor spelling error ("overwritting") and could be slightly clearer about where the common env is defined.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

@rmartinoscar rmartinoscar removed the request for review from QuintenQVD0 September 20, 2025 14:12
Copy link

@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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.github/workflows/build.yaml (1)

21-27: Bug: matrix.php is undefined in this job.

php-version: ${{ matrix.php }} will resolve empty and break setup-php. Either pin PHP or add it to the matrix.

Apply one of the following:

Option A (pin):

-          php-version: ${{ matrix.php }}
+          php-version: '8.3'

Option B (single-axis matrix expansion):

       matrix:
-        node-version: [20, 22]
+        node-version: [20, 22]
+        php: [8.3]
.github/workflows/ci.yaml (1)

129-134: Fix actionlint error: port key must be a string.

Use string indexing when reading mapped service ports.

       - name: Integration tests
         run: vendor/bin/pest tests/Integration
         env:
-          DB_PORT: ${{ job.services.database.ports[3306] }}
+          DB_PORT: ${{ job.services.database.ports['3306'] }}
           DB_USERNAME: root
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)

81-90: Minor: service healthcheck looks good; consider explicit host port mapping for consistency.

Postgres uses 5432:5432, MySQL relies on ephemeral mapping + ${{ job.services… }}. Both are valid—pick one style repo‑wide.


135-152: Minor: mirror service/ports style across DB jobs.

Standardizing (either explicit host mapping or ephemeral + expression) simplifies maintenance.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c10280a and 741a7a2.

📒 Files selected for processing (4)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/ci.yaml (6 hunks)
  • .github/workflows/docker-publish.yml (0 hunks)
  • .github/workflows/lint.yaml (1 hunks)
💤 Files with no reviewable changes (1)
  • .github/workflows/docker-publish.yml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/ci.yaml

194-194: property access of object must be type of string but got "number"

(expression)

🔇 Additional comments (4)
.github/workflows/build.yaml (1)

14-16: Fail-fast on UI matrix is fine.

Intended trade-off acknowledged: faster feedback, fewer cross-version signals when the first combo fails.

Confirm this is acceptable for UI builds where Node 20 vs 22 differences may matter.

.github/workflows/lint.yaml (1)

36-39: Enable fail-fast for PHPStan: OK.

Quicker signal; cancels other PHP versions on first failure.

If you still want full cross-version coverage occasionally, consider a scheduled non–fail-fast workflow.

.github/workflows/ci.yaml (2)

9-21: Good: centralized test env.

Reduces duplication across jobs; defaults are sensible for CI.


153-156: Verify driver name mariadb.

Some Laravel setups still use mysql for MariaDB. Ensure config/database.php defines a mariadb connection/driver.

I can scan the repo to confirm if you want.

@rmartinoscar rmartinoscar merged commit e3b3c92 into main Sep 23, 2025
25 checks passed
@rmartinoscar rmartinoscar deleted the chore/tests branch September 23, 2025 23:22
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants