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

Upgrade rails to 6.0.3.2 #812

Merged
merged 7 commits into from
Sep 2, 2020
Merged

Upgrade rails to 6.0.3.2 #812

merged 7 commits into from
Sep 2, 2020

Conversation

benbaumann95
Copy link
Contributor

No description provided.

cdccollins
cdccollins reviewed Jul 10, 2020
View reviewed changes
config/environments/development.rb Outdated
# config.file_watcher = ActiveSupport::EventedFileUpdateChecker

# Using polling file checker, requires VM time to be in sync with host
config.file_watcher = ActiveSupport::FileUpdateChecker

config.hosts << "content-data-admin.dev.gov.uk"
Copy link

Choose a reason for hiding this comment

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

I think we've decided the best way to do this is

config.hosts += [ 
  "content-data-admin.dev.gov.uk"
]

Also, is there a draft version of this app that needs to be added here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, i'll update to that 👍 .
Doesn't appear to be a draft version

@benbaumann95 benbaumann95 force-pushed the upgrade-rails branch 2 times, most recently from e414a14 to 093c8b9 Compare July 13, 2020 10:12
cdccollins
cdccollins reviewed Jul 13, 2020
View reviewed changes
config/environments/production.rb Outdated
# config.active_job.queue_name_prefix = "content-data-admin_#{Rails.env}"
# config.active_job.queue_name_prefix = "content_data_admin_production"

config.action_mailer.perform_caching = false
Copy link

Choose a reason for hiding this comment

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

I think we're trying to avoid changing configuration in production.rb, just checking that you'd seen this and intended it 😄

@thomasleese thomasleese mentioned this pull request Jul 22, 2020
Closed
cdccollins
cdccollins reviewed Aug 11, 2020
View reviewed changes
app/controllers/concerns/file_storage.rb Outdated
filename = "#{basename}.csv"
key = "#{timestamp}/#{filename}"

# rubocop:disable Rails/SaveBang
Copy link

Choose a reason for hiding this comment

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

This might need revisiting

cdccollins
cdccollins reviewed Aug 11, 2020
View reviewed changes
spec/features/index_page/export_csv_spec.rb Outdated
@@ -62,7 +62,9 @@
aws_secret_access_key: ENV["AWS_SECRET_ACCESS_KEY"],
)
connection.directories.each(&:destroy)
# rubocop:disable Rails/SaveBang
Copy link

Choose a reason for hiding this comment

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

👀

@benbaumann95 benbaumann95 marked this pull request as ready for review August 12, 2020 13:27
cdccollins
cdccollins suggested changes Aug 17, 2020
View reviewed changes
Copy link

@cdccollins cdccollins left a comment

Choose a reason for hiding this comment

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

I think rather than deleting the configure_security_policy file we're adding config to it, as Andrew does here https://github.com/alphagov/travel-advice-publisher/blob/2bd66cfeed97cba949612bc19456bf21bcbcbd67/config/initializers/csp.rb

Also, can you remove bin/bundle and bin/update to make it more in line with Rails 6 config. For some reason rails app:update doesn't do it for us :(

@benbaumann95
Remove constraints from dependencies
5a34a9e 
Use the Gemfile.lock to manage the versions instead which should make
it easier to upgrade as it allows bundler a wider set of versions to
resolve the dependencies.

Keep the rails constraint so that the current version can be identified.
@benbaumann95
Update Rails to 6.0.3.2 and dependent gems
0d48d40
@EGiataganas
Copy link

@benbaumann95 Can you please enable the js_compressor option for Sprockets in config/environments/production.rb

  # Compress JS using a preprocessor.
  config.assets.js_compressor = :uglifier

@benbaumann95
Run bundle exec rails app:update
3bb3fdd 
- Adds additional changes that come with Rails 6, including updated comments and minor config changes.
- Uses config.load_defaults 6.0

Add zeitwork.rb to config/initializers to define CSV acronym in certain classes
- https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#customizing-inflections
- There is some inconsistency with uppercase "CSV" and lowercase "csv" in classnames within the app so these uppercase relevant classnames have been added to the zeitwork.rb file.
- As a result of introducing config.load_defaults 6.0 in application.rb, using Concerns::ClassName etc has been modified to just ClassName and now we include just the ClassName

Remove 'layout: true' from render file
- This was raising an ArgumentError with the rails upgrade

Remove bin/update in line with rails 6 changes
@benbaumann95
Add content-data-admin to config.hosts
9b06925
@benbaumann95
Pin rails to specific version and make it stand out
13ca71b 
See alphagov/service-manual-frontend@27390cd for reasoning.
@benbaumann95
Update remaining minor dependencies
6d6f23d 
Inline with the suggestion in alphagov/govuk-rfcs#126 update the
remaining minor dependencies when doing a framework upgrade.
@benbaumann95
Add default GOV.UK Content Security Policy header
aae2202
@benbaumann95 benbaumann95 merged commit f608c2b into master Sep 2, 2020
@benbaumann95 benbaumann95 deleted the upgrade-rails branch September 2, 2020 09:08
kevindew added a commit that referenced this pull request Jan 24, 2023
@kevindew
Comment out GovukContentSecurityPolicy
9ae1159 
GOV.UK hadn't intended for this app to have the GOV.UK Content Security
Policy yet, with us first planning to roll out this to frontend app. It
looks like this was added as part of an outsourced Rails update [1],
where the dev couldn't have known about our nuanced context.

As this is an app that doesn't receive a lot of developer attention I'm
disabling this as I don't want breaking changes to the CSP [2] to end up
in this app.

[1]: #812
[2]: alphagov/govuk_app_config#279
kevindew added a commit that referenced this pull request Jan 24, 2023
@kevindew
Comment out GovukContentSecurityPolicy
3767926 
GOV.UK hadn't intended for this app to have the GOV.UK Content Security
Policy yet, with us first planning to roll out this to frontend app. It
looks like this was added as part of an outsourced Rails update [1],
where the dev couldn't have known about our nuanced context.

As this is an app that doesn't receive a lot of developer attention I'm
disabling this as I don't want breaking changes to the CSP [2] to end up
in this app.

[1]: #812
[2]: alphagov/govuk_app_config#279
@kevindew kevindew mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdccollins cdccollins cdccollins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benbaumann95 @EGiataganas @cdccollins