Shared definitions of cloud manager targets

[Ladas]

Shared definitions of cloud manager targets

[Ladas]

@durandom so this is how it looks like for me I think

then aws would have

class ManageIQ::Providers::Amazon::Inventory::Target::CloudManager < ManageIQ::Providers::Amazon::Inventory::Target
  def cloud
    ManageIQ::Providers::Amazon::InventoryCollectionDefault::CloudManager
  end
  
  def initialize_inventory_collections
    add_inventory_collections(
      cloud,
      %i(vms miq_templates hardwares networks disks availability_zones
         flavors key_pairs orchestration_stacks orchestration_stacks_resources
         orchestration_stacks_outputs orchestration_stacks_parameters orchestration_templates)
    )
  end
end

[durandom]

I think the base class should be at app/models/manageiq/providers/inventory/targets/cloud_manager

Then there should something like a class_method that sets the module, because in most cases this is the only thing that changes.

Like

class Target::CloudManager
  def vms
    ManagerRefresh::InventoryCollection.new(model_class: provider_module::CloudManager::Vm)
  end
end

class Amazon::Inventory::Target::CloudManager
  def provider_module
    ManageIQ::Providers::Amazon
  end
end

and even that could be somehow automagically inferred, that you eventually, maybe just have to inherit from the base target and it works

[Ladas]

@durandom inheritance will not work here, since e.g in NetworkManager, we need ICs of CloudManager (same for StorageManager)

So we either need them as class methods or modules I think

[miq-bot]

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

[Ladas]

@miq-bot remove_label wip

[Ladas]

@miq-bot assign @agrare

[Ladas]

@agrare you could extract Infra for Vmware here. might be, we could move some definitions to a shared superclass

[miq-bot]

Checked commits Ladas/manageiq@41060e1~...9bcc88d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 2 offenses detected

app/models/manager_refresh/inventory_collection_default/cloud_manager.rb

• ❗ - Line 183, Col 17 - Rails/SkipsModelValidations - Avoid using update_attribute because it skips validations.
• ❗ - Line 64, Col 106 - Rails/FindBy - Use find_by instead of where.first.

[Ladas]

@durandom @agrare then this is how AWS uses these ManageIQ/manageiq-providers-amazon#134

I do like the 'add_inventory_collection' helper to fill up what can be inferred automatically and 'add_inventory_collections' helper for adding many ICs with the same config

The 'add_remaining_inventory_collections' is also cool, but probably only to easily say 'rest is coming from the DB' for the targetted refresh. Otherwise I like that Inventory::Target has to name every IC that will be saved

[durandom]

Can you show me where those Defaults are used? To me this looks like another layer of abstraction

[durandom]

I think this should be at app/models/manageiq/providers/inventory/targets/cloud_manager - then it's closer to the actual models

[durandom]

oh, if its a target only

[Ladas]

yeah, this is default for InventoryCollection, under any target

[durandom]

attributes reminds me of the model attributes
but this is rather params for creating the InventoryCollections right?

[Ladas]

yeah, possibly :extra_params might be more descriptive

[durandom]

I appreciate the effort put into simplifying this and I'm ok with merging this as is so we can further improve on it.

All in all I dont like hashes being passed around here. I would rather see the InventoryCollections created directly. But not sure.

[Ladas]

@durandom those are kwargs not hashes :-D a biiig difference :-D

[durandom]

@durandom those are kwargs not hashes :-D a biiig difference :-D

I only see hashes in this PR 😛 Maybe you use them later as kwargs, but still it starts with a hash