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 Rails 8 Support #405

Merged
merged 9 commits into from
Dec 3, 2024
Merged

Conversation

OskarEichler
Copy link
Contributor

Copy link
Owner

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

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

Very nicely done! I think you can exclude the Ruby 1.9.3 / Rails 8 combination from the test matrix @OskarEichler, so that there are no failing test suites related to Rails 8.

Other than that, this is ready to go!

Side note: I'm not too sure about what's going on in the Ruby 2.6 / Rails 4 / Devise 3 combo, or which component is at fault, but that is unrelated to your changes so it's not a blocker.

@OskarEichler
Copy link
Contributor Author

@gonzalo-bulnes This is done - it's weird that rails_7_devise_4 failed, as it was successful in the run before

Copy link
Owner

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

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

Thank you @OskarEichler!

Indeed, the rails_7_devise_4 suite succeeded on retry, which is not great, but unrelated to your PR.

I'll approve the PR for now and merge it as soon as I've got my signing gear handy 🙂

@eclectic-coding
Copy link

I am trying to upgrade a client application to Rails 8, and I was wondering when we can expect this PR to be merged.

@gonzalo-bulnes
Copy link
Owner

gonzalo-bulnes commented Dec 3, 2024

👋 That's a question that's harder to answer than it may look like @eclectic-coding because I get to interpret what you mean without really knowing it. And I'd hate for my reply to be misinterpreted. I'll try, please bear with me knowing I don't mean this reply to sound short or disregarding of what I believe is a genuine concern of yours. (Or patronizing or anything like that! 😬)

I can't talk about yours or anyone's expectations. They are what they are. Now, that aside let's get to what I think is the core of your question, which I'll rephrase as: "I want to use this new Rails 8 support, and I need the PR to be merged for that!"

First of all, I'm glad you do! That's why the gem is there in the first place! 🙂
Now, on one hand I'm not sure you mean "PR merged" as much as "gem released" (I'll get to this in case it's not immediately clear why, and please bear with me if it is immediately clear to you!), and on the other I'm pretty confident you don't actually need the gem to be released to upgrade your application to Rails 8. Plus perks! Let me unpack.

Merge vs release

Once I merge this PR, master will contain this code... and the latest release of the simple_token_authentication gem will still be v1.18.1 with no Rails 8 support. Assuming that your application's Gemfile requires the gem by gem 'simple_token_authentication', '~> 1.0' (which I think is a fair assumption), you wouldn't get any new Rails 8 support without a new release anyway.

Is a gem release needed in order to use the Rails 8 supporting code?

Actually not. The code is written (thanks @OskarEichler!), and GitHub attests that it has been reviewed by someone (me, of all people). At this point, outside of a verifiable signature of mine (which I assume is not what you're after), from a trust perspective things are pretty much how they're going to be once the new gem release is cut. By that I mean that there isn't much more reason to trust the released gem more than @OskarEichler branch at this point.

Using the code in that branch is as simple as replacing the Gemfile line by: gem 'simple_token_authentication', :git => 'https://github.com/OskarEichler/simple_token_authentication.git', :ref => 'b5ac075'. (The reference is the last commit before my approval, visible above this comment -- relevant docs)

Ok-ish, maybe... am I settling for less?

Believe me, I hear you. Please hear me out. Certainly, this isn't a long-term solution, since it would prevent you from getting future upgrades. That being said, for the purpose of kicking off a Rails 8 upgrade, and given the overall slow pace at which new simple_token_authentication releases appear these days, I wouldn't disregard it as a reasonable interim solution. Again, that code was formally "approved" after review, and given the test suite is green and I have reasons to trust @OskarEichler thoroughness, I won't be doing anything more before merging and releasing the gem.

Now, something that could make things even better, would be if someone could indeed test the code and confirm publicly if it behaves as they expect. That Gemfile tweak is the way to do that, and I would personally appreciate such a third-party confirmation before cutting a new release. I make mistakes, and if I had overlooked anything in my review I'd largely prefer finding out before making a release that after. So if you take that step, upgrade your app, and give that feedback, that would be a significant (and appreciated!) contribution. (Not everything is code in open-source.)

To recap:

  • When will I merge the PR? Very soon (TM).
  • What about the release? Not before that, and honestly it might take a bit longer.
  • Do I/you need that to happen before being able to upgrade to Rails 8? I really don't think so.
  • Okay, but there is a bit of extra work to use unreleased code... right? Right. But that extra effort / temporary Gemfile clunky-ness can easily become an appreciated contribution (appreciated by me anyway, FWIW!) What do you think? Am I convincing you? 😉

@gonzalo-bulnes gonzalo-bulnes merged commit f1cba4e into gonzalo-bulnes:master Dec 3, 2024
22 of 24 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.

3 participants