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

Drop support for old Rails versions < 4.2 #392

Merged
merged 6 commits into from
Nov 30, 2018
Merged

Drop support for old Rails versions < 4.2 #392

merged 6 commits into from
Nov 30, 2018

Conversation

deivid-rodriguez
Copy link
Member

Summary

What do you think about dropping support for old versions of Rails and Ruby that already reached their End of Life?

Details

Motivation and Context

  • Make maintainance easier.
  • Simplify and optimize CI.
  • Encourage easier contributions that don't need to be compatible with an excessive range of versions.

How Has This Been Tested?

Through the current CI setup.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@xtrasimplicity
Copy link
Member

Personally, I'm not too keen on removing support for rails 4-4.2, as these versions are still heavily used and I don't believe the maintenance impact is really such an issue.

The really old ruby versions, however, I can see having a slight impact on future maintenance.

Let's wait and see what the core team think. :)

@deivid-rodriguez
Copy link
Member Author

Yeah, let's wait and see :)

@luke-hill
Copy link
Contributor

In my opinion dropping the testing of old items and whatnot is fine (the rails 4.2 argument aside)

However whilst we are not using ruby 2.3 (or higher), specific code, there is no need to restrict users. Furthermore, you'll see this in other OS software that I / others maintain. Selenium for example still allowed 2.0 because they didn't use any 2.1 / 2.2 specific code. Although for selenium4 they will be, hence dropping support of older rubies.

@deivid-rodriguez
Copy link
Member Author

@luke-hill Yeah, I see what you mean, but if we accept that as a reason, it kind of becomes a vicious circle: we keep support for old rubies because we don't use newer ruby features, and we don't use newer ruby features because we still support old rubies. For example, a contributor might find interesting to use squiggly heredocs for her code, but refrain from doing it because of the current support matrix.

@deivid-rodriguez
Copy link
Member Author

In my opinion, the mere fact that a library or language version no longer gets security fixes applied is reason enough for dropping support.

@luke-hill
Copy link
Contributor

luke-hill commented Nov 26, 2018

The 2nd point you made about not using new features isn't what I was getting at.

If cucumber hypothetically needed some new feature, then the old ruby/ies would be removed. There's no question over (I can't use method X because it doesn't exist in old ruby/ies), unless you wanted dual support (Something very few gems I know about do)

To use an example. curb actively does not have a minimum ruby requirement, but they only test from 2.0 onwards I believe.

@deivid-rodriguez
Copy link
Member Author

Normally you don't need to use new features and can get away without them, it's just nice to be able to use them.

To use an example. curb actively does not have a minimum ruby requirement, but they only test from 2.0 onwards I believe.

Not sure what you mean, that's an example of what?

In any case, it makes sense to keep the ruby support matrix in sync with cucumber-core, so I'm dropping the changes about ruby versions! 👍

@deivid-rodriguez
Copy link
Member Author

I removed the changes related to ruby versions. Now the branch name is a bit misleading, I can close this PR and reopen a new one with a better branch name if needed 😃.

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Generally this seems fine. I'm not a huge rails user, but I believe 4.2 is still quite widely used. But other than that it seems good to me.

Only reason for not approving is I know others were debating about this.

@olleolleolle olleolleolle changed the title Drop old rails and rubies CI: Drop old Rails versions Nov 27, 2018
@olleolleolle olleolleolle changed the title CI: Drop old Rails versions CI: Drop old Rails versions < 5.0 from testing matrix Nov 27, 2018
@olleolleolle
Copy link
Contributor

@deivid-rodriguez I edited the title of this PR, to increase visibility. I hope my wording reflects your intent.

@deivid-rodriguez
Copy link
Member Author

Yes, although that brings another discussion to the table. I think the minimum rails version should also be limited via gemspec, but I understand from previous messages that some contributors might prefer to only stop testing but keep implicit support?

@koic
Copy link
Member

koic commented Nov 29, 2018

I also thought that it would be better to leave Rails 4.2.
Rails 4.2 is in the stage of security release (So it is not an EOL) .
https://guides.rubyonrails.org/maintenance_policy.html#severe-security-issues

Recently Rails 4.2.11 has been released.
https://weblog.rubyonrails.org/2018/11/27/Rails-4-2-5-0-5-1-5-2-have-been-released/

@olleolleolle
Copy link
Contributor

If this were changed to drop rails40, rails41 from the testing matrix, would you be in favor?

@deivid-rodriguez
Copy link
Member Author

Sounds good, I'll readd 4.2 then!

@deivid-rodriguez deivid-rodriguez changed the title CI: Drop old Rails versions < 5.0 from testing matrix CI: Drop old Rails versions < 4.2 from testing matrix Nov 29, 2018
@koic
Copy link
Member

koic commented Nov 30, 2018

If this were changed to drop rails40, rails41 from the testing matrix, would you be in favor?

IMO, If there is no strong reason for the core team, I think that it is good to drop Rails 4.0 and Rails 4.1 from the testing matrix.

@deivid-rodriguez deivid-rodriguez changed the title CI: Drop old Rails versions < 4.2 from testing matrix Drop support for old Rails versions < 4.2 Nov 30, 2018
@deivid-rodriguez
Copy link
Member Author

I ended up limiting the minimum rails version through the gemspec as well, so I edited the PR title accordingly.

@deivid-rodriguez
Copy link
Member Author

This should be ready to go now, are you ok with limiting the minimum rails via gemspec?

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Seems solid.

@@ -4,13 +4,13 @@ source "https://rubygems.org"

gem "rails", "~> 5.2"
gem "railties", "~> 5.2"
gem "capybara", "~> 3.0"
gem "capybara", "~> 3"
Copy link
Contributor

Choose a reason for hiding this comment

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

This might need a slight prod.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? Do I need to change anything?

@deivid-rodriguez
Copy link
Member Author

I'm doing some dependency rework anyways in #395, so I'm going to go ahead and merge this with @luke-hill's approval! :)

@deivid-rodriguez deivid-rodriguez merged commit a3104a0 into cucumber:master Nov 30, 2018
@deivid-rodriguez deivid-rodriguez deleted the drop_old_rails_and_rubies branch November 30, 2018 17:33
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.

5 participants