-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Remove Rails 5.1 support #3328
Remove Rails 5.1 support #3328
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am OK with dropping support for Rails 5.1 now that it is EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we talking about Rails 5.0 or 5.1? Since 5.1 has not reached end of live according to https://guides.rubyonrails.org/maintenance_policy.html
5.1 is still receiving security patches and 5.0 even severe security patches.
Am I missing something?
@tvdeyen I think that document is outdated, according to https://rubyonrails.org/security/ |
I think this is the last one: https://github.com/rails/rails/blob/6d68bb5f695414b1801c1c3952ef955a5b0b6c6a/guides/source/maintenance_policy.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Thanks for pointing me to the correct file 👌
Still, I am hesitant with removing this now. We should at least wait another minor version. I don’t want our users to run into situations where they want to update Solidus for a bug fix or new feature and also force them to upgrade Rails.
Mayor dependency changes like that should always be done separate from bug fixes or mayor new features.
Other than that I am fine with removing it.
@tvdeyen what about printing a deprecation warning if Rails 5.1 is used and just remove it from the CI? |
This sounds like a good idea. |
7ce33e2
to
2a8cde5
Compare
I've created #3333 and updated this PR, so it's ready to merge after the next release. |
2a8cde5
to
d21f025
Compare
Description
Our code is still compatible with Rails 5.1 but that version is in its
EOL
phase, so it's not receiving security patches anymore.This PR removes support for Rails 5.1, and it's a follow-up of #3333. We should merge this PR after we release the next version, probably
2.10
.Checklist:
[ ] I have updated Guides and README accordingly to this change (if needed)[ ] I have added tests to cover this change (if needed)