Skip to content

[1.x] Add mariadb driver support plus Laravel 11 CI tests #330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Mar 13, 2024

GitHub actions had started failing, which appears to be because we were relying on default DB_ env vars and/or default config file vars which have changed.

This PR:

  • Makes the DB auth settings explicit in CI rather than relying on defaults.
  • Adds Laravel 11 to the matrix
  • Adds support for the mariadb database driver, which, in addition to being nice to have, also solved an issue with different default collations.

@jessarcher jessarcher changed the title [1.x] Add Laravel 11 CI tests [1.x] Add mariadb driver support plus Laravel 11 CI tests Mar 13, 2024
@jessarcher jessarcher marked this pull request as ready for review March 13, 2024 02:35
Comment on lines +66 to +69
DB_DATABASE: pulse
DB_USERNAME: pulse
DB_PASSWORD: password
DB_COLLATION: utf8mb4_unicode_ci
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting the credentials explicitly instead of relying on the defaults which recently changed.

The collation needs to be explicitly included here because the default collation is not supported on MySQL 5.7 where these tests are running.

Comment on lines -71 to +76
image: mariadb:10.5
image: mariadb:10
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems MariaDB 10.5 doesn't support the new default collation. Rather than customize the collation here, I've opted to match the framework's CI tests and specify 10, which would give us MariaDB 10.6.

Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Good stuff 👍

@taylorotwell taylorotwell merged commit dfd1413 into 1.x Mar 13, 2024
@taylorotwell taylorotwell deleted the laravel-11-ci branch March 13, 2024 16:04
@francoism90
Copy link

Pulse's first-party storage implementation currently requires a MySQL or PostgreSQL database.

Is this still the case after this PR?

@driesvints
Copy link
Member

@francoism90 no MariaDB was added in here. I'll PR that to the docs.

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.

5 participants