Upgrade from Ruby v2.2.4 to v2.3.0 #2345
Conversation
|
Can we update the PR description with the motivation for upgrading the version of Ruby? |
|
Is Ruby 2.3.0 already available on CI and all staging/production machines? |
|
@chrisroos there are a handful of apps running 2.3.0 in production now |
|
In the description, it might be worth adding that the unicorns need to be restarted when this is deployed. When we upgraded Ruby last time, we saw that the child unicorn processes didn't pick up the new Ruby version without restarting the parent unicorn process. |
floehopper
force-pushed
the
upgrade_from_ruby_2_2_4_to_ruby_2_3_0
branch
from
049e207 to
c75e11d
Compare
|
I've just addressed some of the issues raised by @chrisroos & @erikse which involved re-writing the history on the branch i.e. I've just force-pushed. |
|
I've just updated the PR description to explain my motivations for the PR as requested by @chrisroos. |
|
I've just added a note to the PR description about restarting unicorn processes on deployment as suggested by @erikse. |
|
After @chrisroos highlighted the risk of merging this PR with the What do people think we should do? I don't really want to leave this PR open and not merged - I'd prefer either to close it and re-visit it later or merge it as it stands. |
|
Given that it's only a warning, and we don't expect it to happen in production[1], I think it's probably fine to merge this branch. [1]: I think we'll see the warning in production if someone manually changes the country slug in a URL to something that doesn't exist in the API (e.g. https://www.gov.uk/marriage-abroad/y/foobar). |
It's good practice to implement `#respond_to_missing` whenever you implement `#method_missing` on a class. I'm fixing this as part of the PR to upgrade Ruby to v2.3, because we ran into some changes in `OpenStruct` which caused unexpected exceptions in tests, and having this behaviour working correctly will help us diagnose those problems.
There have been some changes to the implementation of `OpenStruct` in Ruby v2.3. These tests capture some of the behaviour that we are relying on. They should make it easier to diagnose and fix the problems that the upgrade will introduce. They all pass at this point i.e. under Ruby v2.2.4.
The app is already using Bundler v1.11.2 which is the default for Ruby v2.3.
|
@chrisroos: Thanks. |
|
I'm about to rebase against master, apply the fixup commit, and force-push in preparation for merging. |
Some changes have been made to the implementation of `OpenStruct` in Ruby v2.3: * Define OpenStruct attributes lazily [1] * Finish defining OpenStruct attributes lazily [2] This means that attribute methods are now not defined until they are called for the first time. This lazy definition of attribute methods is achieved in `OpenStruct#method_missing`. This in turn means that `State#method_missing` is now invoked when one of these attributes is accessed for the first time - it wasn't being invoked previously. Our implementation of `State#method_missing` was only calling `super` for writer methods and so incorrectly raised a `NoMethodError` exception when one of the default attributes was accessed for the first time. I've changed the implementation of `State#method_missing` so that it uses `OpenStruct#respond_to_missing` to check whether or not the attribute exists. This seems to fix all the failing tests. [1]: ruby/ruby#1033 [2]: ruby/ruby#1037
After the upgrade to Ruby v2.3.0, the following warning started to appear in
the test output:
$ ruby test/unit/world_location_test.rb -n "/return nil if not found/" -v
Run options: -n "/return nil if not found/" -v --seed 37197
# Running:
[warning] The response contained in an RestClient::Exception is now a RestClient::Response instead of a Net::HTTPResponse, please update your code
WorldLocationTest#test_: finding a location by slug should return nil if not found. = 0.02 s = .
Finished in 0.025900s, 38.6108 runs/s, 38.6108 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips
The problem seems to occur when a 404 response is turned into an exception by
the `rest-client` gem and then that exception is rescued in the
`gds-api-adapters` gem. In particular it seems to be related to the lines
where the `http_body` on the exception is converted into JSON for logging [1].
I'm pretty sure this problem is related to this [issue][2] in `rest-client`,
but reading the comments, it sounds as if there's not going to be a fix until
v2.0.0 is released. We're currently using the latest in the 1.x series, v1.8.
As far as I can tell the problem isn't fixed in newer versions of
`gds-api-adapters` either.
So I've worked around the problem by stubbing at the `Services.worldwide_api`
level just for the relevant test. Personally I think this is a better level to
stub at rather than using the `gds-api-adapters` helper methods which use
`WebMock` to stub at a very low level. Ideally we'd change all the tests in
`WorldLocationTest` to work this way, but that would be a fair bit more work.
[1]: https://github.com/alphagov/gds-api-adapters/blob/v25.1.0/lib/gds_api/json_client.rb#L266-L267
[2]: rest-client/rest-client#454
floehopper
force-pushed
the
upgrade_from_ruby_2_2_4_to_ruby_2_3_0
branch
from
5807c89 to
b137093
Compare
…y_2_3_0 Upgrade from Ruby v2.2.4 to v2.3.0
Description
This upgrades Ruby from v2.2.4 to v2.3.0 and addresses a couple of issues which this caused.
I've addressed the problem with
SmartAnswer::Stateand the changes toOpenStruct. I've also worked around a warning that was being generated from theWorldLocationTest. All the tests now pass. Also the full regression test suite passes. See individual commit notes for details.Motivation
My main motivations for working on this were (a) the PR had been open for a long time and I kept seeing it in the list; and (b) the
CGI.escapeHTMLoptimisation (ruby/ruby#1164) examined in the Is Ruby 2.3 Faster? Rails ERB Template Rendering Performance article which @erikse linked to in #2269.Deployment
The unicorn processes should be restarted after this PR is deployed. @erikse says that when we upgraded Ruby last time, we saw that the child unicorn processes didn't pick up the new Ruby version until their parent process was restarted.
This supersedes #2269.