Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Rails 5.1 #1108

Merged
merged 10 commits into from Aug 9, 2017
Merged

Upgrade to Rails 5.1 #1108

merged 10 commits into from Aug 9, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 3, 2017

Questions:

  • We have some initialisers that remove XML and JSON parsers from the default parsers. These were added in 2013: #177. Parsers have moved so this no longer works, I'm not sure whether they are still needed. Commented out for now. Initializers removed in 9f689d6
  • Sprockets seems to be including asset digest URLs in tests now – I've updated the tests in 053e08a so they are less strict about what they expect but I'm not sure if this is a symptom of the slower sprockets on development. Set digest assets to false in test environment (05bf7cb, based on http://railsdiff.org/4.2.7.1/5.1.2)

As part of the Rails 5 upgrade:

  • Refactored the root routes. The controller method can no longer take two arguments. I believe the change I've added is equivalent.
  • Remove quiet_assets gem, it's now config
  • Switch from render text: and render nothing:, they were deprecated and now removed
  • Use rack_strip_client_ip 0.0.2 for Rails 5 compatibility
  • Update actionpack-page_cachingto 1.1.0 for Rails 5 support
  • Update airbrake to remove middleware errors (matching government-frontend version). Further upgrades possible.
  • Stop pinning sprockets-rails
  • Update nokogiri to avoid security vulns
  • Update jasmine-rails for Rails 5

In 6976fc0 I've also updated a lot of the Rails boilerplate code to be more modern. Though I've kept this intentionally minimal.

The two Snyk vulnerabilities have no known fixes.

As of sprockets 3.1.0 the gem is now deprecated:
https://github.com/evrone/quiet_assets

Replace with config flag in development.rb
@fofr fofr force-pushed the rails-upgrade branch Aug 3, 2017

gem 'uglifier', ">= 1.3.0"
gem 'sass-rails', "5.0.6"
gem 'airbrake', '~> 4.3.1'

This comment has been minimized.

@h-lame

h-lame Aug 3, 2017
Contributor

In most apps we've been using our own fork of airbrake 4 to get rails 5 compatibility and avoid losing some functionality on our errbit install (see: https://docs.publishing.service.gov.uk/manual/upgrade-rails.html#airbrake)

This comment has been minimized.

@fofr

fofr Aug 3, 2017
Author Contributor

I've updated in 60798cd and removed 3e6b9d1

This comment has been minimized.

@h-lame

h-lame Aug 3, 2017
Contributor

👍

This comment has been minimized.

@boffbowsh

boffbowsh Aug 8, 2017
Contributor

So excited to switch this to Sentry soon...

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 3, 2017

We added gem 'actionpack-page_caching', '1.1.0' in the Rails 4 upgrade, but it looks like we haven't included the config it needs: https://github.com/rails/actionpack-page_caching#usage

@fofr fofr force-pushed the rails-upgrade branch 2 times, most recently Aug 3, 2017
fofr added 9 commits Aug 2, 2017
* Parsers have moved in Rails 5
* Security fixes from 2012 no longer required
* Use rack_strip_client_ip 0.0.2 for Rails 5 compatibility
* Update `actionpack-page_caching`to 1.1.0 for Rails 5 support
* Use forked version of airbrake rather than upgrade to avoid
  breaking errbit:
  - https://docs.publishing.service.gov.uk/manual/upgrade-rails.html#airbrake
* Stop pinning sprockets-rails
* Update nokogiri to avoid security vulns
Render text: and nothing: were deprecated and removed in Rails 5.1

Switch to `head :not_found` and `render plain:`
As of Rails 5 two arguments can’t be passed to the controller method.

* Use a format: false scope
* Remove need for “controller” method wrapping block
Run `rails app:update` and take on minimal changes
Fixes: “NoMethodError: undefined method `alias_method_chain' for
Sprockets::Rails::Helper:Module”

https://github.com/searls/jasmine-rails/blob/master/CHANGELOG.md
* Rails 5 now includes hashes for assets in test by default
* Add config to disable digest
* Update cache control config to Rails 5.1 defaults
* Include eager_load comments for clarity
* phase state is no longer needed. All phase labels are govuk_blue
@fofr fofr force-pushed the rails-upgrade branch to dd0ef63 Aug 8, 2017
@fofr fofr changed the title [Do not merge] Upgrade to Rails 5.1 Upgrade to Rails 5.1 Aug 8, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 8, 2017

Removed old initialisers for XML/JSON after discussing with @boffbowsh. (9f689d6)

Sprockets slow downs will not affect production and can be investigated separately to this PR. I will track with an issue.

@fofr fofr requested a review from boffbowsh Aug 8, 2017
Copy link
Contributor

@boffbowsh boffbowsh left a comment

LGTM. Test on integration first though I reckon.

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 8, 2017

@boffbowsh It's on integration now if you wanted to give it a once-over.

@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Aug 8, 2017

You'll be able to test it better than me, @fofr

@fofr fofr requested a review from binaryberry Aug 9, 2017
Copy link
Contributor

@binaryberry binaryberry left a comment

There now seems to be 3 Snyk vulnerabilities - is there a known fix for the third one?

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 9, 2017

I can't see the 3rd issue, only 2 are showing on Snyk itself.
screen shot 2017-08-09 at 15 26 28

@binaryberry
Copy link
Contributor

@binaryberry binaryberry commented Aug 9, 2017

Ah ok. Waiting to get access to Snyk myself, so wasn't able to click through to the details. Interesting there is a discrepancy, but shouldn't be a blocker. I've approved now!

@fofr fofr merged commit d65a6f3 into master Aug 9, 2017
1 of 2 checks passed
1 of 2 checks passed
security/snyk 3 new vulnerable dependency paths
Details
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the rails-upgrade branch Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.