Backward compatible rails 7 changes #735
Conversation
The dummy application doesn't need assets so it's easier to remove the calls instead of adding a needless gem to the gemfile. Rails 7 doesn't have sprocket-rails by default so we'd need to drop these calls or add it to the Gemfile. See: https://github.com/rails/rails/issues #43793
|
cc @kbrock |
| @@ -46,18 +46,17 @@ def up | |||
| ServiceParametersSet.in_my_region.where(:ext_management_system => managers).update_all(:type => "#{provider_klass}::ServiceParametersSet") | |||
| end | |||
| end | |||
| end | |||
There was a problem hiding this comment.
oops! I can't believe 6.1 didn't fail previously with this
| has_many :host_switches, :class_name => "AddMissingEmsIdToSwitch::HostSwitch" | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
There is no reason to re-open the migration class to define stubs... this was actually causing issues in the tests in rails 7 because it couldn't find the has_many :host_switches, :class_name => "AddMissingEmsIdToSwitch::HostSwitch" class AddMissingEmsIdToSwitch::HostSwitch. It makes sense to move this to the migration class, even if the migration isn't using the stubs.
There was a problem hiding this comment.
The reason these are in the test only is because the migration itself doesn't use those classes. At "runtime" we don't want to have to create stubs we don't need, because it can only introduce potential problems.
That being said, the way it was doing it before by reopening the migration is also wrong. In other migrations where we need a "new" migration stub, we have a helper method called new_migration_stub. Granted, it only creates a basic stub class, but it's possible we could pass a block to it to allow further manipulation.
There was a problem hiding this comment.
$ git grep "new_migration_stub"
spec/migrations/20200221200855_add_ancestry_to_miq_ae_namespace_spec.rb:4: let(:ns_stub) { new_migration_stub(:miq_ae_namespaces) }
spec/migrations/20210519204247_move_miq_set_memberships_spec.rb:7: let(:set_stub) { new_migration_stub(:miq_sets) }
spec/migrations/20210519204247_move_miq_set_memberships_spec.rb:8: let(:item_stub) { new_migration_stub(:scan_items) }
spec/migrations/20210519204247_move_miq_set_memberships_spec.rb:9: let(:rp_stub) { new_migration_stub(:resource_pools) }
spec/migrations/20210824231354_add_timestamps_to_binary_blobs_spec.rb:4: let(:binary_blob_stub) { new_migration_stub(:binary_blobs) }
spec/support/migration_helper.rb:62: def new_migration_stub(table_name)
There was a problem hiding this comment.
yeah, we can possibly add the block case. I don't see the potential problem of defining a migration stub within the migration class if it's only used in the test. It feels wrong to be in the test itself unless it's a basic new_migration_stub.
There was a problem hiding this comment.
To be fair, our reuse of migration stubs through multiple tests are problematic under rails 7 so we may have to revisit this anyway.
There was a problem hiding this comment.
I'll drop this change from this PR. I don't yet have a solution I like.
There was a problem hiding this comment.
Oh wow - ok yeah - we'll have to discuss - sounds like we need to "reimplement" some of the stub handling for Rails 7 - what a pain.
| :userid => ApplicationRecord.configurations[Rails.env]["username"], | ||
| :userid => ApplicationRecord.configurations.configs_for(:env_name => Rails.env).first.configuration_hash["username"], |
There was a problem hiding this comment.
wow the new way is gross - wonder if we should move it into a spec helper method.
There was a problem hiding this comment.
I'll check to see if there's a less gross way to do this. Although, I believe we had changes like this for appliance console and other areas already.
There was a problem hiding this comment.
a little better but still the way rails wants it:
ApplicationRecord.configurations.configs_for(:env_name => Rails.env, :name => "primary").configuration_hash["username"]
|
|
||
| has_many :hosts, :through => :host_switches, :class_name => "AddMissingEmsIdToSwitch::Host" | ||
| has_many :host_switches, :class_name => "AddMissingEmsIdToSwitch::HostSwitch" | ||
| end |
There was a problem hiding this comment.
Why is any of this necessary? It's not used in the script itself.
There was a problem hiding this comment.
Oh I see - See my other comment.
|
Does this PR upgrade the dummy app to Rails 7? Asking because it's changing some of the files within the dummy app. |
DEPRECATION WARNING: [] is deprecated and will be removed from Rails 7.0 (Use configs_for)
jrafanie
force-pushed
the
backward_compatible_rails7_changes
branch
from
fad094b
to
1d31791
Compare
|
Checked commits jrafanie/manageiq-schema@8529571~...1d31791 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
No, I'm just dropping some sprockets related files that aren't needed in the dummy app for testing migrations... those sprockets files assume sprocket-rails is required by rails... which is not the case with rails 7 so dropping the needless files is one less thing to deal with for rails 7. |
I think they were deprecated in 6.1 and dropped on 7.0. I guess we never saw the deprecations in 6.1. |
Extracted from #715
These are backward compatible changes to fix failures in rails 7. Interestingly, detected the typo preventing the down migration from running and also the weird migration stub issue where the it's defined in the test itself.