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

Allow attr_encrypted version four #237

Merged
merged 2 commits into from Apr 23, 2023

Conversation

mjankowski
Copy link

As of version 4.0.0, attr_encrypted supports Rails 7.

This change relaxes the version requirement for attr_encrypted so that there's a path to upgrade to Rails 7 while staying in the devise-two-factor 4.x series, for apps who are ready for the Rails update but not yet ready for the devise-two-factor 5.x update.

Relevant attr_encrypted version: attr-encrypted/attr_encrypted@b565876
Rails 7 changes: attr-encrypted/attr_encrypted#434

Related - #226

Opening this to start discussion and to kickoff the CI version matrix and see how it looks.

One thing I'm not sure about re: versioning here is how the internal method renaming that happened in attr_encrypted for Rails 7 support interacts with the gemspec requirements. I've started by changing < 4 to < 5 in the gemspec (presumably would bump this gem to 4.0.3?), but possibly changing the whole line to only allow ~> 4.0 is more appropriate (and maybe bump to 4.1.0?). Thoughts appreciated on that front ... happy to add doc changes or other version changes here as needed.

As of version 4.0.0, attr_encrypted supports Rails 7.

This change relaxes the version requirement for attr_encrypted so that
there's a path to upgrade to Rails 7 while staying in the
`devise-two-factor` 4.x series, for apps who are ready for the Rails
update but not yet ready for the `devise-two-factor` 5.x update.

Relevant attr_encrypted version: attr-encrypted/attr_encrypted@b565876
Rails 7 changes: attr-encrypted/attr_encrypted#434
@caiofct
Copy link

caiofct commented Apr 20, 2023

Hey there. Getting this PR merged would really benefit us since we are working in a Rails 7 upgrade but don't want to migrate to devise-two-factor 5.0.0 at the same time to reduce the chances of regressions, especially given this is such a critical piece of the app.

Any chance we can have this merged soon? Anything I could help with?

Runs spec assertions conditionally based on which version of
attr_encrypted is loaded.
@mjankowski
Copy link
Author

I just pushed another commit which changes the spec to run conditionally based on which version of attr_encrypted is loaded, which should allow for older versions to run specs correctly as well as newest.

@mjankowski
Copy link
Author

@bsedat - you have most recent commit ... are you still maintaining this gem?

Would love some guidance on what approach to take here and how to get this merged. Let me know if my previous comment about how to handle the version changes wasn't clear. I'd love guidance from a maintainer on which approach to take and then I'm happy to make the changes.

@bsedat
Copy link
Member

bsedat commented Apr 21, 2023

@bsedat - you have most recent commit ... are you still maintaining this gem?

Would love some guidance on what approach to take here and how to get this merged. Let me know if my previous comment about how to handle the version changes wasn't clear. I'd love guidance from a maintainer on which approach to take and then I'm happy to make the changes.

I won't be able to test changes in any live Rails apps but I'm fine to merge into the v4.x branch since it seems like encrypted_attributes is the only breaking change according to the attr_encrypted release.

@mjankowski Let me know if you can try it out in your own Rails app and then I can work on perhaps doing a proper gem release. In the meantime you can have bundler pull v4.x source directly from Github.

@mjankowski
Copy link
Author

I won't be able to test changes in any live Rails apps but I'm fine to merge into the v4.x branch since it seems like encrypted_attributes is the only breaking change according to the attr_encrypted release.

Sounds good -- and yes, from what I can tell it's only that internal method rename they needed to do for the attr_encrypted version bump, and that's not even directly relied on (only in specs) for devise-two-factor, so it seems safe.

@mjankowski Let me know if you can try it out in your own Rails app and then I can work on perhaps doing a proper gem release.

As of now I have this WIP PR to update Mastodon to Rails 7 - mastodon/mastodon@ace9a42 - and while there are still unrelated things to resolve there, pointing to my PR branch here was sufficient to allow the upgrades, and specs all still pass, etc.

In the meantime you can have bundler pull v4.x source directly from Github.

Thanks, that would be great, and feel less brittle than pointing at a PR branch.

@bsedat bsedat merged commit e685f91 into devise-two-factor:v4.x Apr 23, 2023
43 checks passed
@mjankowski mjankowski deleted the allow-attr-encrypted-four branch April 24, 2023 14:47
@caiofct
Copy link

caiofct commented May 2, 2023

Any chance we could get a new gem release after this one has been merged into the v4.x branch?

@bsedat
Copy link
Member

bsedat commented May 5, 2023

Any chance we could get a new gem release after this one has been merged into the v4.x branch?

Pushed v4.1.0.

@mjankowski
Copy link
Author

Thanks!

@caiofct
Copy link

caiofct commented May 5, 2023

Thank you!

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