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

Don't add default-mysql-client when running db:system:change to Trilogy #50170

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

y-yagi
Copy link
Member

@y-yagi y-yagi commented Nov 26, 2023

Motivation / Background

This Pull Request has been created because db:system:change --to trilogy adds an unneeded package.

Detail

This PR removed adding default-mysql-client to Dockerfile when running db:system:change --to trilogy because Trilogy doesn't depend on the libmariadb / libmysqlclient library.
Ref: https://github.blog/2022-08-25-introducing-trilogy-a-new-database-adapter-for-ruby-on-rails/#should-you-use-trilogy

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Nov 26, 2023

assert_file("Dockerfile") do |content|
assert_match "build-essential git", content
assert_match "curl libvips", content
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add assert_no_match "default-libmysql-client", etc? Otherwise if someone accidentally adds back the dependency this test will still pass, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense to me. I added checking default-libmysqlclient-dev. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@y-yagi Why not add one for default-mysql-client too?

Copy link
Member Author

Choose a reason for hiding this comment

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

@zzak
This is a test to prevent to addition of MySQL client packages when using Trilogy adapter. So I thought one package is enough.

@y-yagi y-yagi force-pushed the fix-db_system_change-to-trilogy branch from 5d7ee69 to 4f966a5 Compare November 26, 2023 04:33
@y-yagi y-yagi force-pushed the fix-db_system_change-to-trilogy branch from 4f966a5 to 1e3e013 Compare November 26, 2023 04:34
@eileencodes eileencodes merged commit 1b67822 into rails:main Nov 27, 2023
4 checks passed
@y-yagi y-yagi deleted the fix-db_system_change-to-trilogy branch November 27, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants