Add automate engine support for array elements containing text values.

[gmcculloug]

Automate supports passing an array of objects on the URL to the engine. This PR modifies that logic to allow strings to be passed as well. This functionality supports the multi-select drop-down being added to the Service Dialogs in PR #10270.

Links

• Backend support for PR Add multiselect option to dropdowns #10270

Steps for Testing/QA

Test using the multi-select control from the service dialog wired to a custom button. Configure the button to call the System/Request/InspectMe method. Logging will show the array containing each selected key passed from the dialog field.

[gmcculloug]

@mkanoor @Fryguy Please review

cc @eclarizio @d-m-u

[mkanoor]

@gmcculloug @tinaafitz
We can use this format which is support by the engine to have an array of integers
"Array::my_values" => "integer::1,integer::2,integer::3"

this would get stored as my_values = [1,2,3]

This is already supported in https://github.com/ManageIQ/manageiq/blob/master/lib/miq_automation_engine/engine/miq_ae_object.rb#L524

But there is a bug in this line which only seems to honor vmdb objects
https://github.com/ManageIQ/manageiq/blob/master/lib/miq_automation_engine/engine/miq_ae_object.rb#L285
If we remove this line we can pass in integer, float, bool and all the basic types.

Another bug that exists is we cannot have spaces between values after the ,
e.g.
integer::1, integer::2, integer::3 won't work
but
integer::1,integer::2,integer::3 will work
We just need to add a trim after we get the elements one by one
https://github.com/ManageIQ/manageiq/blob/master/lib/miq_automation_engine/engine/miq_ae_object.rb#L283

[eclarizio]

I think I would just structure this slightly differently:

describe "#process_args_as_attributes" do
  let(:result) { @miq_obj.process_args_as_attributes("Array::my_values" => my_values) }

  context "with an array containing strings" do
    let(:my_values) { "abc,xyz,,1" }

    it "stores the values as an array of strings" do
      expect(result["my_values"]).to eq(%w(abc xyz 1))
    end
  end

  context "with an array containing strings and objects" do
    let(:my_values) { "abc,VmOrTemplate::#{@vm.id}" }

    it "stores the first value as a string" do
      expect(result["my_values"].first).to eq("abc")
    end

    it "stores the second value as an MiqAeMethodService::MiqAeServiceVmOrTemplate" do
      expect(result["my_values"].second).to be_kind_of(MiqAeMethodService::MiqAeServiceVmOrTemplate)
    end
  end
end

[syncrou]

@mkanoor - Regarding the bug referenced in

but there is a bug in this line which only seems to honor vmdb objects
https://github.com/ManageIQ/manageiq/blob/master/lib/miq_automation_engine/engine/miq_ae_object.rb#L285

Is there any known reason why that check took place?

[syncrou]

@mkanoor - @gmcculloug's implementation will allow for "fred::12" to get passed as datatype fred value 12 to process_args_as_attributes.

We had talked earlier of forcing process_args_as_attributes to raise an error if a datatype was invalid.

I prototyped some code to see how this would affect the the current specs.

CLASS_TYPES = %w( boolean TrueClass FalseClass time Time symbol Symbol integer Fixnum float Float array String password )

With the list of allowed datatypes as above

raise MiqAeException::InvalidClass unless CLASS_TYPES.include?(datatype)

This brought out other issues as there are sundry datatypes passed through process_args_as_attributes when the a new instance of the MiqAeEngine is instantiated. Many of these types are neither an exposed service model - or part of the basic types handled here: https://github.com/gmcculloug/manageiq/blob/b07a184a9c7cee349fac2725d93dc7afd6485a72/lib/miq_automation_engine/engine/miq_ae_object.rb#L527-L535

The original code would allow all of these invalid datatypes to pass through.

I'm wondering if @gmcculloug's implementation will allow for fewer legacy Automate code issues?

[mkanoor]

@syncrou Are we checking if datatype is specified before we check for validity and raise the error

[syncrou]

@gmcculloug - I ran into an issue with klass if strip is not called here.

when a string is passed in "integer::1, integer::3, integer::5" klass is set to " integer" and doesn't resolve correctly in convert_value_based_on_datatype https://github.com/gmcculloug/manageiq/blob/b07a184a9c7cee349fac2725d93dc7afd6485a72/lib/miq_automation_engine/engine/miq_ae_object.rb#L532

[mkanoor]

@syncrou we would have to trim the klass value before we use it.

[mkanoor]

@syncrou
is the ,, testing nil values? But wouldn't fred::12 raise the exception before it processes the nil value

[syncrou]

@mkanoor - I modified the test you commented on and added a new one to handle the empty values.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commits gmcculloug/manageiq@82711a5~...3d1954e with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 1 offense detected

spec/lib/miq_automation_engine/miq_ae_object_spec.rb

• ❕ - Line 101, Col 121 - Metrics/LineLength - Line is too long. [129/120]

[gmcculloug]

@mkanoor Please review

[simaishi]

@gmcculloug Is there a BZ for this (or any related PRs)? Can you please create one if it doesn't already exist?

[gmcculloug]

@simaishi BZ https://bugzilla.redhat.com/show_bug.cgi?id=1428552 opened to track this feature.

Also requires #10270 and ManageIQ/manageiq-ui-classic#114

[simaishi]

@gmcculloug lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb doesn't exist in Euwe. Please create an Euwe-specific PR (referencing this one) or suggest other PRs to backport.

[simaishi]

Backported to Euwe via #14394

[moolitayer]

@mkanoor @mkanoor @syncrou I suspect I'm seeing an error related to this on euwe:
https://bugzilla.redhat.com/show_bug.cgi?id=1437128

We might be using the automate wrong, but anyway this change exposed the error.
Also might be happening for vm_or_template.check_policy_prevent since those also aren't exposed in SM_LOOKUP