Update to Rails 5 #1688
Update to Rails 5 #1688
Conversation
|
Awesome, nice work! I'll give this some testing over the weekend. Did you use http://railsdiff.org/? |
|
Yes that tool is a lifesaver 😄 |
|
I tested Heroku and am deploying this to my personal instance to see how it runs. |
|
Great! I am have the single-process docker container running for a few days without errors and will verify Evernote soon. Do you know weibo or tumblr users? |
|
We can see if @deanputney or @oroce would be up for testing this with Tumblr, and I know @albertsun uses Weibo. @dsander, do you use MySQL or PostgreSQL in production? I've gone down an upgrade rabbit hole and need to pick which one I use for a new Huginn instance, and I should probably pick whichever you don't use so that we get more coverage. (I tried to push this branch to my instance, which was running an older (supported) version of Ruby. Rails 5 needs a newer version, so I tried to upgrade, only to discover that the Ubuntu LTS version I was on is too old, and apt sources needed to be pointed at a legacy server, but I figured, I might as well update the server to a newer LTS, but that broke with tons of errors, so I decided to provision a new server and install Huginn with Ansible, so then I started writing Ansible scripts... anyway, I'm sure I'll be done sometime soon! 😄)
|
|
@cantino Hah, the ruby version was one of the reasons I switched to docker a few months ago 😄 . I am running PostgreSQL both in production and development. Sounds like you are almost done 😛 |
|
I've deployed your Rails 5 branch to my instance. Seems fine so far. |
|
Awesome, I found and fixed a few bugs today and tested evernote as well as the user location agent. |
|
Updated my installation. @albertsun, would you be able to test Weibo by any chance? |
|
Yes, I'll test Weibo Just curious - I know I can build a Docker image from this PR myself, but is there a pre-built one for this branch being built by a CI tool somewhere? |
|
@albertsun Yes there is, the image builder is reporting in the "checks section" at the bottom. |
|
Ok - so weibo_2 isn't working. But I actually haven't been using it in production for a while now either and I tested it on master and it has the same error there that I think is related to weibo changing how they enforce authorization for their API. Error is: The right way to do this would be to update weibo to be a Service the way Twitter and other things are — the way access tokens were passed in as options was always hacky. I'm wondering if anyone is actually using this agent at all anymore? |
|
@albertsun Thanks for testing it. I think you are right if we want to keep the Agents we should migrate them to use the oAuth services. Would you be willing to work on that in a separate pull request? |
|
Would it be worth removing Weibo and if anyone complains, they can pull it into a gem? |
|
Yea, I think we could remove it. I can open an issue and add it to my list to work on it as a service, but truth be told it's not going to be very high priority and am unlikely to get to it any time soon. |
|
This has been deployed on my system for a bit without issue. How's it going on yours? |
|
It's running fine as well. I will squash a few commits and merge it tonight if you are fine with it. |
| @@ -8,6 +8,8 @@ def index | |||
| elsif params[:source_ids] | |||
| Event.where(agent_id: current_user.agents.where(id: params[:source_ids]).pluck(:id)) | |||
| .order("id DESC").limit(5) | |||
| else | |||
There was a problem hiding this comment.
Because of deep_munge an empty array in the params will be converted to nil and that results in @events being undefined.
| devise_parameter_sanitizer.for(:sign_up) { |u| u.permit(:username, :email, :password, :password_confirmation, :remember_me, :invitation_code) } | ||
| devise_parameter_sanitizer.for(:sign_in) { |u| u.permit(:login, :username, :email, :password, :remember_me) } | ||
| devise_parameter_sanitizer.for(:account_update) { |u| u.permit(:username, :email, :password, :password_confirmation, :current_password) } | ||
| devise_parameter_sanitizer.permit(:sign_up, keys: [:username, :email, :password, :password_confirmation, :remember_me, :invitation_code]) |
There was a problem hiding this comment.
I assume you tested a manual signup?
| def scenario_import_params | ||
| merges = params[:scenario_import].delete(:merges) | ||
| params.require(:scenario_import).permit(:url, :data, :file, :do_import) do |params| | ||
| params[:merges] = merges |
There was a problem hiding this comment.
Do we need params[:merges].permit! here?
There was a problem hiding this comment.
We don't I think its because that hash isn't directly passed to AR.
| @@ -1,4 +1,5 @@ | |||
| require 'json' | |||
| require 'google_calendar' | |||
There was a problem hiding this comment.
Can we require this in the receive method so it's not needed when gem_dependency_check is false?
| ActiveSupport.to_time_preserves_timezone = true | ||
|
|
||
| # Require `belongs_to` associations by default. Previous versions had false. | ||
| Rails.application.config.active_record.belongs_to_required_by_default = true |
There was a problem hiding this comment.
Will this cause errors in agent.rb's belongs_to :service, :inverse_of => :agents, or in agent_log.rb's inbound_event or outbound_event which are optional?
There was a problem hiding this comment.
It doesn't, but to be honest I don't know why. The docs sound like non optional belongs_to associations needs to be present, but the Agent and AgentLog creation works without it.
There was a problem hiding this comment.
While working on the resque branch I found the reason why it is working without changes: collectiveidea/delayed_job_active_record#128
We might need to fix that at some point.
Due to eager_loading being active in production helper classes in lib need to be explicitly required.
|
@cantino @albertsun Thanks for the review and testing this! |
|
Nice! |
|
An odd thing happened when I pulled and migrated after this was merged (unfortunately not in my console logs anymore) where my test db had no tables except for the schema migrations table. I had to drop and re-create the test db to run tests. Not sure if this PR or Knu's recent one caused it, but pretty odd. Tests all pass once I did that. |
|
(Which could definitely be user error on my part; let's see if anyone else reports anything odd with tests.) |
|
That sounds really odd, did you have spring and/or guard running when you switched branches? |
|
Should we now update documents and state that Ruby 2.2.2 is now a minimum requirement, or just allow users to stay with Rails 4 and Ruby 2.0/2.1? |
|
No, you're right @knu that Rails 5's minimum Ruby (Ruby 2.2.2+) is now our requirement. Do we reference that anywhere besides doc/manual/requirements.md? |
|
Right, totally forgot about that. I will go through the docs and wiki later and update them if it's required. |
The update went relatively smooth, the
weibo_2gem had theomniauthandhashiedependencies locked, all other gems are compatible with each other (even though I had to bump the versions of a lot of them).Most of the changes are caused by deprecations and configuration updates, the upgrade process can be followed best by looking at the individual commits.
I tried to test every action manually in development but it would be great if at least one other person could verify that.
Manual testing in production that still needs to be done:
In Rails 5 the default application server changed to
pumawhich is require to useActionCable. Since we do not use it (yet) I would like to keepunicornfor now and switch to puma when needed (or only offer it as an option as it requires some work to update "manual" installations).