Change ConversionHost resource_type validation

[djberg96]

This is something of a followup to #18277 where I think I got confused about how resource_type should be validated, thinking that STI would automatically kick in. But no, that's not how it works. Instead, since we're dealing with strings, it will fail in practice because the resource_type will be e.g. 'ManageIQ::Providers::Openstack::CloudManager::Vm' rather than just 'Vm'.

This PR removes hard coded string checks and instead relies on the individual providers to tell us if the resource supports conversion hosts or not. With both ManageIQ/manageiq-providers-ovirt#315 and ManageIQ/manageiq-providers-openstack#407 now merged, which implemented supports :conversion_host using the SupportsFeature mixin, we can now use this in practice for validations intead.

https://bugzilla.redhat.com/show_bug.cgi?id=1622728
https://bugzilla.redhat.com/show_bug.cgi?id=1695797

[ghost]

👍

[jameswnl]

how about creating the following vm factory and so that you don't need to skip_validate?

factory(:vm_support_conversion, :parent => :vm) do
    after(:build) do |vm|
      vm.stub(:supports_conversion_host?).and_return true
    end
  end

and similarly to the host?

[djberg96]

@jameswnl Seems using rspec-mocks inside factories no longer works. Take a look at the comments for spec/factories/dialog.rb, for example.

[jameswnl]

yeah, so this works

factory(:vm_support_conversion, :parent => :vm) do
    after(:build) do |obj|
      def obj.supports_conversion_host?; true; end
    end
end

[djberg96]

I think this is frowned upon, though. The only other factories that do this are tagged with TODO comments to fix them.

[jameswnl]

ok, we can do a simple re-arrangement. After moving up the factory lines outside of context, use allow in before do as in here

[agrare]

How about using a host_redhat factory? That should already supports_conversion_host?

[djberg96]

I did that originally, but @jameswnl complained since it's supposed to be provider independent methods.

[agrare]

I see vm_openstack above and host_redhat below so while I completely agree with that we don't need to solve that problem right here.

We should probably define a vm_supports_conversion_host factory in the future to handle this.

[jameswnl]

ok, we can do a simple re-arrangement. After moving up the factory lines outside of context, use allow in before do as in here

@agrare no need for the factory, just mock the method would already do

[djberg96]

Too late, I already switched them back. :)

Note that I also had to add an ems since the redhat provider demands a minimum api_version.

[agrare]

It would be really cool if we could delegate :supports_conversion_host?, :to => :resource then just validate supports_conversion_host? but it looks like supports_feature_mixin can't do this type of validation yet....

[djberg96]

Reasonable idea. Let's investigate this in a separate, refactoring PR.

[djberg96]

I noticed that Rails 5.1 has delegate_missing_to, so perhaps we can point that to resource once we're on 5.1+.

[agrare]

👍 LGTM

[djberg96]

@miq-bot add_label hammer/yes

[simaishi]

Hammer backport details:

$ git log -1
commit 376bc43b6badf0006f13b74b7cd1f0c6af17ee65
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Feb 11 10:43:25 2019 -0500

    Merge pull request #18434 from djberg96/conversion_host_resource_type_validation
    
    Change ConversionHost resource_type validation
    
    (cherry picked from commit 6ecc88399feb70a837684dd047aea08e33b48709)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696437