-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
1181 upgrade rails 6 #1307
1181 upgrade rails 6 #1307
Conversation
…most changes from this task that either clobbered app-specific setup or were unecessary comments or formatting changes. Update Rails to 6.0.0, update all gems with outdated dependencies
Update rspec-rails to 4.0.0.beta3 (required for rails 6 controller tests; see rspec/rspec-rails#2086) Configure action_dispatch media_type return to deal with deprecation warning
…idation (fixes Rails 6 breakage)
# PG::ConnectionBad: | ||
# connection is closed | ||
# Likely due to some changed order of operations | ||
# ActiveRecord::Migration.maintain_test_schema! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That error comes up in the app currently -- you just have to re-run the command. I'm not sure why it's happening and agree that it probably shouldnt, but this isn't a Rails 6 specific issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting it consistently on my machine, and I don't get it on master. Maybe it's a bit of both, sprinkled with a little OS X funtimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha -- That's possible.
This issue looks related: rails/rails#37407
It might be an upstream problem. What's the consequences of leaving this commented for now? We just have to do RAILS_ENV=test bundle exec rails db:migrate
after running regular migrations?
Agreed that we should test this manually. We might want to wait until after the conference to actually push the upgrade out; we're doing a live Demo on Thursday afternoon. |
also -- looks like the only reason the build fails is because of a rubocop "useless assignment" thing related to thanks for tackling this! I appreciate preserving a lot of the existing config stuff we have built. I wish the Rails upgrader would be a bit more judicious about applying those changes :/ |
D'oh -- that's because it should have been |
I'm good with this, but I'd like the input from @seanmarcia and @mdworken first before we merge it. (Note the issue above with the |
Checklist:
Resolves #1181
Description
Start with
rails app:upgrade
. However, this inflicts many changes, most of which are unnecessary or clobber needed app functionality or setup. So I went through all changes and discarded such changes, confirming the app still runs and tests pass.I also discarded most formatting / text / commenting changes to keep the PR easier on the eyes. For a review of the changes that are caused by
rails app:upgrade
, reference #1204Upgrade rails and other gems that had broken dependencies or behaviors. This include rspec, which had to be upgraded to >=4 (currently in beta, but due to be released soon. See rspec/rspec-rails#2086)
I reviewed https://edgeguides.rubyonrails.org/6_0_release_notes.html for other breaking changes and search the app for any occurrences. Only relevant item (caught by a test) was dealing with
content_type
on attachments (seeorganziation.rb
)Labelled WIP because I think it would be wise to manually test various app setups and features in addition to relying on rspec tests, and I am not fluent enough in this app to do so with confidence.
Type of change
How Has This Been Tested?
Tests pass. Poked around app. Should poke around more.