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

Add support for Rails 7.0, 7.1, and 7.2 #6

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

mkrfowler
Copy link
Contributor

There are a few changes of interest in this sequence.

* `all_versions` undergoes a rename (to `versions`) from 7.1.
* The `schema_migration` object is accessed via `pool` from 7.2
* Multiple-database support alters the format of `db:version`
* The minimum supported Ruby version is increased in 7.0 and 7.2

As a whole, the existing integration tests remain suitable: there is only the one behavioural change to speak of. As a result, rename the integration test to be less confusing in the presence of non-Rails-6 gemfiles.

There are a few changes of interest in this sequence.

    * `all_versions` undergoes a rename (to `versions`) from 7.1.
    * The `schema_migration` object is accessed via `pool` from 7.2
    * Multiple-database support alters the format of `db:version`
    * The minimum supported Ruby version is increased in 7.0 and 7.2

As a whole, the existing integration tests remain suitable: there is only the
one behavioural change to speak of. As a result, rename the integration test to
be less confusing in the presence of non-Rails-6 gemfiles.
@rwojsznis
Copy link
Member

Hey there, thanks for this contribution! As I have been away for a while once I will catch up with my day work I will definitely take a look at this :)

Sadly it's been a while since I used this gem and I think we would need to backport more code from Gitlab for proper multi-database support - did you had a chance to run it with a real-life project after introducing suggested changes? 🤔

ref: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64884

@mkrfowler
Copy link
Contributor Author

Hey there, thanks for this contribution! As I have been away for a while once I will catch up with my day work I will definitely take a look at this :)

Thanks, I've certainly been at very low availability myself!

Sadly it's been a while since I used this gem and I think we would need to backport more code from Gitlab for proper multi-database support - did you had a chance to run it with a real-life project after introducing suggested changes? 🤔

ref: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64884

I agree that full-fledged multi-database support will want some of this work to be ported over – for instance, the implementation of #schema_directory from that changeset. My own projects don't make use of the parts of that functionality that actually exercise migrations – I suspect with little evidence that this is relatively rare. I'd probably bias to that being a separate set of changes myself.

We've been using the current version of this gem under Rails 7.0 for a while now with no issues. These changes are motivated by a real-world 7.1 update. I see reasonable behaviour both with that codebase and (more tentatively) the same application updated to Rails 7.2.

@rwojsznis
Copy link
Member

thanks for the context & feedback!

I will review and release new version of this gem with a note that 7.x support is a little bit limited (a.k.a. no multi-database support)

It would be great to port all those new changes from Gitlab eventually tho 😩

@rwojsznis rwojsznis merged commit 1b0a0af into stolen-ruby:main Nov 12, 2024
28 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.

2 participants