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.

Sign up for GitHub

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

Jump to bottom

AP950 Upgrade to Rails 6 #1240

Merged
merged 14 commits into from
Feb 21, 2020
Merged

AP950 Upgrade to Rails 6 #1240

merged 14 commits into from
Feb 21, 2020

Conversation

Obsiye
Copy link
Contributor

@Obsiye Obsiye commented Feb 13, 2020 •
edited
Loading

Upgrade Rails to 6.0.2

Link to story

  • Upgrade rails to version 6.0.2

  • Revert sprockets gem to 3.7.2 , which fixes a segmentation error in CircleCi after upgrading to rails 6 this is when the docker file runs assets:precompile
    Related sass issue

  • Revert to using classic as the rails autoloader instead of the rails 6 default of zeitwerk. This fixed issues relating to a loading in rails_helper require error :

File.expand_path('../../config/environment', __FILE__)
FrozenError:
  can't modify frozen
  • Keep sidekiq at version 5.7.2 because version 6 requries zeitwerk (which we don't use)
  • Update layouts helpers rendering of files
  • Update Module#parent to #module_parent due to deprecation
  • Update tests to use #media_type instead of #content_type
  • Update form builder test to use action controller view context
  • Update to use MailDeliveryJob instead of DeliveryJob after rails 6 upgrade
  • Update config/application.rb after rails 6 upgrade
  • Set delivery job to use GovukNotifyMailerJob to allow email monitoring

  1. Remove mailer tests in the request tests. This is because sidekiq is overridden in request tests by the test adapter (in Rails 6) and thus sidekiq mailer tests in the requests were faulting Rails 6 inconsistently overrides ActiveJob queue_adapter setting with TestAdapter rails/rails#37270 and the solution is to continue using the test adapter as the mailers are tested seperately.

  2. Add type :request to request tests

  3. Update tests and cassettes

Checklist

Before you ask people to review this PR:

  • Tests and rubocop should be passing: bundle exec rake
  • Github should not be reporting conflicts; you should have recently run git rebase master.
  • There should be no unnecessary whitespace changes. These make diffs harder to read and conflicts more likely.
  • The PR description should say what you changed and why, with a link to the JIRA story.
  • You should have looked at the diff against master and ensured that nothing unexpected is included in your changes.
  • You should have checked that the commit messages say why the change was made.

@Obsiye Obsiye added the WIP Work In Progress label Feb 13, 2020
@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch from d91e4b2 to e44b28a Compare February 13, 2020 13:36
@Obsiye Obsiye changed the title Ap 950 upgrade rails AP950 upgrade rails Feb 13, 2020
@Obsiye Obsiye changed the title AP950 upgrade rails AP950 Upgrade to Rails 6 Feb 13, 2020
@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch 9 times, most recently from e71dde8 to 43feeb7 Compare February 13, 2020 22:37
@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch from 43feeb7 to 824765f Compare February 14, 2020 13:43
@Obsiye Obsiye added ready for review Please review and removed WIP Work In Progress labels Feb 14, 2020
colinbruce
colinbruce approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@colinbruce colinbruce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💥 🏆
Nice work, love the clarity of the commit messages and love the comments to highlight
why possible counter-intuitive changes (e.g, rollback of zeitwork) have been made allowing
us to refresh in the future

@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch from e33cc96 to 8d37061 Compare February 14, 2020 14:08
@Obsiye Obsiye added UAT and removed ready for review Please review labels Feb 14, 2020
@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch 5 times, most recently from 0158558 to 8aeeb09 Compare February 21, 2020 14:03
@Obsiye
Upgrade rails to version 6.0.2 and update sprockets
6be2c8f 
Update Rails to version 6.0.2

Downgrage sprockets to 3.7.2. This fixes a segmentation error in circle ci after upgrading to rails 6
this is when the docker file runs assets:precompile
sass/sassc-rails#122
@Obsiye
Update layouts helpers rendering of files
a6e7de0 
Update render file to render template. This fixes a deprecation as shown > DEPRECATION WARNING: render file: should be given the absolute path to a file
@Obsiye
Update Module#parent to #module_parent due to deprecation
55dfc44 
Update self.class.parent to self.class.module_parent to fix errors
DEPRECATION WARNING: `Module#parent` has been renamed to `module_parent`. `parent` is deprecated and will be removed in Rails 6.1
@Obsiye
Update tests to use #media_type instead of #content_type
ae28c35 
Update tests to use #mediate_type instead of #content_type to remove deprection after upgrading to rails 6 as show :
DEPRECATION WARNING: Rails 6.1 will return Content-Type header without modification. If you want just the MIME type, please use `#media_type` instead.
@Obsiye
Update form builder test to use action controller view context
19cecd2 
This is to remove the warning below after updating to rails 6

DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context,
assignments, and a controller.
@Obsiye
Update to use MailDeliveryJob instead of DeliveryJob after rails 6 up…
fcd2aa8 
…grade

Update to use MailDeliveryJob instead of DeliveryJob after rails 6 upgrade

Use new arguments format for ActionMailer::MailDeliveryJob because the changes to ActionMailer's arguments to args: @Args affects the email monitor in this app. I updated to fix this.

https://github.com/rails/rails/blob/master/actionmailer/lib/action_mailer/message_delivery.rb

Also, update tests with new MailDeliveryJob arguments format
@Obsiye
Update config/application.rb after rails 6 upgrade
4e2858b 
Set up new config defaults for rails 6 https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults

Set config.action_mailer.delivery_job = "ActionMailer::MailDeliveryJob"

Also, zeitwork is causing errors so use :classic
@Obsiye
Set delivery job to use GovukNotifyMailerJob to allow email monitoring
adfcd21
@Obsiye
Remove mailer tests in the request tests
25237db 
This is because in rails 6 the test queue adapter is overriding sidekiq in request tests. The ResendLinkRequestMailer and FeedbackMailer is already tested seperately in mailer tests.

Tried to use alternative solutions such as disabling the queue adapter but that would cause errors elsewhere e.g. in a email monitor service test. ((ActiveJob::Base.descendants << ActiveJob::Base).each { |a| a.disable_test_adapter })
@Obsiye
Add type :request to request tests
e890679
@Obsiye
Update tests and cassettes
40e1e87
@Obsiye
Update gemfile and mailers
469aed2
@Obsiye
Regress sidekiq version to 5.2.7
c79e19d 
This is because sidekiq version 6 requires zeitwerk as the autoloader but we are using classic due to
an error it give with expanding frozen paths in the rails helper
@Obsiye Obsiye force-pushed the ap-950-upgrade-rails branch from 8aeeb09 to c79e19d Compare February 21, 2020 14:04
@Obsiye Obsiye merged commit fcb2d5b into master Feb 21, 2020
@Obsiye Obsiye deleted the ap-950-upgrade-rails branch February 21, 2020 14:31
@Obsiye Obsiye mentioned this pull request Mar 2, 2020
Closed
6 tasks
@Obsiye Obsiye added approved Approved by code reviewers and removed UAT labels May 27, 2020
This was referenced May 27, 2020
Closed
Closed
Closed
@Obsiye Obsiye mentioned this pull request Nov 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenrichards stephenrichards stephenrichards approved these changes

@colinbruce colinbruce colinbruce approved these changes

Assignees
No one assigned
Labels
approved Approved by code reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Obsiye @stephenrichards @colinbruce