Provide base graph refresh attributes for ::Snapshot

[miha-plesko]

With this commit we provide base function to be used when inventoring snapshots.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1571120

@miq-bot assign @Ladas
@miq-bot add_label enhancement,gaprindashvili/yes

Related PR: ManageIQ/manageiq-providers-vmware#217

[miha-plesko]

Ignoring rubocop comment because we're following pattern used many times in this same file. Sorry dude for offending you once again! 😀

[Ladas]

Looks great 👍

yeah the rucocop rules were changed, I am ok with consistency, later we can run rubocop -a on all of these files, to enforce the new rules.

[Ladas]

@miq-bot assign @agrare

[Ladas]

@miha-plesko hm, can we have more snapshots per Vm or Image? If yes, the manager_ref needs to have another column to uniquely identify them. Right now we are saying 1 snapshot per Vm/Image

[miha-plesko]

If I omit :manager_ref it means it'll be inventoried based on snapshot's ems_ref, right? We have unique ems_ref on snapshot, let me try out.

[Ladas]

right, if you omit it, the default is manager_ref => [:ems_ref]

so might be that in this case we want manager_ref => [:vm_or_template, :ems_ref]

or if the :ems_ref is unique under the whole provider, just :manager_ref => [:ems_ref] should be enough. Manager ref should serve as list of columns that are unique under manager

[Ladas]

to sum the choices:
1).
manager_ref => [:ems_ref] which is the default, means :ems_ref is unique for the whole provider. Then we will just do .assign_attributes(:vm_or_template => parsed_vm) in the parser to link them together, which will make sure we first save the Vm then Snapshot

2).
manager_ref => [:vm_or_template, :ems_ref] means :ems_ref is unique under the Vm only.
Then with these we also have :parent_inventory_collections => [:vms, :miq_templates] which says the Snapshot is nested to Vm(or template). That usually means we fetch it from DB through Vms. And the main implication for targeted refresh is we always need to provide all Snapshots with the Vm target. For API it usually means we can't we fetch me all snapshots but we get them under the Vm only.

[miha-plesko]

Thanks for explanation @Ladas , for VMware option (2) seems more appropriate. I've updated the code accordingly . Will adjust the vmware PR as well, but locally it works already so many tanks 👍

[miq-bot]

Checked commit miha-plesko@34e9197 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 2 offenses detected

app/models/manager_refresh/inventory_collection_default/cloud_manager.rb

• ❗ - Line 138, Col 42 - Style/SymbolArray - Use %i or %I for an array of symbols.
• ❗ - Line 139, Col 42 - Style/SymbolArray - Use %i or %I for an array of symbols.

[Ladas]

@miha-plesko awesome, I am trying to put together better documentation for parent_inventory_collections here #17336

[agrare]

👍 LGTM

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit b991f9cb9a855e007831aadba36ba1e2cbe2f055
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Apr 24 07:55:57 2018 -0400

    Merge pull request #17335 from miha-plesko/graph-refresh-snapshot
    
    Provide base graph refresh attributes for ::Snapshot
    (cherry picked from commit 9e84461b9f0de87fd56b88e895bef2dce9cc7035)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1584727