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

Rails 6.0 compatibility #225

Merged
merged 1 commit into from Dec 9, 2019
Merged

Rails 6.0 compatibility #225

merged 1 commit into from Dec 9, 2019

Conversation

zhuravel
Copy link
Contributor

@zhuravel zhuravel commented Dec 2, 2019

Tests pass with the updated phenix code from zendesk/phenix#10.

@bquorning
Copy link
Member

I have configured CircleCI to allow forked PRs to run. And I’ve released Phenix v0.6.0.

If you push this branch again, I think (hope) the build is going to run.

Copy link
Member

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Generally, good job – thank you!

A bit more context: The relevant changes in ActiveRecord are

@@ -117,15 +117,15 @@

describe "#shard_status" do
it "shows nothing if everything is ok" do
ActiveRecord::Migrator.shard_status([1]).must_equal([{}, {}])
_(ActiveRecord::Migrator.shard_status([1])).must_equal([{}, {}])
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this change in words, please?

Copy link
Contributor Author

@zhuravel zhuravel Dec 3, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI require 'maxitest/global_must' will silence this too, but converting is fine too 👍

@bquorning
Copy link
Member

@zhuravel Did you look into the new connection switching API in ActiveRecord v6.0? rails/rails#34052 and rails/rails#35073

I must admit that I haven’t yet looked at it in details, but I imagined that it might be used to simplify how this gem works. I am actually surprised to see that it apparently still works without using the new connection API.

@zhuravel
Copy link
Contributor Author

zhuravel commented Dec 9, 2019

@bquorning I did not look into how the new connection switching API can be used to simplify the gem. My goal was to upgrade our project to Rails 6 and this PR is mostly for people whose Rails 6 upgrade is also blocked by active_record_shards.

@bquorning
Copy link
Member

Could you please also lock to `"phenix", ">= 0.6.0" in the gemspec?

@bquorning
Copy link
Member

Could you please also lock to `"phenix", ">= 0.6.0" in the gemspec?

Never mind – I’ll merge this PR first, then fix up a few things before releasing the new version.

Thanks for the contribution – and your patience with me 😄

@bquorning bquorning merged commit 0c22956 into zendesk:master Dec 9, 2019
@grosser grosser mentioned this pull request Dec 9, 2019
@bquorning
Copy link
Member

@zhuravel I have just released v3.16.0 which includes these changes. Thank you again.

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.

None yet

3 participants