Upgrade to Rails 5.1.1 #1912

dsander · 2017-02-24T21:20:40Z

Currently using 5.1.1, the update was done with the help of railsdiff as usual.

I did some additional manual testing and did not find any issues.

cantino · 2017-02-27T03:48:33Z

config/initializers/assets.rb

@@ -9,8 +9,3 @@
 # Add additional assets to the asset load path
 # Rails.application.config.assets.paths << Emoji.images_path

-# Precompile additional assets (application.js.coffee.erb, application.css, and all non-JS/CSS are already added)
-Rails.application.config.assets.precompile += %w( diagram.js graphing.js map_marker.js ace.js )


Does it automatically infer these from our requiring them or something?

sprocket-rails now requires a manifest.js file which is used as a configuration which assets should be precompiled.

Ah, ok, I see that you added that. But how does it know to generate diagram.js, graphing.js, map_marker.js, ace.js, etc. as final output packages so that they can be served? We link them directly, e.g. https://github.com/cantino/huginn/blob/5868c7b4b23516bce7f7a9867b2310d6ef85980b/app/views/agents/agent_views/user_location_agent/_show.html.erb#L3

Yeah ... that is a good question I don't know the answer to. I suspected that it would not work with additional configuration, but somehow it does:

root@38d92fad1b9d:/app# ls -al public/assets/*.js -rw-r--r-- 1 root root 803641 Feb 24 19:06 public/assets/ace-b7a9a36a7e16893758aa7910f8b51810fb1673b0a7f8b5495c369cfddf6751e7.js -rw-r--r-- 1 root root 432321 Feb 24 21:08 public/assets/ace-edaac82bab5f8f31f17d02dac1f82c356dab951ea95e48d977a9b0022a5b263c.js -rw-r--r-- 1 root root 301207 Feb 24 21:08 public/assets/application-af5d3ed2164b4c06a6ab10e891ff4e4cc24e3b8227e3c9b45b0190220f5c31ae.js -rw-r--r-- 1 root root 759678 Feb 24 19:06 public/assets/application-b53519a18e5154acfef3f481b6eb549f433c297ce5b5d20ff4f9c0e80a3e9a13.js -rw-r--r-- 1 root root 613 Feb 24 21:08 public/assets/diagram-778f361aa4ed8e87fe51c63d2c61bde329dbf89fc57f31a3f044ec3f2e9154b5.js -rw-r--r-- 1 root root 987 Feb 24 19:06 public/assets/diagram-8c003dba8f5583de4723c10c9633cbca33de071949984908797f6d478cc0e819.js -rw-r--r-- 1 root root 166668 Feb 24 21:08 public/assets/graphing-141c9aa5934e3446123bf8e6b735e9d731fa2c9c803ef997c5a73ad72ad090ea.js -rw-r--r-- 1 root root 333654 Feb 24 19:06 public/assets/graphing-5aa056a62d2dc93d05c4bd2f3d9c63395659962a7b923debb11d4b0d942fb8f5.js -rw-r--r-- 1 root root 0 Feb 24 21:08 public/assets/manifest-d0ff5974b6aa52cf562bea5921840c032a860a91a3512f7fe8f768f6bbe005f6.js -rw-r--r-- 1 root root 3 Feb 24 19:06 public/assets/manifest-e761884522a9ca2dac475b68bac1946c990fb9af8b676d3b6b45692a920c8d04.js -rw-r--r-- 1 root root 1350 Feb 24 19:06 public/assets/map_marker-06756fac4a853bdcde5557d1f2e0d5333441903cf97ec1e75414e5871cc1af22.js -rw-r--r-- 1 root root 356 Feb 24 21:08 public/assets/map_marker-d4e91cde532b7818afd0e878e3acc4f49e84af9e006976b5abfb08b5a865c549.js

My guess is the rules defined in manifest.js mean that every file in app/assets/javascript gets precompile into a separate js bundle

I guess that must be what's happening, and application.js.coffee does it's own bundling of components/* and pages/*.

dsander · 2017-04-29T09:39:52Z

This is now ready for manual testing, since we plugged a lot of holes in our test suite with/after the Rails 5.0 so I don't expect any issues.

I would like to wait for an official devise release before we merge. Having a release for delayed_job would be nice as well, but I don't know how actively maintained the gem is at the moment.

@knu Does Rails 5.1 solve the issues you had left with adding support for Ruby 2.4?

dsander · 2017-05-16T19:49:00Z

I would consider this ready for testing/review now. The branch is running on my instance for a few weeks without issues and devise released a new version with supports Rails 5.1.
It seem that delayed_job still needs some time until they release (collectiveidea/delayed_job#974).

knu · 2017-05-17T09:10:50Z

@dsander I've just updated my support_ruby2.4 PR/branch. How do we go about upgrading Ruby and Rails? I guess this PR can be merged first and then mine?

dsander · 2017-05-17T09:33:31Z

@knu It would be great if you could test this branch on your instance for a day or two. Our controller spec coverage is pretty good, but I don't use that many Agents (even though I don't think the Rails broke any Agent related code).

dsander · 2017-05-17T09:35:01Z

Ah, and yes I think we should merge this PR first to check if they fixed all the deprecation warnings.

knu · 2017-05-17T09:54:31Z

@dsander Just upgraded my instance which now bases on this PR. I'll see how it works.

knu · 2017-05-19T06:20:50Z

@dsander Looks like it is working without a problem.

knu · 2017-05-19T06:24:31Z

Gemfile.lock

      mini_portile2 (~> 2.1.0)
+    nokogiri (1.7.2)


This line looks suspicious. Maybe a merge mistake?

You are right, mini_portile2 is a nokogiri dependency 😄 I am surprised bundler did not complain about it or fix the Gemfile.lock

knu

@dsander Great work! Please check the comment for a suspicious change in Gemfile.lock.

knu · 2017-05-19T06:27:35Z

app/models/agent.rb

  has_many :links_as_source, :dependent => :delete_all, :foreign_key => "source_id", :class_name => "Link", :inverse_of => :source
  has_many :links_as_receiver, :dependent => :delete_all, :foreign_key => "receiver_id", :class_name => "Link", :inverse_of => :receiver
  has_many :sources, :through => :links_as_receiver, :class_name => "Agent", :inverse_of => :receivers
+  has_many :received_events, -> { order("events.id desc") }, :through => :sources, :class_name => "Event", :source => :events


Is this just a cosmetic change? It's OK if that's the case, I'm just curious whether the order matters now in Rails 5.1.

Rails 5.1 complains when a has_many :through association is defined to before the through association is defined. That's why it had to be moved after the definition of sources
rails/rails@f8ab3ae

That makes sense. I'll keep it in mind, thanks.

Use `to_unsafe_h` to convert unpermitted parameters to a Hash Specify rails version in migrations

In Rails 5.1 transactional tests share the same connection id between the webserver and test runner. This removes the need for special cleanup strategies. This speeds up the tests significantly, before: ``` Finished in 3 minutes 30.3 seconds (files took 5.46 seconds to load) ``` After: ``` Finished in 1 minute 41.61 seconds (files took 5.45 seconds to load) ``` rails/rails#28083

By passing an empty hash to `permit` strong parameters permits the complete hash. rails/rails@e86524c Refactor ApplicationController#agent_params

dsander · 2017-05-19T07:49:29Z

Thanks for testing an reviewing this @knu!

cantino · 2017-05-20T09:10:29Z

Nice!

cantino reviewed Feb 27, 2017

View reviewed changes

0xdevalias mentioned this pull request Mar 2, 2017

Support Ruby >=2.4 #1876

Merged

dsander force-pushed the rails51 branch from 4227058 to 68ba406 Compare April 11, 2017 20:52

dsander changed the title ~~WIP: Upgrade to Rails 5.1 (beta)~~ WIP: Upgrade to Rails 5.1 (rc1) Apr 11, 2017

dsander force-pushed the rails51 branch from 68ba406 to a2de6c2 Compare April 28, 2017 19:25

dsander changed the title ~~WIP: Upgrade to Rails 5.1 (rc1)~~ Upgrade to Rails 5.1 Apr 29, 2017

dsander force-pushed the rails51 branch from 36440f7 to 4c01804 Compare April 29, 2017 09:33

dsander closed this May 4, 2017

dsander reopened this May 4, 2017

dsander force-pushed the rails51 branch from 4c01804 to 6e23b69 Compare May 15, 2017 15:08

dsander changed the title ~~Upgrade to Rails 5.1~~ Upgrade to Rails 5.1.1 May 16, 2017

dsander force-pushed the rails51 branch from 6e23b69 to 63cf6ee Compare May 16, 2017 19:53

knu reviewed May 19, 2017

View reviewed changes

knu requested changes May 19, 2017

View reviewed changes

dsander added 3 commits May 19, 2017 09:24

Upgrade rails and configuration to 5.1.1

ff2615a

Use `to_unsafe_h` to convert unpermitted parameters to a Hash Specify rails version in migrations

Utilize strong parameter API to pass arbitrary hashes

1d3d3eb

By passing an empty hash to `permit` strong parameters permits the complete hash. rails/rails@e86524c Refactor ApplicationController#agent_params

dsander force-pushed the rails51 branch from 63cf6ee to 1d3d3eb Compare May 19, 2017 07:25

knu approved these changes May 19, 2017

View reviewed changes

dsander merged commit 625e8c7 into huginn:master May 19, 2017

dsander deleted the rails51 branch May 19, 2017 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Rails 5.1.1 #1912

Upgrade to Rails 5.1.1 #1912

dsander commented Feb 24, 2017 •

edited

cantino Feb 27, 2017

dsander Feb 27, 2017

cantino Feb 27, 2017 •

edited

dsander Feb 27, 2017

cantino Feb 27, 2017

dsander commented Apr 29, 2017

dsander commented May 16, 2017

knu commented May 17, 2017

dsander commented May 17, 2017

dsander commented May 17, 2017

knu commented May 17, 2017

knu commented May 19, 2017

knu May 19, 2017

dsander May 19, 2017

knu left a comment

knu May 19, 2017

dsander May 19, 2017

knu May 19, 2017

dsander commented May 19, 2017

cantino commented May 20, 2017

Upgrade to Rails 5.1.1 #1912

Upgrade to Rails 5.1.1 #1912

Conversation

dsander commented Feb 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cantino Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsander commented Apr 29, 2017

dsander commented May 16, 2017

knu commented May 17, 2017

dsander commented May 17, 2017

dsander commented May 17, 2017

knu commented May 17, 2017

knu commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knu 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

dsander commented May 19, 2017

cantino commented May 20, 2017

dsander commented Feb 24, 2017 •

edited

cantino Feb 27, 2017 •

edited