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

rails 5.2 upgrade #498

Merged
merged 20 commits into from Jun 14, 2018

Conversation

Projects
None yet
2 participants
@thomasdziedzic-pd
Contributor

thomasdziedzic-pd commented May 28, 2018

I wanted to see what it takes to upgrade a rails application and I used lobsters to learn!

Note that there is probably a migration that needs to happen after; see config/initializers/new_framework_defaults_5_2.rb

@pushcx

Hi, thanks for digging in to this! It looks like it's a pretty small update and I really appreciate you volunteering to take it on.

I had a couple questions and it looks like there's some unrelated changes and additions that we don't need, or at least need to discuss in other issues/PRs. Otherwise I look forward to merging this soon.

If any of my questions are off-base, maybe grab me on chat to talk through; I've only skimmed the upgrade guide.

.gitignore Outdated
@@ -40,3 +40,6 @@ config/database.yml
config/initializers/secret_token.rb
config/*.sphinx.conf
config/unicorn.conf.rb

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

This personal config should go into your ~/.gitconfig, add

[core]
  excludesfile = /home/whatever/.gitignore

and list this there.

Gemfile Outdated
@@ -23,6 +23,9 @@ gem "actionpack-page_caching"
gem "exception_notification"
gem "unicorn"
# Reduces boot times through caching; required in config/boot.rb
gem 'bootsnap', '>= 1.1.0', require: false

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

Please break this out into a separate PR for discussion if it's not required for Rails 5.2.

Gemfile Outdated
@@ -47,4 +50,5 @@ group :test, :development do
gem "rubocop", require: false
gem "sqlite3"
gem "faker"
gem 'listen'

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

Please break this out into a separate PR for discussion if it's not required for Rails 5.2.

@@ -1,3 +1,3 @@
#!/usr/bin/env ruby
ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../../Gemfile', __FILE__)
ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__)

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

Does this throw a warning in Rails 5.2 or something? I can't see why you've changed this in a few places.

This comment has been minimized.

@thomasdziedzic-pd

thomasdziedzic-pd Jun 13, 2018

Contributor

These are rails generated files. When you run rails app:update as suggested in the upgrade guide, the update task automatically updates this to the latest code for these tools. I would suggest we keep it if possible since this will occur on every rails app:update run which might include further changes.

This comment has been minimized.

@pushcx

pushcx Jun 14, 2018

Member

Ah, I didn't notice the bin/ path. Good call.

begin
exec "yarnpkg #{ARGV.join(" ")}"
exec "yarnpkg", *ARGV

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

This looks like a personal style thing and not worth the churn. Please remove it.

This comment has been minimized.

@thomasdziedzic-pd

thomasdziedzic-pd Jun 13, 2018

Contributor

ditto with rails app:update comment

@@ -1,4 +1,4 @@
Lobsters::Application.configure do
Rails.application.configure do

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

Merging the updates to these config files must not have been fun, so let me highlight that I appreciate you doing them. :)

This comment has been minimized.

@thomasdziedzic-pd

thomasdziedzic-pd Jun 13, 2018

Contributor

I appreciate that you noticed the amount of work it took :)

This comment has been minimized.

@pushcx

pushcx Jun 14, 2018

Member

I, too, have shaved this yak.

# Add additional assets to the asset load path.
# Rails.application.config.assets.paths << Emoji.images_path
# Add Yarn node_modules folder to the asset load path.
Rails.application.config.assets.paths << Rails.root.join('node_modules')

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

We don't have node modules, so we don't need this. If it's a new default for this file, please just comment it out.

@@ -0,0 +1,26 @@
# Be sure to restart your server when you modify this file.

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

I'm looking forward to this in #486. 😄

@@ -0,0 +1,35 @@
# Be sure to restart your server when you modify this file.

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

If we can't flip any of these now, let's make an issue for each.

This comment has been minimized.

@thomasdziedzic-pd

thomasdziedzic-pd Jun 13, 2018

Contributor

Created a ticket with a checklist: #509

end
# Store uploaded files on the local file system (see config/storage.yml for options)
config.active_storage.service = :local

This comment has been minimized.

@pushcx

pushcx Jun 13, 2018

Member

Can we entirely remove ActiveStorage's config and not load it at all? I'd rather remove unused Rails components for speed + memory, and "saves files to the local filesystem" also makes me wary of security concerns. For example, disabling DEFAULT_PARSERS has saved us exposure to security bugs.

ActionCable, too, if we haven't pulled that out yet.

This comment has been minimized.

@thomasdziedzic-pd

thomasdziedzic-pd Jun 13, 2018

Contributor

Removed storage & cable. Unfortunately there is no clean way of doing this other than inlining the include file and deleting the individual lines.

@thomasdziedzic-pd thomasdziedzic-pd force-pushed the thomasdziedzic-pd:rails-5.2 branch from b918b9b to 60e57bd Jun 13, 2018

thomasdziedzic-pd added some commits Jun 13, 2018

@thomasdziedzic-pd thomasdziedzic-pd force-pushed the thomasdziedzic-pd:rails-5.2 branch from 07ed700 to 4d2b9ec Jun 13, 2018

thomasdziedzic-pd added some commits Jun 13, 2018

thomasdziedzic-pd added some commits Jun 13, 2018

@thomasdziedzic-pd

This comment has been minimized.

Contributor

thomasdziedzic-pd commented Jun 13, 2018

I believe that I have addressed all of the things mentioned or have created tickets for the things that still needed to be done after the merge. Waiting for your feedback.

@pushcx pushcx referenced this pull request Jun 14, 2018

Closed

Turn on rails 5.2 defaults after upgrade #509

6 of 6 tasks complete
@pushcx

This comment has been minimized.

Member

pushcx commented Jun 14, 2018

My feedback is that this is rock solid and I really appreciate you doing it, thank you. Merging.

@pushcx pushcx merged commit 94d9549 into lobsters:master Jun 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@thomasdziedzic-pd thomasdziedzic-pd deleted the thomasdziedzic-pd:rails-5.2 branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment