Use leaf classes for ems type validation

[d-m-u]

Recent changes put the automation thingy factory back to not being a leaf class and LJ wouldn't leaf me alone so.

From comment here: https://github.com/ManageIQ/manageiq/pull/19330/files#r329235409

@miq-bot assign @jrafanie
I think this's all, right?

[jameswnl]

Don't do this. Change to use factory :automation_manager_ansible_tower instead of :automation_manager

[miq-bot]

Checked commit d-m-u@8e61b03 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[jrafanie]

Don't do this. Change to use factory :automation_manager_ansible_tower instead of :automation_manager

We can't. The automation_manager factory is unusable right now.

[jameswnl]

Don't do this. Change to use factory :automation_manager_ansible_tower instead of :automation_manager

We can't. The automation_manager factory is unusable right now.

This is passing: ManageIQ/manageiq-content#586

[d-m-u]

@jameswnl LJ's spec on https://github.com/ManageIQ/manageiq/pull/19330/files#r329235208 shows that the factory is currently unusable. Even if the content repo greens because we change those specs, the factories in this file need to be leaf classes. All of them. Argue with LJ about it all you want.

[kbrock]

This fixes an issue introduced in #19330

[jrafanie]

@jameswnl I'll merge this for now since ems factories can't be abstract/non-leaf classes, see:

manageiq/app/models/ext_management_system.rb

Line 852 in c518bf5

errors.add(:base, "emstype #{self.class.name} is not supported for create") unless ExtManagementSystem.supported_types_and_descriptions_hash.key?(emstype)

We can discuss other solutions for the original problem of preventing creation of these non-leaf classes that was implemented in #18842. Ultimately, in that PR, we decided it was better to have all the base factories use a concrete leaf ems class from a provider to allow non-provider tests to be able to use the base factory in situations that are provider agnostic. In other words, it's better to have provider class information in one location in the base factories than to force tests in several repos to know this class information in their tests. Of course, if the non-provider tests are testing provider specific use cases, then they should be using the specific provider, so I think your change in ManageIQ/manageiq-content#586 was good.

It's a gnarly problem and any other solutions would be greatly appreciated.

cc @agrare

[jameswnl]

We can discuss other solutions for the original problem of preventing creation of these non-leaf classes that was implemented in #18842. Ultimately, in that PR, we decided it was better to have all the base factories use a concrete leaf ems class from a provider to allow non-provider tests to be able to use the base factory in situations that are provider agnostic. In other words, it's better to have provider class information in one location in the base factories than to force tests in several repos to know this class information in their tests. Of course, if the non-provider tests are testing provider specific use cases, then they should be using the specific provider, so I think your change in ManageIQ/manageiq-content#586 was good.

This approach is problematic because it allows creating tests that seems general (i.e. applicable to all implementations of that provider), but in fact it depends on implementation-specific features.

This approach also creates barrier to make our providers more pluggable.

I still believe ManageIQ/manageiq-content#586 is the way to go.

Yeah, we can discuss more on "ems factories can't be abstract/non-leaf classes"

[jrafanie]

This approach is problematic because it allows creating tests that seems general (i.e. applicable to all implementations of that provider), but in fact it depends on implementation-specific features.

Yes, these tests need to depend on the specific subclass factory like I mentioned was good about ManageIQ/manageiq-content#586

This approach also creates barrier to make our providers more pluggable.

Sure, if there are better solutions to prevent non-leaf ems classes from being created, I think we're open to something better. I think #18842 was the solution we disliked least.

I still believe ManageIQ/manageiq-content#586 is the way to go.

👍 It's ansible tower specific tests so it should use ansible tower specific factories.

This PR was merely to make the base factory usable again. It's not the complete solution.