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

Multiple connection support #249

Merged
merged 11 commits into from
Jun 6, 2023

Conversation

chaunce
Copy link
Contributor

@chaunce chaunce commented Apr 5, 2023

This implements support for multiple connections now native to Rails, as defined in guides https://guides.rubyonrails.org/active_record_multiple_databases.html

To add support without extensive branching logic I removed all Rails 5-specific code, this appears to align with the current state of the gem as specified in the changelog https://github.com/ilyakatz/data-migrate/blob/main/Changelog.md#900

Some modifications to the existing code were made to accommodate this support, some of this more closely matches the newest Rails schema migration code structure to allow :with_data rake tasks to function properly.

Tests were updated to match code changes, but a review of these to ensure they are still testing the intended functionality would be great. This is also light on specs specific to multiple connections, I can work on additional tests later.

@ilyakatz
Copy link
Owner

Hi @chaunce thank for making this PR, this is great. However, a couple of comments

  1. looks like tests are failing for ruby 3.2
  2. looks like you updated a bunch of things related to styling and such. It makes it difficult to see the real changes that were part of this PR. Is it possible to undo those stylistic changes and only include changes relevant for this PR? you can still make the style changes, but it's should be a separate PR.

thanks gain for the PR!

@chaunce
Copy link
Contributor Author

chaunce commented Apr 21, 2023

Done, and done.

@ilyakatz
Copy link
Owner

Great, thanks for the changes. Since this is a pretty big refactor and it's a bit hard for me to validate it, I created another PR from your branch and made it so that it can push release candidates. First rc candidate is in rubygems now (see https://rubygems.org/gems/data_migrate/versions/10.0.0.rc1) so let's monitor it and see if anyone reports problems

@ilyakatz
Copy link
Owner

update, it's been running for a couple of weeks and no reported problems. i think i'll give it another week or 2 and after that we can take it out of rc

@ilyakatz ilyakatz merged commit c3624a5 into ilyakatz:main Jun 6, 2023
@ilyakatz
Copy link
Owner

ilyakatz commented Jun 6, 2023

This is now merged. Let's see if we get any issues in the coming days

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