Enhanced inventory collector target and parser classes #13907

Ladas · 2017-02-14T13:16:25Z

Enhanced inventory collector target and parser classes to align with the Amazon usecase.

ManagerRefresh::Inventory::Parser is the new base class for refresh.

Modify Base target class to align with Amazon usecase

Modify Collector base class to align with Amazon usecase

Modify Inventory base class to align with Amazon usecase

Target initialize is taking a collector object as a parameter

Modify Ansible Target to use the new target and manager methods

durandom · 2017-02-14T13:31:58Z

app/models/manager_refresh/inventory/collector.rb


+  # @param manager [ManageIQ::Providers::BaseManager] A manager object
+  # @param target [ActiveRecord|Hash] A refresh Target object


Why could this be a Hash?

durandom · 2017-02-14T13:32:41Z

app/models/manager_refresh/inventory/target.rb

-  def collections
-    @collections ||= {}
+  # @param collector [ManagerRefresh::Inventory::Collector] A collector object
+  def initialize(collector)


not sure if I like the collector being tied to this class.
This increases the coupling

Ansible tower enhance target IC definitions

Unify Target and Collector interface

Allow to overwrite aninsible IC defaults

djberg96 · 2017-02-14T15:28:00Z

@Ladas Will this require any changes to the external provider repos?

Rename Target to Persister under Inventory

Ladas · 2017-02-14T16:36:02Z

@djberg96 yes, but currently only AWS should be affected.

@Fryguy we are going with Inventory::Persister instead of Inventory::Target

@durandom so now we can do persister.configured_systems instead of persister.collections[:configured_systems]. It uses a method missing that defines a getter using collections, so it should be quick enough.

Dynamic access of collections with Rails like method cache, first pass goes through methods missing and looks into collections hash by creating a getter metho. So second+ pass is much faster.

durandom

I really ❤️ that we move from Inventory::Target to Inventory::Persister - though this name is still a bit strange, but 🤷‍♂️ <- thanks github

Besides internal naming of some attributes and some comments about future refactorings, thats good stuff

durandom · 2017-02-15T07:53:30Z

app/models/manager_refresh/inventory/persister.rb

+
+  def self.define_collections_reader(collection_key)
+    define_method(collection_key) do
+      collections.try(:[], collection_key)


why try here? collections will always respond to []

yeah, it's initialized to {}, so it should be safe

durandom · 2017-02-15T07:55:52Z

app/models/manager_refresh/inventory/persister.rb

+  attr_reader :manager, :target, :collections
+
+  # @param manager [ManageIQ::Providers::BaseManager] A manager object
+  # @param target [Object] A refresh Target object


So, do we already have something else than an ActiveRecord::Base as a target?
If not, I'd put this in here.

Yes, I have the TargetCollection, which is not AR based class

could you put an OR (|) in there? knowing it is an ActiveRecord::Base (or similar) seems to be helpful

yeah, I need to finish the targeted refresh to know what will come through here, so I was thinking Object for now, which is always true :-D

durandom · 2017-02-15T07:56:54Z

app/models/manager_refresh/inventory/persister.rb

+
+  # @param manager [ManageIQ::Providers::BaseManager] A manager object
+  # @param target [Object] A refresh Target object
+  def initialize(manager, refresh_target)


this should be target

durandom · 2017-02-15T07:59:58Z

app/models/manager_refresh/inventory/persister.rb

+  #
+  # @param inventory_collection_data [Hash] Hash used for ManagerRefresh::InventoryCollection initialize
+  def add_inventory_collection(inventory_collection_data)
+    data = inventory_collection_data


why this assignment?
I find neither data nor inventory_collection_data really revealing.
How about init_params or just params, or just options - at least options is what we are used to read as init_options

I think I just wanted to fit to 120 cols :-) not sure why I did this. :-)

'options' sounds good

durandom · 2017-02-15T08:04:41Z

app/models/manager_refresh/inventory/persister.rb

+    data = inventory_collection_data
+    data[:parent] ||= manager
+
+    if !data.key?(:delete_method) && data[:model_class]


What happens if there is no :delete_method and not :model_class ?

If thats a problem, I think that should be guarded in the initializer of InventoryCollection

Maybe not now.

the default delete method is always rails :destroy, this helper just sets it to disconnect_inv if there is any defined

The alternative might be to have this defined in the MIQ core definitions

durandom · 2017-02-15T08:06:29Z

app/models/manager_refresh/inventory/persister.rb

+
+    if !data.key?(:delete_method) && data[:model_class]
+      # Automatically infer what the delete method should be, unless the delete methods was given
+      data[:delete_method] = data[:model_class].new.respond_to?(:disconnect_inv) ? :disconnect_inv : nil


I don't really like the coupling and magic.
I rather have this explicit in the Default classes.

You know, the less magic the better. People have to directly see they need a :delete_method.

yeah, make sense, I was leaning towards this too

durandom · 2017-02-15T08:07:15Z

app/models/manager_refresh/inventory/persister.rb

+  # @param default [ManagerRefresh::InventoryCollectionDefault] Default
+  # @param inventory_collections [Array] Array of method names for passed default parameter
+  # @param inventory_collections_data [Hash] Hash used for ManagerRefresh::InventoryCollection initialize
+  def add_inventory_collections(default, inventory_collections, inventory_collections_data = {})


same comment about _data here

I dont really like the default and how it just emmits hashes...
But ok for now and do some refactoring later

durandom · 2017-02-15T08:10:45Z

app/models/manager_refresh/inventory/persister.rb

+  #
+  # @param defaults [Array] Array of ManagerRefresh::InventoryCollectionDefault
+  # @param inventory_collections_data [Hash] Hash used for ManagerRefresh::InventoryCollection initialize
+  def add_remaining_inventory_collections(defaults, inventory_collections_data = {})


This is an indicator, that the InventoryCollectionDefault concept is somehow not fitting.
So leave it for now, but I have the feeling this will and should go away

right, so this helper is mainly for the Targeted refresh, where I define few ICs and want to be able to easily say "Rest of the ICs should come from a DB"

Renames in Persister according to review

Explicitly define a delete method instead of auto-magic, the default IC definitions have delete_method defined, if it differs from a default :destroy.

Ladas · 2017-02-15T10:06:46Z

@miq-bot assign @agrare

miq-bot · 2017-02-15T10:13:01Z

Checked commits Ladas/manageiq@28a74aa~...85ad5bf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 0 offenses detected
Everything looks good. 🍰

durandom

👍 from me

kbrock

This looks really nice.

just a few nits - all of them can be ignored

kbrock · 2017-02-15T18:32:42Z

app/models/ems_refresh/refreshers/ems_refresher_mixin.rb

@@ -135,13 +135,13 @@ def parse_targeted_inventory(ems, target, collector)
          _log.debug "#{log_header} Parsing inventory..."
          inventory_collections, = Benchmark.realtime_block(:parse_inventory) do
            provider_module = ManageIQ::Providers::Inflector.provider_module(ems.class).name
-            inventory_target_class = "#{provider_module}::Inventory::Target::#{target.class.name.demodulize}".safe_constantize
-            inventory_target = inventory_target_class.new(target)
+            persister_class = "#{provider_module}::Inventory::Persister::#{target.class.name.demodulize}".safe_constantize


Are you using strings/safe_constantize to avoid class not found?

(basically asking will this work if you don't .name, but just reference the class)?

Good point, and I'm chiming in, because the original code was mine, so I'm to blame 😉
So, if the class is not found, it would return nil and nil.new will blow up.
So we should assume the class is there, check if its not and raise an exception like you should really implement this - so the dev knows what he's missing

kbrock · 2017-02-15T18:35:33Z

app/models/manager_refresh/inventory/persister.rb

+  attr_reader :manager, :target, :collections
+
+  # @param manager [ManageIQ::Providers::BaseManager] A manager object
+  # @param target [Object] A refresh Target object


could you put an OR (|) in there? knowing it is an ActiveRecord::Base (or similar) seems to be helpful

kbrock · 2017-02-15T18:41:30Z

app/models/manageiq/providers/ansible_tower/inventory/target/automation_manager.rb

-    collections[:inventory_groups] ||= ManagerRefresh::InventoryCollection.new(
-      :model_class    => ManageIQ::Providers::AutomationManager::InventoryRootGroup,
-      :association    => :inventory_root_groups,
-      :parent         => @root,


really like how @root (single refresh specific variable) was removed from the collection definition.

Would be nice if Persister definition was reusable. (future thought)

agrare · 2017-02-16T21:33:20Z

though this name is still a bit strange, but 🤷‍♂️

Agree Inventory::Persister is strange but its better than Target, we can revisit this if we can think of a better name :)

agrare · 2017-02-16T21:35:43Z

app/models/manager_refresh/inventory_collection_default/cloud_manager.rb

@@ -4,6 +4,7 @@ def vms(extra_attributes = {})
      attributes = {
        :model_class          => ::ManageIQ::Providers::CloudManager::Vm,
        :association          => :vms,
+        :delete_method        => :disconnect_inv,


I like the explicit delete method 👍

agrare · 2017-02-16T21:37:55Z

This LGTM 👍

Ladas added 6 commits February 14, 2017 14:15

Remove old base classes for new refresh

28a74aa

ManagerRefresh::Inventory::Parser is the new base class for refresh.

Modify Base target class to align with Amazon usecase

e2d8125

Modify Base target class to align with Amazon usecase

Modify Collector base class to align with Amazon usecase

0f57da2

Modify Collector base class to align with Amazon usecase

Modify Inventory base class to align with Amazon usecase

9435aca

Modify Inventory base class to align with Amazon usecase

Target initialize is taking a collector object as a parameter

4fc9ecf

Target initialize is taking a collector object as a parameter

Modify Ansible Target to use the new target and manager methods

6ec3730

Modify Ansible Target to use the new target and manager methods

durandom reviewed Feb 14, 2017

View reviewed changes

Ladas added 3 commits February 14, 2017 14:34

Ansible tower enhance target IC definitions

42ab658

Ansible tower enhance target IC definitions

Unify Target and Collector interface

bcf3d5d

Unify Target and Collector interface

Allow to overwrite aninsible IC defaults

b58126d

Allow to overwrite aninsible IC defaults

chessbyte assigned blomquisg Feb 14, 2017

chessbyte requested a review from Fryguy February 14, 2017 16:18

chessbyte added enhancement providers labels Feb 14, 2017

Rename Target to Persister under Inventory

8981373

Rename Target to Persister under Inventory

Dynamic access of collections with Rails like method cache

c333f2d

Dynamic access of collections with Rails like method cache, first pass goes through methods missing and looks into collections hash by creating a getter metho. So second+ pass is much faster.

Ladas force-pushed the enhanced_inventory_collector_target_and_parser_classes branch from 12ba718 to c333f2d Compare February 14, 2017 16:43

Ladas mentioned this pull request Feb 14, 2017

Use base classes for Inventory, Collector, Persistor and Parser ManageIQ/manageiq-providers-amazon#139

Merged

durandom suggested changes Feb 15, 2017

View reviewed changes

Ladas added 2 commits February 15, 2017 10:56

Renames in Persister according to review

4214aa2

Renames in Persister according to review

Explicitly define a delete method instead of auto-magic

85ad5bf

Explicitly define a delete method instead of auto-magic, the default IC definitions have delete_method defined, if it differs from a default :destroy.

miq-bot assigned agrare and unassigned blomquisg Feb 15, 2017

durandom approved these changes Feb 15, 2017

View reviewed changes

Ladas mentioned this pull request Feb 15, 2017

Use inventory object writers and readers instead of hash #13923

Merged

1 task

kbrock reviewed Feb 15, 2017

View reviewed changes

durandom mentioned this pull request Feb 16, 2017

[WIP] Use SDK4 for refreshing #13742

Closed

agrare reviewed Feb 16, 2017

View reviewed changes

agrare merged commit 063b42d into ManageIQ:master Feb 16, 2017

agrare added the euwe/no label Feb 16, 2017

agrare added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced inventory collector target and parser classes #13907

Enhanced inventory collector target and parser classes #13907

Ladas commented Feb 14, 2017

durandom Feb 14, 2017

Ladas Feb 14, 2017

durandom Feb 14, 2017

djberg96 commented Feb 14, 2017

Ladas commented Feb 14, 2017

durandom left a comment

durandom Feb 15, 2017

Ladas Feb 15, 2017

durandom Feb 15, 2017

Ladas Feb 15, 2017

kbrock Feb 15, 2017

Ladas Feb 16, 2017

durandom Feb 15, 2017

durandom Feb 15, 2017

Ladas Feb 15, 2017 •

edited

Loading

durandom Feb 15, 2017

Ladas Feb 15, 2017

durandom Feb 15, 2017

Ladas Feb 15, 2017

durandom Feb 15, 2017

durandom Feb 15, 2017

durandom Feb 15, 2017

Ladas Feb 15, 2017

Ladas commented Feb 15, 2017

miq-bot commented Feb 15, 2017

durandom left a comment

kbrock left a comment

kbrock Feb 15, 2017

durandom Feb 16, 2017 •

edited

Loading

kbrock Feb 15, 2017

kbrock Feb 15, 2017

agrare commented Feb 16, 2017

agrare Feb 16, 2017

agrare commented Feb 16, 2017


		# @param manager [ManageIQ::Providers::BaseManager] A manager object
		# @param target [ActiveRecord\|Hash] A refresh Target object

Enhanced inventory collector target and parser classes #13907

Enhanced inventory collector target and parser classes #13907

Conversation

Ladas commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 commented Feb 14, 2017

Ladas commented Feb 14, 2017

durandom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas commented Feb 15, 2017

miq-bot commented Feb 15, 2017

durandom left a comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Feb 16, 2017

Choose a reason for hiding this comment

agrare commented Feb 16, 2017

Ladas Feb 15, 2017 •

edited

Loading

durandom Feb 16, 2017 •

edited

Loading