Add a verify_credentials_task method

[agrare]

This adds a method that takes the options returned by
ems.params_for_create and calls a common verify_credentials API call on
the provider.

This can be called by the API/UI without needing to know anything about
the specific provider in a common way.

TODO need to update the task with the failure and ensure a boolean is returned so we don't leak the credentials on the queue.

#18818

[miq-bot]

Checked commit agrare@27daabf with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/mixins/verify_credentials_mixin.rb

• ❗ - Line 6, Col 7 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

[Fryguy]

👍 I'm only holding off merging cause you said you wanted to manually test a few things. Let me know how that goes and than I can merge.

[agrare]

Oh I tested it with you @Fryguy I just wanted to make sure after we renamed the module that everything still worked.

[skateman]

@agrare I'm playing around with the method in the API, but I have no idea what to feed the zone argument with. My first idea was to retrieve it from the form itself, but at least for the oVirt schema there's no such thing.

[Fryguy]

@agrare Oh I thought you were talking about the move of validate_credentials and ensuring that still works. (Spoke with @agrare offline and he said this was all tested as well and works).

[Fryguy]

@skateman Do we not have zone as part of the form? I'm thinking that every EMS needs a zone, and it doesn't really belong to the DDF form as it's more of a general ManageIQ thing as opposed to a provider specific thing.

[skateman]

Well, I think we should ask @Hyperkid123 about it, but injecting data into the DDF seems like an antipattern for me...and we don't have any other way of displaying the Zone field than injecting data into the DDF 😕

[agrare]

We could add zone to the DDF form at the top level (like region for amazon) but it can't be passed in to verify_credentials_task like the rest of the args because it has to be part of the queue_options not the args. This isn't something that is different between the different providers though, it is the same for every provider.

[skateman]

@agrare I'm okay with explicitly pulling out the zone for the verification, especially if the field is the same in every form.

    def verify_credentials_resource(_type, _id, options = {})
      zone = options.delete('zone')
      ExtManagementSystem.verify_credentials_task(current_user, zone, options)
    end

This should be done even when we add the zone field to the DDF as you wanted first. So I'm totally okay with it.

[Hyperkid123]

@agrare From UI perspective. there is minimum of two sections for every provider form:

• general info (top level)
• credentials info.
  In UI zone is always in the "general" part of form. So it should be part of the top level of the schema IMO.

@skateman I do agree that injecting data into the schema is not exactly the cleanest way how to it, but I also believe that in this case we probably don't have any other options.

[skateman]

Okay, if you can guarantee me that the zone will be sent with the form, I can work with that. Just let's agree on the format, so I can adjust my method from above.

[skateman]

This is failing with an undefined method if called from ExtManagementSystem. My guess is that it's only applicable when an already existing provider is being verified, which is not very useful in our case. @Fryguy @agrare

[agrare]

@skateman this is expected to be called on the provider class not on the base class.

[skateman]

Thanks, this can be sorted out.

[skateman]

The zone argument here should be the name of the zone, right? I think it's not obvious, at least I was debugging this for half an hour 😕

[agrare]

Correct, sorry if its not obvious but that's the standard for all tasks and queue items.