gonzalo-bulnes / simple_token_authentication

Simple (and safe*) token authentication for Rails apps or API with Devise.
GNU General Public License v3.0
1.51k stars 238 forks source link

Add Rails 7 Support #391

Closed OskarEichler closed 1 year ago

OskarEichler commented 2 years ago

This PR adds support for Rails 7.

Closes https://github.com/gonzalo-bulnes/simple_token_authentication/pull/393

pglombardo commented 1 year ago

From what I gather, TravisCI is no longer running the tests here and there was mention to migrate to Github actions.

Plus passing tests would be ๐Ÿ‘ for this PR.

This PR on my fork successfully runs a matrix of tests on Github actions - but the tests themselves fail unfortunately.

Screenshot 2022-11-10 at 09 22 27

Feel free to copy that .github/workflows/ruby.yml file into this PR to enable Github Action based CI testing.

oskarpearson commented 1 year ago

Iโ€™ve opened a PR to clean up some of the cruft in master. I hope that it helps move this PR forward.

https://github.com/gonzalo-bulnes/simple_token_authentication/pull/394

The intention is to get CI working so that this PR is a smaller and easier-reviewed change.

Note that on that being merged to master this will need:

Apologies for any inconvenience - and thanks for having done this!

Pipinha commented 1 year ago

Hi guys, when this could be merged?

pglombardo commented 1 year ago

Hi @gonzalo-bulnes - this PR has been open for 12 months. Do you have an ETA on this or could you provide guidance on how the community should proceed?

gonzalo-bulnes commented 1 year ago

@pglombardo If I knew you I'd probably ask you what have you done in the past 12 months. :raised_eyebrow: But I we don't know each other, so I don't. And based on that premise you seem very mistaken on the nature of our relationship.

To make it clear: you're not entitled to any of my time. In addition, I don't know what makes you think I have any interest in making any of your decisions for you. :woman_shrugging:

Please take note that that kind of posturing is not welcome, and let's move on.

OskarEichler commented 1 year ago

@gonzalo-bulnes Thanks so much! I went ahead and merged master, resolved the conflict and made the changes suggested by you - thanks for the thorough review! Please let me know if there's anything else you'd like me to change / update.

Regarding the version number I recommend potentially skipping 1.18.0 and instead using 1.18.1 or higher. Reason is, that quite a few people have been using my fork which had version 1.18.0 for the past few months, so they might not be pulling in the latest changes correct when they switch back to master as the version would remain the same 1.18.0 for them. Happy holidays ๐ŸŽ„

gonzalo-bulnes commented 1 year ago

Note that, as-is, relying on ActiveModel::Serializers requires dropping support for Rails 4. That's unfortunate, and I wonder if we could drop the need for ActiveModel::Serializers instead. I'm not super keen on cutting a v2.0.0 for a change with little significance like some obscure Rails dependency change. (Not that obscure, but not very meaningful for Simple Token Authentication either.)

(Support for Ruby 1.9.3 is potentially dropping too, but that's unrelated. It has to do with setting up a successful CI job, another not very significant reason to cut a v2.0.0.)

gonzalo-bulnes commented 1 year ago

I didn't see your comment when writing mine! :P (I didn't expect it today!)

Regarding version numbers: there isn't much to choose from. The gem adheres to semantic versioning. Breaking changes (like dropping support for anything that was supported) require a major version change.


When you've got a moment, take a look at this copy of your branch. I've been thinking about it and shaping what the different constraints lead to. The commits are not in order or grouped, that copy is in a very raw draft state, but the resulting file changes are worth a look.

OskarEichler commented 1 year ago

@gonzalo-bulnes Ohh yeah I saw the test failing for Rails 4. Had a look at your copy of the branch, and the changes look good. I guess in that case the version would probably be 2.0.0 due to the breaking changes.

pglombardo commented 1 year ago

@pglombardo If I knew you I'd probably ask you what have you done in the past 12 months. ๐Ÿคจ But I we don't know each other, so I don't. And based on that premise you seem very mistaken on the nature of our relationship.

To make it clear: you're not entitled to any of my time. In addition, I don't know what makes you think I have any interest in making any of your decisions for you. ๐Ÿคทโ€โ™€๏ธ

Please take note that that kind of posturing is not welcome, and let's move on.

This response was uncalled for and not very professional. Maybe you misjudged my intent and tone?

As the owner of a fairly popular opensource repo that others depend upon you are the gatekeeper. Properly setting expectations is common courtesy for the community so they can act appropriately. I don't think my request was out of line.

To make it clear: you're not entitled to any of my time.

I'm sorry to hear that this is your attitude but it's understood now.

gonzalo-bulnes commented 1 year ago

Note that, as-is, relying on ActiveModel::Serializers requires dropping support for Rails 4. That's unfortunate, and I wonder if we could drop the need for ActiveModel::Serializers instead.

It turns out that the active model serializer gems are not needed at all. I've used some ActiveModel::Serializers properties as a proxy for Rails version in the past (to limit the scope of use of the Mongoid adpater if you're curious) but that hasn't been needed for some time.

gonzalo-bulnes commented 1 year ago

@OskarEichler I've opened #401 as a replay of your PR. I've written down the details over there for you, but I think we should continue the conversation over here.

I'll proceed to merging it (which will close this PR) - can you please give a try to installing the gem from master once it's done, so that you can give me a thumbs up to release the gem?

gonzalo-bulnes commented 1 year ago

:shipit:'d (via #401)

gonzalo-bulnes commented 1 year ago

I've released v1.18.0, thank you for seeing this PR through @OskarEichler! :tada: (I didn't wait for your thumbs up, but if anything is wrong I'll fix it.)

(Regarding the version number, there shouldn't be any issues for people who are currently using your branch/fork, since the source of the gem will be different.)