Remove unused conditional from class_for_platform in MiqProvisionWorkflow #19751
Conversation
djberg96
changed the title
|
@miq-bot add_label refactoring |
| # example, the argument "openstack" would become: | ||
| # | ||
| # "ManageIQ::Providers::Openstack::CloudManager::ProvisionWorkflow" | ||
| # | ||
| def self.class_for_platform(platform) |
There was a problem hiding this comment.
@Fryguy Are we ready to drop support for the old naming scheme?
There was a problem hiding this comment.
@agrare Do you know of any providers that still use these old constant names? (OpenstackInfra)
There was a problem hiding this comment.
I'm not really sure... at a minimum @djberg96 can you do cross-repo tests on this to ensure the constant is not being used anywhere?
There was a problem hiding this comment.
There was a problem hiding this comment.
We should probably log a deprecation warning for a release first. What do you think?
There was a problem hiding this comment.
@bdunne I guess I'm not following how you want it to look then.
There was a problem hiding this comment.
if classy =~ /(.*)Infra/
Vmdb::Deprecation.deprecation_warning("MiqProvisionWorkflow.class_for_platform with platform #{platform}")
find_matching_constant("MiqProvision#{classy}Workflow") ||
...
would give you
DEPRECATION WARNING: MiqProvisionWorkflow.class_for_platform with platform OpenstackInfra is deprecated and will be removed from ManageIQ L-release (called from irb_binding at (irb):4)
There was a problem hiding this comment.
I guess I don't see the benefit of that. The cross-repo tests passed, and nothing uses the old naming scheme any more, right?
There was a problem hiding this comment.
I guess I don't see the benefit of that. The cross-repo tests passed, and nothing uses the old naming scheme any more, right?
Yes, the code paths that have test coverage aren't using it.
@gmcculloug What's your comfort level with this?
There was a problem hiding this comment.
@bdunne Made the change locally and tried a few paths, all looked good. 👍
|
@bdunne, @gmcculloug Are you ok with this? |
|
Checked commits https://github.com/djberg96/manageiq/compare/0244c00ae610a43954e74058416e97abce175041~...fe1ad03741f2f7be4461c02a05e10fd255c49233 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint |
This PR removes what appears to be a legacy conditional that will never actually be triggered. I also added some comments.
Followup to #19713