Perform instance name validation for google provider

[selmanj]

• Modify validation code to be able to use multiple validation methods
• Modify regex validation to produce detailed error message on failure
• Modify instance name dialog for Google Provider to validate according to regex

Fixes downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1330982

/cc @agrare @blomquisg @erjohnso

[selmanj]

I see @miq-bot complaining about the block nesting - I can tackle this if you guys want but it looks like a non-trivial amount of work. I acknowledge that I made the situation worse by adding an additional block nesting :(.

Also, I have no idea how to interpret that coveralls number - I tried to add tests covering the changes I made (except for the google yaml file, which I'm unsure how to test or how to assert what I want).

[agrare]

So you had to do this because nothing else has two validation methods? @gmcculloug would this be helpful for other dialogs as well?
Alternatively could you have a single gce specific :validate_vm_name that runs the current checks plus the regex check?

[selmanj]

A second check for a gce-specific validate_vm_name would work too. I do tend to favor this approach though, as it follows the more declarative style of putting the field validations and their restrictions in miq_provision_google_dialogs_template.yaml.

However, if I truly believed that, I'd change :validate_vm_name to the more direct :validate_length as that's much more declarative. Another downside is that it might be surprising for fld[:required_method] to be optionally enumerable. I could go either way on this - what do you think?

[selmanj]

Changed kind_of? Enumerable to instead just wrap the field with Arrays#wrap

[gmcculloug]

@agrare Yes, I imagine this could be useful other places. However, there is a goal to rewrite these dialogs using the Service Dialog modeling or something similar so not much effort is going into advancing them at this point.

[agrare]

Thanks @gmcculloug !

[agrare]

Up to you but Array.wrap(nil) is just [] so we could probably take out the .nil? check above (and take out another level of indentation 😄)

[selmanj]

I was tempted to change that as well, but unfortunately the behavior is different for nil; specifically it falls back to a default required method for validation, if defined (or it complains loudly if not defined).

Only vmware uses this:

jsselman@dev:~/manageiq (perform_instance_name_validation u= origin/perform_instance_name_validation)$ ag default_require app/
app/models/miq_request_workflow.rb
206:            default_require_method = "default_require_#{f}".to_sym
207:            if self.respond_to?(default_require_method)
208:              fld[:error] = send(default_require_method, f, values, dlg, fld, value)

app/models/miq_provision_virt_workflow/dialog_field_validation.rb
3:  def default_require_sysprep_enabled(_field, _values, dlg, fld, value)
9:  def default_require_sysprep_custom_spec(_field, _values, dlg, fld, value)
jsselman@dev:~/manageiq (perform_instance_name_validation u= origin/perform_instance_name_validation)$ ag sysprep_enabled product/
product/dialogs/miq_dialogs/miq_provision_dialogs.yaml
309:        :sysprep_enabled:

product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_clone_to_template.yaml
309:        :sysprep_enabled:

product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_clone_to_vm.yaml
318:        :sysprep_enabled:

product/dialogs/miq_dialogs/miq_provision_vmware_dialogs_template.yaml
309:        :sysprep_enabled:
jsselman@dev:~/manageiq (perform_instance_name_validation u= origin/perform_instance_name_validation)$

Alternatively I could remove the default logic and make the vmware fields explicitly define their validation method...

[agrare]

Ah okay, probably best to leave that as is in this PR then

[miq-bot]

Checked commits selmanj/manageiq@38da103~...3ebc6cd with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🍪

[gmcculloug]

@selmanj @agrare The only thing to note here is that users could specific an enumeration pattern ($n{#}) as part of the VM name and this would prevent that. I do not believe it is a common use-case and users could update the dialog if it is required to support it.

Here's an example where automate applies it based on provisioning more than one VM: db/fixtures/ae_datastore/ManageIQ/Cloud/VM/Provisioning/Naming.class/methods/vmname.rb#L53

[agrare]

@gmcculloug so the validation run before the names are expanded out?

[gmcculloug]

Correct. The VM name entered in the dialog is capture as-is at the request level, it is not until the request_task is created that the name is fully resolved.

[agrare]

@selmanj want to break out a regex book and try to allow $n{0-9} as well?

[selmanj]

Wouldn't the better fix be to run the validation after the name has expanded? How big of a change would that be?

[selmanj]

I guess the obvious downside is that the regex then allows illegal characters that are only needed for automate; perhaps we are ok with this because its very unlikely that the customer will write $n{} in their hostname, but I'd prefer to avoid complicating the regex if needed.

[agrare]

If its not a common use case to enter it into that field then it should be fine to leave the regex as it is. I was worried we wouldn't be able to provision multiple instances and have the names work out but that seems to work fine.

[gmcculloug]

If you do the name validation after the name has been expanded it will processed on the task. In that case the validation should really happen right before we send it to the provider so it can check any changes to the vm_name made from automate. But the provider is already doing this validation for us at the task level and it seem odd to repeat it here.

I realized this morning there is a different approach we can take here. The :vm_name field in the dialog has a :notes section which can be used to display text below the field.

Here is an example using the :required_regex_fail_details from this PR:

This could relieve the need for any of the validation checks. Thoughts?

[agrare]

I think people aren't going to read that and the validation is still a good idea 😄

[selmanj]

Sounds good. Let me know if I should take any action on this PR.

[agrare]

Looks good @selmanj 👍

[gmcculloug]

👍 Thanks.