Upgrade to Rails 7 #3
Conversation
From Rails guides: https://guides.rubyonrails.org/configuring.html#versioned-default-values config.load_defaults loads default configuration values for a target version and all versions prior. With this commit, config.load_defaults 7.1 will load defaults for all versions up to and including version 7.1.
With Rails 7.1.2, configuring `fixture_path` has been deprecated in favor of using an aray of `fixture_paths`.
This was deprecated in 7.0 and removed in 7.1. This config no longer has effects since multiple databases config in Rails has to be configured manually. It is now set to false by default.
since rails/rails#42445, this method has been changed from cattr_accessor to attr_accessor in ActiveRecord.
otherwise, we can't upgrade this gem on the main app ;)
These changes allow us to support Rails 7.1, so we can remove the second constraint completely. Otherwise, when we upgrade it to > 7.1, we will need to update the version again.
| @@ -7,7 +7,7 @@ class ImpressionistGenerator < Rails::Generators::Base | |||
| # FIX, why is this implementing rails behaviour? | |||
| def self.next_migration_number(dirname) | |||
| sleep 1 | |||
| if ActiveRecord::Base.timestamped_migrations | |||
| if ActiveRecord.timestamped_migrations | |||
There was a problem hiding this comment.
I'm not 100% if this change is supported in Rails 6.1? Either way I don't think we use this generator, but in case its important:
> ActiveRecord.timestamped_migrations
(irb):1:in `<main>': undefined method `timestamped_migrations' for ActiveRecord:Module (NoMethodError)
> ActiveRecord::Base.timestamped_migrations
=> true
There was a problem hiding this comment.
Yeah, considering that we only use it for the landing pages and not for anything else, I wouldn't worry about the generators. This gem is probably gonna be removed right after the upgrade work is done anyway, so I am gonna leave as it is 👍 thank you for raising that, though 💯
We have this in the main app so this file is probably used only when the generator is run for the first time:
| @@ -23,7 +23,7 @@ | |||
|
|
|||
| # See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration | |||
| RSpec.configure do |config| | |||
| config.fixture_path = "spec/fixtures" | |||
| config.fixture_paths = ["spec/fixtures"] | |||
There was a problem hiding this comment.
Similarly, I believe this may only be supported in newer versions of Rails
> require "rspec/rails"
=> true
> RSpec.configure { |c| c.fixture_paths = ["spec/fixtures"] }
NoMethodError: undefined method `fixture_paths=' for #<RSpec::Core::Configuration:0x00007f9cfc9968d0
Did you mean? fixture_path=
fixture_path
fixture_path?
from (pry):8:in `block in __pry__'
> RSpec.configure { |c| c.fixture_path = "spec/fixtures" }
=> "spec/fixtures"
There was a problem hiding this comment.
Yeah, good call! If this was being used for other things and outside of convertkit, I would take more care of these cases but going with the comment above, let's go with this for now.
There was a problem hiding this comment.
Good to go, for what its worth, I imagine these changes are more a blocker for Rails 7.1 vs Rails 7.0 correct? If so, may want to prioritize changing the git SHA ahead of the Rails 7.1 upgrade instead of the Rails 7.0? Although perhaps this helps to silence some warnings? Either way looks good, thank you!
|
Thanks, @keegnotrub !
I've read this a couple of times and still can't follow, sorry 🙈 could you describe it a bit more, please? |
|
Apologies, just wondering if we'd necessarily need this updated in order to get to Rails 7.0 (as opposed to Rails 7.1) for the main ckapp repo. |
|
Oh, I see what you mean, now, thanks! It was more to have one less thing to be done later than anything else. Also, I tested the changes in staging and the visitors are working, which is the only thing, AFAIK, that we rely on. So if it works, great, right?! 😆 |
This is one of the dependencies we need to upgrade to Rails 7 first before upgrading the main CK's app.
The major changes are:
active_record.legacy_connection_handling. This was deprecated in 7.0 and removed in 7.1. This config no longer has effects since multiple databases config in Rails has to be configured manually. It is now set to false by default on new rails apps.fixture_pathhas been renamed tofixture_pathsbecause it's an array that accepts multiples paths.The other file changes are made so the gem is running on Rails 7.1.2.