Skip to content
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

feat: Add testing for activerecord 7.1 #77

Merged
merged 8 commits into from
Oct 29, 2023
Merged

feat: Add testing for activerecord 7.1 #77

merged 8 commits into from
Oct 29, 2023

Conversation

seuros
Copy link
Member

@seuros seuros commented Oct 14, 2023

Dropped support for unsupported rails and ruby version.

I rewrote the test using native syntax to allow easier future upgrade.

@seuros seuros changed the title chore: Add testing for activerecord 7.1 feat: Add testing for activerecord 7.1 Oct 14, 2023
@seuros seuros marked this pull request as draft October 14, 2023 19:36
Copy link
Contributor

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

I think the only failing tests are missed cleanup due to dropping MySQL < 5.7.5, everything else looks good

else
WithAdvisoryLock::MySQLNoNesting
end
WithAdvisoryLock::MySQL
Copy link
Contributor

@skipkayhil skipkayhil Oct 26, 2023

Choose a reason for hiding this comment

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

If we're dropping support for MySQL versions that don't support shared locks then there's a few more lines to change:

Remove:

raise ArgumentError, 'shared locks are not supported on MySQL' if shared

Nested is different than shared, let me check again

Edit: ah I understand now, the error gets printed to $stderr because of Thread.report_on_exception but the error itself is expected

test/nesting_test.rb Outdated Show resolved Hide resolved
test/nesting_test.rb Outdated Show resolved Hide resolved
test/shared_test.rb Outdated Show resolved Hide resolved
@skipkayhil
Copy link
Contributor

My guess for the Trilogy packet error is that the mysql docker container is configured to use caching_sha2_password, which is currently unsupported: trilogy-libraries/trilogy#26

The container may need to be configured to use mysql_native_password instead

seuros and others added 2 commits October 27, 2023 18:05
Co-authored-by: Hartley McGuire <skipkayhil@gmail.com>
@seuros seuros requested a review from skipkayhil October 27, 2023 17:18
@seuros seuros force-pushed the rails71 branch 2 times, most recently from a416592 to 9b958ec Compare October 27, 2023 18:29
@seuros seuros marked this pull request as ready for review October 27, 2023 19:09
@seuros
Copy link
Member Author

seuros commented Oct 27, 2023

Fixed the build .
@joevandyk @skipkayhil , can you try this branch .
the trilogy support works well and the legacy mysql support was removed.

@skipkayhil
Copy link
Contributor

Looks great to me, I was able to point my app at this branch and the test suite ran fully (instead of falling back to flock) 👍

Thanks for following up so quickly!

@@ -11,7 +11,7 @@ jobs:
runs-on: ubuntu-latest
services:
mysql:
image: mysql/mysql-server:8.0.30

Choose a reason for hiding this comment

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

why was this downgraded?

Copy link
Contributor

Choose a reason for hiding this comment

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

mysql 8 uses caching_sha2_password by default which is currently unsupported by Trilogy

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 still possible to make trilogy work with mysql 8 by forcing the legacy auth mode.

@seuros seuros merged commit 69c23fe into master Oct 29, 2023
63 checks passed
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.

3 participants