Rails 5 update #1341

sonalkr132 · 2016-06-25T19:04:38Z

Please read commit message for explanation of changes.

It is very likely that I may have missed deprecations which didn't show up in test, feel free to open issues when you see them.

I have removed quite_assets gem because their rails requirement wouldn't have allowed update and looks like gem is abandoned. The feature is now implemented in sprocket-rails, we just have to be wait for version release.

I really want to compare before and after performance, object allocation etc. I would really appreciate it if someone pointed me to relevant resources or took out time for general advice.

dwradcliffe · 2016-06-26T01:42:29Z

I haven't looked at the commits yet, but if there are any of these things that are compatible with both Rails 5 and the current version we should get those into separate PRs and merge them sooner. Smaller changes are always better to test.

cc: @arthurnn @rafaelfranca

sonalkr132 · 2016-06-26T10:47:10Z

if there are any of these things that are compatible with both Rails 5 and the current version we should get those into separate PRs and merge them sooner.

I am not sure if that would be a good idea. You won't be able to see anything in the logs other than deprecation warnings.

This PR is not as bad as it looks. Just gemfiles change amount for 128 lines, id: .. => params: { id: .. } cloc at 138 lines and adding version to migration is 88 line (82% total change).

homu · 2016-06-26T22:22:17Z

☔ The latest upstream changes (presumably #1327) made this pull request unmergeable. Please resolve the merge conflicts.

kmcphillips · 2016-06-27T12:25:47Z

To remove quiet_assets you need to add config.assets.quiet = true to environments/development.rb for the equivalent behaviour.

As soon as there is a sprockets-rails release we can make that change separate to this pr. cc @rafaelfranca

sonalkr132 · 2016-06-27T12:48:03Z

Thank you for checking out the PR @kmcphillips

you need to add config.assets.quiet = true to environments/development.rb for the equivalent behaviour.

I didn't add it because I didn't want to add a configuration which did nothing at the point(commit) it was added. quite_assets isn't anything critical as in we can live without it for a day or two. I hope it makes sense 😅

kmcphillips · 2016-06-28T15:19:18Z

#1352 simplifies this a bit. sprockets-rails 3.1.0 was released today.

sonalkr132 · 2016-06-28T16:35:33Z

I have great news.. 😞

It has treading issue and as they go.. it is not reliably reproduceable. Failing build1, build2, build3.
Almost all of them are on increment function of GemDownload. How do I even report this?

homu · 2016-07-01T23:47:08Z

☔ The latest upstream changes (presumably f32321d) made this pull request unmergeable. Please resolve the merge conflicts.

sonalkr132 · 2016-07-06T09:23:33Z

I have updated this to 5.0.0. Treading error was fixed here: rails/rails#25707
I guess we will have to wait for 5.0.1.

prathamesh-sonpatki · 2016-07-06T11:08:17Z

config/application.rb

@@ -28,6 +25,9 @@ class Application < Rails::Application
    config.plugins = [:dynamic_form]

    config.autoload_paths << Rails.root.join('lib')
+
+    # Do not halt callback chains when a callback returns false.
+    ActiveSupport.halt_callback_chains_on_return_false = false
  end


@sonalkr132 Did you run rails app:update, this config will go to config/initializers/new_framework_defaults.rb after that.

I was hoping I would change config after we are sure that changes up to this point don't break anything.
However, now it seems like it is going to be a while before this gets deployed.. I have added the changes of rails app:update. Thanks for having a look at the PR :)

homu · 2016-07-08T18:10:31Z

☔ The latest upstream changes (presumably e0006da) made this pull request unmergeable. Please resolve the merge conflicts.

simi · 2016-07-11T18:44:03Z

Gemfile

-group :development do
-  gem 'rails-erd'
-end
+gem 'rails-erd', group: :development


Is this needed?

It is used by https://github.com/rubygems/rubygems.org/blob/071df7d625ee18be64d225e78abb445fdbf17462/lib/tasks/gen_erd.rake

Sure, but why changed?

Oh! It has to do with rebase 😝 I had removed quite_assets changed group to one line but quite_assets was removed in different PR and I had to rebase.

I guess it saves two lines, but yes, the change is unnecessary. May be I can restore it when I update the PR for 5.0.1 if you think it is important.

Since this PR is changing 169 files I think it will be nice to keep it as minimal as it is possible for future reviewers.

simi · 2016-07-12T08:26:57Z

I'm not sure if ApplicationRecord is needed and it will save a lot of line changes since it is not used (it is just blank abstract class). Is it really worth it now?

simi · 2016-07-12T08:28:30Z

app/controllers/profiles_controller.rb

-    @user           = User.find_by_slug!(params[:id])
-    rubygems        = @user.rubygems_downloaded
-    @rubygems       = rubygems.slice!(0, 10)
-    @extra_rubygems = rubygems


Is this removal of @extragems_rubygems and rubygems var assignments and following view change related to Rails 5 update or is it just a code cleanup/refactoring?

simi · 2016-07-12T08:32:29Z

I left some notes @sonalkr132 to your awesome work!.
Anyway I'm just random passerby so it will be good to wait for someone from RubyGems.org team to reply if those comments make sense.

…emplate HTTP request methods will accept only the following keyword arguments in future Rails versions: params, headers, env, xhr, as Rails discourages use of assert_template and the feature is moved out to gem. Possibly we will be adding tests which test for elements in view.

`assigns` is moved out to a gem in rails 5 and its use is discouraged.

Use of plain fixes: DEPRECATION WARNING: `render :text` is deprecated because it does not actually render a `text/plain` response. Switch to `render plain: 'plain text'` to render as `text/plain`, `render html: '<strong>HTML</strong>'` to render as `text/html`, or `render body: 'raw'` to match the deprecated behavior and render with the default Content-Type, which is `text/plain`. Use of request.env fixes: DEPRECATION WARNING: env is deprecated and will be removed from Rails 5.1 Use of request.env mentioned at: rails/rails@05934d2

Reusing params in ES suggestion view was showing error for unsafe use of params. `count` takes `conditions` as argument , but we don't need even that. Sytax of content_type changed. Rails now uses async queue_adapter which uses a different thread to work off jobs. We can't let that happen in our integration test because we are relying on the mail being delievered and doing that in a different thread leads to unpredictable delay.

Use of symbol or string as middleware name is deprecated. I moved it to env files because Redirect was env specific. Now errors in transactional callbacks are raised by default. serve_static_file and static_cache_control has been deprecated in favour of public_file_server. Tests now run after_commit callbacks by default. New rails test runner is cool 😎

We are not adding puma and actioncable config here.

indirect · 2016-08-27T07:24:04Z

@sonalkr132 can you re base this and update to 5.0.1 to fix the connection pool issue?

sonalkr132 · 2016-08-27T07:43:34Z

Rails 5.0.1 is not released yet. rails 5.0.1 Milestone
Rails 5.0.0.1 was released and it was just a security update.

indirect · 2016-08-27T09:46:53Z

@sonalkr132 ah, I see. 😞 I guess this is blocked until Rails 5.0.1 comes out, then.

homu · 2016-09-04T20:47:09Z

☔ The latest upstream changes (presumably #1398) made this pull request unmergeable. Please resolve the merge conflicts.

arthurnn · 2016-09-14T16:19:13Z

I agree 100% with @dwradcliffe . We cannot ship one big update PR to upgrade Rails version. it is too risky.
We need to slowly introduce the changes in master, and make sure they run on both versions. Once we have master able to run on rails 4.2 and 5, and we test it out the move to 5

connorshea · 2016-10-03T21:26:56Z

test/functional/api/v1/profiles_controller_test.rb

@@ -47,7 +47,7 @@ def response_body
      context "on GET to show when hide email" do
        setup do
          @user.update(hide_email: true)
-          get :show, id: @user.handle, format: format
+          get :show, params: { id: @user.handle }, format: format


Changes like this should be done in a follow-up PR, in my opinion. This adds unnecessary review overhead to the Pull Request.

connorshea · 2016-10-03T21:27:38Z

This would be a lot smaller if we didn't bother fixing deprecation warnings in this PR. We should focus on doing that afterward.

connorshea

Some comments on what we should remove from this PR to keep it lean, what we can backport, etc.

connorshea · 2016-10-03T21:28:39Z

Gemfile

@@ -16,7 +16,7 @@ gem 'daemons'
 gem 'dalli'
 gem 'delayed_job'
 gem 'delayed_job_active_record'
-gem 'doorkeeper'
+gem 'doorkeeper', '~> 4.0.0.rc4'


This can be backported (and can use the stable 4.0.0 version)

This is actually gone in master now.

connorshea · 2016-10-03T21:29:56Z

Gemfile.lock

    nokogiri (1.6.8)
      mini_portile2 (~> 2.1.0)
      pkg-config (~> 1.1.7)
    parser (2.3.1.2)
      ast (~> 2.2)
-    paul_revere (2.0.0)
-      rails (~> 4.0)
+    paul_revere (2.1.0.rc1)


paul_revere seems to be abandoned from what I can tell, can we replace it with something else?

connorshea · 2016-10-03T21:30:38Z

app/controllers/api/v1/api_keys_controller.rb

@@ -7,9 +7,9 @@ def show
      sign_in User.authenticate(username, password)
      if current_user
        respond_to do |format|
-          format.any(:all) { render text: current_user.api_key }
+          format.any(:all) { render plain: current_user.api_key }


Replacing render text with render plain seems like an unnecessary deprecation warning fix? Should be a follow-up PR.

connorshea · 2016-10-03T21:31:16Z

config/initializers/filter_parameter_logging.rb

+# Be sure to restart your server when you modify this file.
+
+# Configure sensitive parameters which will be filtered from the log file.
+Rails.application.config.filter_parameters += [:password]


Does this override an existing filter_parameters config?

connorshea · 2016-10-03T21:32:33Z

app/controllers/api/base_controller.rb

@@ -1,7 +1,6 @@
 class Api::BaseController < ApplicationController
  before_action :doorkeeper_authorize!, if: :doorkeeper_token
  before_action :authenticate_with_oauth, if: :doorkeeper_token
-  skip_before_action :require_ssl


What's the explanation for this change?

connorshea · 2016-10-03T21:33:26Z

Gemfile.lock

@@ -142,7 +144,7 @@ GEM
      activesupport (>= 4.1.0)
    gravtastic (3.2.6)
    hashie (3.4.4)
-    high_voltage (2.4.0)
+    high_voltage (3.0.0)


This seems to be something we can backport.

sonalkr132 · 2016-10-06T10:38:43Z

Thanks for review @connorshea I will keep your comments in mind when I make changes.

We cannot ship one big update PR to upgrade Rails version. it is too risky.

If it's okay I would like to open a new PR with ☝️. It doesn't it has to wait for rails 5.0.1 for people to review it.

We need to slowly introduce the changes in master, and make sure they run on both versions.

I haven't read (blog/implementation) anything related to this. Can someone please explain it with a little more detail? Would it be something like this, where we check for rails versions?

connorshea · 2016-10-31T07:49:04Z

@sonalkr http://shayfrendt.com/posts/upgrading-github-to-rails-3-with-zero-downtime/

connorshea · 2016-10-31T08:04:05Z

See also what I'm doing for GitLab: https://gitlab.com/gitlab-org/gitlab-ce/issues/14286

Lots of things can be backported to master to make this PR a lot smaller.

sonalkr132 · 2016-10-31T18:47:03Z

@connorshea thanks :) That was helpful. I will send a new PR soon.

sonalkr132 · 2017-05-01T15:21:28Z

See #1615

sonalkr132 force-pushed the rails-5-update branch from b66ac1c to b62d848 Compare June 27, 2016 12:38

sonalkr132 force-pushed the rails-5-update branch 2 times, most recently from 06ca8f3 to 5f41680 Compare June 28, 2016 15:16

kmcphillips mentioned this pull request Jun 28, 2016

Replace quiet_assets gem with sprockets-rails 3.1.0 config #1352

Merged

sonalkr132 force-pushed the rails-5-update branch from 5f41680 to 49c388a Compare June 28, 2016 15:27

sonalkr132 force-pushed the rails-5-update branch 4 times, most recently from 17cb351 to 2d7257e Compare July 5, 2016 10:26

prathamesh-sonpatki reviewed Jul 6, 2016
View reviewed changes

sonalkr132 force-pushed the rails-5-update branch 4 times, most recently from 384b0fa to 8133cc1 Compare July 6, 2016 13:39

sonalkr132 force-pushed the rails-5-update branch from 8133cc1 to d804fe4 Compare July 11, 2016 07:19

simi reviewed Jul 11, 2016
View reviewed changes

simi reviewed Jul 12, 2016
View reviewed changes

sonalkr132 added 6 commits August 12, 2016 09:15

Remove assertion for assignment

3d919db

`assigns` is moved out to a gem in rails 5 and its use is discouraged.

Update doorkeeper to fix before_filter deprecation

a96c138

sonalkr132 force-pushed the rails-5-update branch 2 times, most recently from 6819da7 to e696b48 Compare August 12, 2016 03:50

Run rails app:update to update config

29940ef

We are not adding puma and actioncable config here.

sonalkr132 force-pushed the rails-5-update branch from e696b48 to 29940ef Compare August 12, 2016 04:31

sonalkr132 added needs-more-work health labels Sep 14, 2016

connorshea reviewed Oct 3, 2016

View reviewed changes

sonalkr132 added the backlog label Dec 7, 2016

This was referenced Dec 27, 2016

Remove unused function, append to collection and fix unsafe used of params #1526

Merged

Remove render_template and assigns from tests #1527

Merged

sonalkr132 closed this May 1, 2017

sonalkr132 deleted the rails-5-update branch June 1, 2019 08:47

Rails 5 update #1341

Rails 5 update #1341

Conversation

sonalkr132 commented Jun 25, 2016

dwradcliffe commented Jun 26, 2016

sonalkr132 commented Jun 26, 2016 • edited Loading

homu commented Jun 26, 2016

kmcphillips commented Jun 27, 2016

sonalkr132 commented Jun 27, 2016

kmcphillips commented Jun 28, 2016

sonalkr132 commented Jun 28, 2016 • edited Loading

homu commented Jul 1, 2016

sonalkr132 commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homu commented Jul 8, 2016

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

simi commented Jul 12, 2016

Choose a reason for hiding this comment

simi commented Jul 12, 2016

indirect commented Aug 27, 2016

sonalkr132 commented Aug 27, 2016

indirect commented Aug 27, 2016

homu commented Sep 4, 2016

arthurnn commented Sep 14, 2016

Choose a reason for hiding this comment

connorshea commented Oct 3, 2016

connorshea 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

Choose a reason for hiding this comment

sonalkr132 commented Oct 6, 2016

connorshea commented Oct 31, 2016

connorshea commented Oct 31, 2016

sonalkr132 commented Oct 31, 2016

sonalkr132 commented May 1, 2017

sonalkr132 commented Jun 26, 2016 •

edited

Loading

sonalkr132 commented Jun 28, 2016 •

edited

Loading