Skip to content

Conversation

@Austio
Copy link

@Austio Austio commented Sep 18, 2025

This PR adds the ability for teams to use trilogy instead of mysql2 for their adapter. Basic logic is this

  • When we want trilogy and are rails 8.1
  • Load the trilogy adapter instead of the mysql2

Due to changes in #131 and #128 it is essentially a new adapter that does very little except pass alter statements to pt-online-schema-change and ensure that add/remove index have

There is some duplication in this that i will probably pull to a concern but opting to keep separate while in WIP

Logic for the adapter selection is documented in the readme

| Option                            | Default | What it Controls                                                                                                                                                          |
|-----------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| db_adapter_name                   | nil     | 'mysql2' or 'trilogy' - gives users an ability to provide the specific db adapter to override the db_adapter we use in departure see Db Adapter Support Below for details |

Db Adapter Support

Starting in Rails 8.1 we add support for the use of the trilogy database adapter gem. Logic for selecting an adapter follows this logic

  1. Departure.configuration.db_adapter_name is set to 'mysql2' or 'trilogy' use that value
  2. If the database configuration specifies 'mysql2 or 'trilogy' use that adapter
  3. Default to mysql2

@Austio Austio force-pushed the trilogy-adapter-support branch from 03dd91d to 0a35ff9 Compare September 18, 2025 21:50
@Austio Austio force-pushed the trilogy-adapter-support branch 3 times, most recently from 96f4d58 to 3ae2df4 Compare September 19, 2025 15:47
@Austio Austio force-pushed the trilogy-adapter-support branch from 3ae2df4 to a525937 Compare September 19, 2025 16:15
@Austio Austio force-pushed the trilogy-adapter-support branch 2 times, most recently from 828f1ab to 56cb327 Compare September 19, 2025 21:27
@Austio Austio force-pushed the trilogy-adapter-support branch 3 times, most recently from e71729b to d9131a8 Compare September 22, 2025 19:38
a host:port combo for connections to the db in tests
@Austio Austio force-pushed the trilogy-adapter-support branch from d9131a8 to 7b1112f Compare September 22, 2025 19:38

### Trilogy Adapter Support

Starting in Rails 8.1 we add support for the use of the trilogy database adapter gem. Logic for selecting an adapter follows this logic
Copy link

Choose a reason for hiding this comment

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

Would it be a problem to support trilogy since rails 7.1?

Copy link

Choose a reason for hiding this comment

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

I was also wondering if it wouldn't be possible to get that from the original connection. On my original attempt to implement trilogy support I remember trying that but ended up with the configuration like you did, but I was wondering whether that wasn't possible now.

Copy link
Author

Choose a reason for hiding this comment

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

You can derive it from the connection, i'm not 100% there yet with this pr but once i am we can answer the pre 8.1 for complexity if someone is actually needing in rails 8.0 or before.

My initial thought is to put it in 8.1 and beyond so that as people upgrade they will be able to use it.

7.1 is EOL October 1st

Copy link
Member

Choose a reason for hiding this comment

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

7.1 -> 8.0 and 8.0 -> 8.1 are easy upgrades, so I won't bother about supporting previous versions.

end
end

extend Forwardable
Copy link

Choose a reason for hiding this comment

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

Did we forget this here in previous PRs?

Copy link
Author

Choose a reason for hiding this comment

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

🙈 yes my bad

# We need the OS not to buffer the IO to see pt-osc's output while migrating
$stdout.sync = true

Departure::RailsAdapter.for_current.register_integrations
Copy link

Choose a reason for hiding this comment

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

Why is this not needed anymore? Did this get moved to the railtie?

Copy link
Author

Choose a reason for hiding this comment

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

Still tinkering but hoping to pull all the registration into the railtie, if not this will have to work and go back in

Copy link
Member

Choose a reason for hiding this comment

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

+1 on this, a railtie is the most appropriate place for registration.

…on now that we aren't automatically connecting to departure when loading the library and instead putting that logic in a railtie
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