fixes #19050 - add Ruby on Rails 5.0 support #4422

domcleal · 2017-03-30T07:59:23Z

Adds configurable support for running on Ruby on Rails 5.0 or 4.2,
defaulting now to 5.0 on Ruby 2.3 or higher. Ruby versions with partial
or no support remain using 4.2 by default. Other points:

SETTINGS[:rails] is loaded pre-Rails boot, then post-boot the
majority of the SETTINGS are loaded depending on the environment.
settings.yaml.dist will allow packagers to override the default
version of Rails loaded to match their distribution.
rails-controller-testing, record_tag_helper replace extracted Rails
4.x features.
ParamsParser middleware is no longer loaded by default and is
deprecated, CatchJsonParseErrors is no longer order dependent.
quiet_assets is not supported under Rails 5, will be replaced under
ticket #18500, only affects development logging.

mention-bot · 2017-03-30T07:59:24Z

@domcleal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lzap, @ehelms and @ohadlevy to be potential reviewers.

ohadlevy · 2017-03-30T08:32:44Z

@domcleal thanks for doing this, it seems like a major effort... a few questions:

how sustainable is it to keep compatibility with both versions? I would assume we would need to double our CI matrix to find issues?
Is there any guidance to plugins?
which platforms are requiring rails 4.x at the moment ? or in other words, when would be able to upgrade fully?
can we use some of the rails 5 features (action cable etc) or rather this force us to be 4.2 compatible until we upgrade fully?

are there any other highlight to the release (performance etc) that you've noticed?

thanks!

domcleal · 2017-03-30T08:53:18Z

how sustainable is it to keep compatibility with both versions? I would assume we would need to double our CI matrix to find issues?

We can do, though I don't see much point in that. Testing the default version ought to be sufficient.

Is there any guidance to plugins?

I don't think any is necessary. Rails 4 provides deprecation warnings where required.

which platforms are requiring rails 4.x at the moment ? or in other words, when would be able to upgrade fully?

All Ruby 2.0, 2.1 and early 2.2 installations, as Rails 5.0 requires 2.2.2 or higher. For OS versions, that would be Ubuntu 14.04, Debian 8, current EL7 packages, and Fedora 24.

can we use some of the rails 5 features (action cable etc) or rather this force us to be 4.2 compatible until we upgrade fully?

I don't see why not.

are there any other highlight to the release (performance etc) that you've noticed?

No.

domcleal · 2017-03-30T08:53:39Z

domcleal · 2017-04-03T14:21:38Z

PR rebased for review. Fixes for MySQL test failures etc. merged via other PRs.

ohadlevy · 2017-04-04T13:45:10Z

@domcleal is there an easy way to run this patch across many plugins? I am assuming some of them will break and hope to allow them to prepare in advance?

domcleal · 2017-04-04T13:54:44Z

Add the plugins to your checkout, run their rake test tasks?

ohadlevy · 2017-04-04T13:55:33Z

I know that :-) , I mean using jenkins matrix.

domcleal · 2017-04-04T14:05:39Z

Sure, you can create a matrix job using the test plugin job or its scripts then.

ohadlevy · 2017-05-26T18:41:51Z

[test katello]

ohadlevy · 2017-05-26T19:00:22Z

@domcleal I often get

21:50:58 rails.1   | 2017-05-26T21:50:58 46ad1e75 [app] [F] ActiveRecord::ConnectionTimeoutError (could not obtain a connection from the pool within 5.000 seconds (waited 5.000 seconds); all pooled connections were in use):

bumping the pool size value helped to avoid it (from 5 to 10) but I wonder if you know more about the root cause or if we should update our installer too?

ohadlevy · 2017-05-26T19:01:01Z

foreman and katello test failures seems unrelated.

ohadlevy · 2017-05-26T19:32:52Z

@lzap heads up, this breaks discovery a bit
@ares, @dLobatog also breaks templates rex ansible plugins with a similar error

rake aborted!
NameError: uninitialized constant Rake::TestTask
/home/foreman/gems/bundler/gems/foreman_discovery-ce2b2f7a023b/lib/discovery.rake:4:in `block in <top (required)>'
/home/foreman/gems/bundler/gems/foreman_discovery-ce2b2f7a023b/lib/discovery.rake:2:in `<top (required)>'
/home/foreman/gems/bundler/gems/foreman_discovery-ce2b2f7a023b/lib/foreman_discovery/engine.rb:211:in `block in <class:Engine>'
/home/foreman/gems/gems/railties-5.0.3/lib/rails/railtie.rb:239:in `instance_exec'

ohadlevy · 2017-05-28T12:04:20Z

also,when running tests locally i see failures on all facts related tables, any idea why it fails on my env?
I can't seems to find any reason why the data is empty, adding a logger before yields the correct values.

ActiveRecord::StatementInvalid: SQLite3::ConstraintException: NOT NULL constraint failed: fact_values.id: INSERT INTO "fact_values" ("value", "fact_name_id", "host_id", "updated_at", "created_at") VALUES (?, ?, ?, ?, ?)

ohadlevy · 2017-05-28T12:04:30Z

[test foreman]

domcleal · 2017-05-30T07:33:19Z

@domcleal I often get

21:50:58 rails.1 | 2017-05-26T21:50:58 46ad1e75 [app] [F] ActiveRecord::ConnectionTimeoutError (could not obtain a connection from the pool within 5.000 seconds (waited 5.000 seconds); all pooled connections were in use):

bumping the pool size value helped to avoid it (from 5 to 10) but I wonder if you know more about the root cause or if we should update our installer too?

I don't know the root cause without more information on how to reproduce it. I don't see why the pool would need to be higher unless something (internally or your web server) is multi-threaded.

also,when running tests locally i see failures on all facts related tables, any idea why it fails on my env?

It suggests that the auto-increment on the id column is missing, it wouldn't be missing data in the query placeholders, as fact_values.id isn't passed in.

ohadlevy · 2017-06-01T08:31:59Z

thanks @domcleal i had a local user db that probably didnt have the auto increment.

@domcleal could you provide a way to preserve the existing rake task interface behavior
for exampel at https://github.com/theforeman/foreman_discovery/blob/develop/lib/foreman_discovery/engine.rb#L210

this breaks every plugin that adds rake tasks by needing to require 'rake/testtask'

ohadlevy · 2017-06-01T11:00:33Z

config/boot_settings.rb

+SETTINGS = File.exist?(dist_settings_file) ? YAML.load(ERB.new(File.read(dist_settings_file)).result) || {} : {}
+
+settings_file = File.expand_path('../settings.yaml', __FILE__)
+SETTINGS.merge! YAML.load(ERB.new(File.read(settings_file)).result).select { |k,v| k == :rails }


nitpick: can use something like

SETTINGS[:rails] = YAML.load(...)[:rails]

dLobatog

@domcleal Left a comment inline about the packaging work that could be needed, let me know what you had in mind.

Still need to check breakages on plugins or tasks like @ohadlevy said before.

Thanks, this looks like a neat solution to the problem (especially since it looks like for some time we'd have 4.2 on some OSs).

dLobatog · 2017-06-02T15:59:03Z

Gemfile

+when '4.2'
+  gem 'rails', '4.2.8'
+when '5.0'
+  gem 'rails', '5.0.3'


There is no RoR 5.0 nor Ruby 2.3+ SCL that I'm aware of, I asked the CentOS SCL SIG on #centos-devel and @pvalena replied it's on its way, but not yet there.

How would this be handled package-wise? Would we need 2 different packages foreman-1.16-rails-50 foreman-1.16-rails-42 or is there some other solution I'm not yet aware of? Fedora/Ubuntu/etc... using 4.2 until it's possible to switch to 5.0 and EL7 relying on the SCL?

I'm not sure I understand your question. Supporting both is unlikely to be useful, but they can use the default Rails version or override it via config/settings.yaml.dist if desired.

I don't advocate for supporting both on every platform either, my question is more about what to do once this is merged:

CentOS/RHEL for 1.16 will probably have the required SCLs to run this without a problem (so this is fine)

Fedora 25+ doesn't need anything AFAIK (https://fedoraproject.org/wiki/Changes/Ruby_on_Rails_5.0). Older fedoras cannot be supported

Debian only ships Ruby 2.3+ on stretch and sid (jessie is 2.1.5). So it should ship with the 4.2 version.

Ubuntu xenial, yakkety and zesty all have 2.3+. Trusty, however, is still on 1.9.3

Therefore, Debian Jessie and Ubuntu Trusty if we want to keep maintaining them will have to be 4.2, while the rest would be 5.0. Is that your original plan?

Trusty can upgrade Ruby version using PPA, so only Jessie blocks from upgrading all to 2.3.

@dLobatog I assume we could use a newer SCL once its avail on CentOS/RHEL.
@domcleal are you planning to maintain a rails 5 scl? afaiu there is a ruby 2.4 scl too (which we might change to too?)

@dLobatog largely correct, though Fedora and EL7 are currently packaged with, and would remain on Ruby on Rails 4.2 until it's changed.
@ohadlevy not at this time.

Adds configurable support for running on Ruby on Rails 5.0 or 4.2, defaulting now to 5.0 on Ruby 2.3 or higher. Ruby versions with partial or no support remain using 4.2 by default. Other points: 1. SETTINGS[:rails] is loaded pre-Rails boot, then post-boot the majority of the SETTINGS are loaded depending on the environment. settings.yaml.dist will allow packagers to override the default version of Rails loaded to match their distribution. 2. rails-controller-testing, record_tag_helper replace extracted Rails 4.x features. 3. ParamsParser middleware is no longer loaded by default and is deprecated, CatchJsonParseErrors is no longer order dependent. 4. quiet_assets is not supported under Rails 5, will be replaced under ticket #18500, only affects development logging.

domcleal · 2017-06-06T08:43:16Z

could you provide a way to preserve the existing rake task interface behavior
for exampel at https://github.com/theforeman/foreman_discovery/blob/develop/lib/foreman_discovery/engine.rb#L210

this breaks every plugin that adds rake tasks by needing to require 'rake/testtask'

Done, however it will be hard to remove this again in the future via a deprecation. Plugins should require files they use.

PR updated.

ohadlevy · 2017-06-06T11:15:03Z

@jlsherrill @dLobatog any idea on the katello failures? is it due to a change in foreman or katello?

dLobatog · 2017-06-06T13:33:27Z

It looks like as soon as it gets to this test `Actions::Pulp::Repository::RemoveScheduleTest#test_remove_schedule`` it aborts (http://ci.theforeman.org/job/test_develop_pr_katello/2912/database=postgresql,ruby=2.2,slave=fast/consoleFull). I'll investigate it further but I don't think this is expected to break, so it's probably changes in this PR.

dLobatog · 2017-06-06T13:44:15Z

Gemfile

+when '4.2'
+  gem 'rails', '4.2.8'
+when '5.0'
+  gem 'rails', '5.0.3'


I don't advocate for supporting both on every platform either, my question is more about what to do once this is merged:

CentOS/RHEL for 1.16 will probably have the required SCLs to run this without a problem (so this is fine)

Fedora 25+ doesn't need anything AFAIK (https://fedoraproject.org/wiki/Changes/Ruby_on_Rails_5.0). Older fedoras cannot be supported

Debian only ships Ruby 2.3+ on stretch and sid (jessie is 2.1.5). So it should ship with the 4.2 version.

Ubuntu xenial, yakkety and zesty all have 2.3+. Trusty, however, is still on 1.9.3

Therefore, Debian Jessie and Ubuntu Trusty if we want to keep maintaining them will have to be 4.2, while the rest would be 5.0. Is that your original plan?

dLobatog · 2017-06-06T14:37:15Z

I'll check what are the problems with this + Katello and if everything's OK, merge sooner than later. I believe only Fedora 25, and newer versions of Ubuntu would be able to use this by default right now though.

jlsherrill · 2017-06-08T20:15:50Z

[test katello]

jlsherrill · 2017-06-08T20:39:15Z

The katello test seems to abort at different points, on the latest run it was at Actions::Katello::CapsuleContent::CreateReposTest#test_0001_creates repos needed on the capsule . Testing locally I'm not seeing this occur...

dLobatog · 2017-06-14T15:56:56Z

[test katello] same here - I cannot reproduce the failures on Rails 4.2, Rails 5.0 + Katello breaks currently with No such middleware to insert before: ActionDispatch::ParamsParser, probably https://github.com/Katello/katello/blob/master/lib/katello/engine.rb#L8

dLobatog · 2017-06-15T10:14:39Z

@jlsherrill I'm seeing a similar failure on http://ci.theforeman.org/job/test_develop_pr_katello/2985/database=postgresql,ruby=2.2,slave=fast/console , which corresponds to #4591 , I wouldn't be so sure it's this PR causing it.

ohadlevy · 2017-06-19T06:17:35Z

@jlsherrill can you confirm? thanks!

jlsherrill · 2017-06-19T12:50:26Z

@dLobatog the failure on http://ci.theforeman.org/job/test_develop_pr_katello/2985/database=postgresql,ruby=2.2,slave=fast/console looks completely different than the failure here. The failure there the tests don't even start and there is some postgresql issue.

The failure here gets into the tests but then randomly aborts. I still have not seen this failure anywhere else.

Lets try again [test katello]

tbrisker · 2017-07-04T14:49:36Z

Closing in favor of #4642. Thanks @domcleal !

theforeman-bot added Needs testing Not yet reviewed labels Mar 30, 2017

domcleal added the Waiting on contributor label Mar 30, 2017

domcleal force-pushed the 19050-rails50 branch from d87a375 to 78ef84d Compare March 30, 2017 10:06

theforeman-bot added Needs testing and removed Waiting on contributor labels Mar 30, 2017

domcleal force-pushed the 19050-rails50 branch 4 times, most recently from 05a2e50 to a5976c0 Compare April 3, 2017 13:37

domcleal force-pushed the 19050-rails50 branch 3 times, most recently from 1dd56c1 to 5386b62 Compare May 18, 2017 09:00

ohadlevy reviewed Jun 1, 2017

View reviewed changes

dLobatog reviewed Jun 2, 2017

View reviewed changes

domcleal force-pushed the 19050-rails50 branch from 5386b62 to 3c2b1d6 Compare June 6, 2017 08:40

dLobatog approved these changes Jun 6, 2017

View reviewed changes

mmoll mentioned this pull request Jul 4, 2017

fixes #19050 - add Ruby on Rails 5.0 support #4642

Merged

tbrisker closed this Jul 4, 2017

fixes #19050 - add Ruby on Rails 5.0 support #4422

fixes #19050 - add Ruby on Rails 5.0 support #4422

Conversation

domcleal commented Mar 30, 2017 • edited Loading

mention-bot commented Mar 30, 2017

ohadlevy commented Mar 30, 2017

domcleal commented Mar 30, 2017

domcleal commented Mar 30, 2017 • edited Loading

domcleal commented Apr 3, 2017

ohadlevy commented Apr 4, 2017

domcleal commented Apr 4, 2017

ohadlevy commented Apr 4, 2017

domcleal commented Apr 4, 2017

ohadlevy commented May 26, 2017

ohadlevy commented May 26, 2017

ohadlevy commented May 26, 2017

ohadlevy commented May 26, 2017 • edited Loading

ohadlevy commented May 28, 2017

ohadlevy commented May 28, 2017

domcleal commented May 30, 2017

ohadlevy commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dLobatog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Jun 6, 2017

ohadlevy commented Jun 6, 2017

dLobatog commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

dLobatog commented Jun 6, 2017

jlsherrill commented Jun 8, 2017

jlsherrill commented Jun 8, 2017

dLobatog commented Jun 14, 2017 • edited Loading

dLobatog commented Jun 15, 2017

ohadlevy commented Jun 19, 2017

jlsherrill commented Jun 19, 2017

tbrisker commented Jul 4, 2017

domcleal commented Mar 30, 2017 •

edited

Loading

domcleal commented Mar 30, 2017 •

edited

Loading

ohadlevy commented May 26, 2017 •

edited

Loading

dLobatog commented Jun 6, 2017 •

edited

Loading

dLobatog commented Jun 14, 2017 •

edited

Loading