Fixed InventoryCollection Vm and Template

[slemrmartin]

Default values name and location were missing

Issue: #17396

• (dependent) Persister: InventoryCollection building through add_collection() manageiq-providers-vmware#288

[slemrmartin]

Cc @agrare

Weird that it doesn't cause problems in all previous tests except vmware.

Btw I finally figured out what means "builder_params" :) Can I rename method add_builder_params to add_default_values in some next PR?

[slemrmartin]

@miq-bot add_label bug

[agrare]

Btw I finally figured out what means "builder_params" :) Can I rename method add_builder_params to add_default_values in some next PR?

Completely agree, default values is a much better name

[agrare]

@slemrmartin why do we need default values for name and location? IMO the parser should not leave these blank.

[slemrmartin]

@agrare because you specify them to definitions in InventoryCollectionDefault and vmware specs fail without it

[agrare]

@slemrmartin hm okay let me check the vmware specs because I don't think this should be needed. I'd rather it fail to save if vendor or name isn't passed because that is a case that shouldn't happen.

[agrare]

Hey @slemrmartin I pushed a fix for this agrare/manageiq-providers-vmware@cbcea7e
Can you cherry-pick that into your branch for the VMware PR if it looks good to you?

[slemrmartin]

@agrare it looks good, cherry-picked.
I'm not sure about vendor, from where I refactor it, probably from amazon/azure.
It's not in InventoryCollectionDefault in core, all amazon/azure and vmware specs seems ok without it.

[agrare]

👍 LGTM

[agrare]

@slemrmartin yeah vendor can't be in the base defaults because it is provider dependent, here is where the list of vendors for vm_or_template is defined https://github.com/ManageIQ/manageiq/blob/master/app/models/vm_or_template.rb#L40-L53

I'd be fine if that was just in the parser since it just needs to be one or two places in the parser.

[slemrmartin]

I know, I derived it from persister
https://github.com/ManageIQ/manageiq/blob/master/app/models/manager_refresh/inventory_collection/builder/shared.rb#L34
It fits it except case sensitivity and xen

[miq-bot]

Checked commits slemrmartin/manageiq@b08991d~...74fa4dc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

[agrare]

@slemrmartin in that case I'm not sure what you weren't sure about up here ☝️ #17602 (comment) haha

[slemrmartin]

I'm sure about value, not sure about from where I get it for refactoring :)) but probably I moved provider-specific definitions of vendor from amazon and azure to core..but nevermind :)