Moving Inventory::Builder's functions to Inventory class #17907

slemrmartin · 2018-08-27T12:41:57Z

Before this PR all providers with graph refresh repeats the same code for their inventory builders.
They didn't inherit from any common class (except Object, of course :).
This PR makes common methods in ManageIQ::Providers::Inventory, then all provider-specific builders can be removed.

dependent Inventory::Builder functions moved to Inventory manageiq-providers-amazon#480
dependent Google graph refresh manageiq-providers-google#67
dependent Inventory::Builder functions moved to Inventory manageiq-providers-azure#286
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-nuage#142
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-vmware#316
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-redfish#25
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-ovirt#284
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-openstack#343
dependent Moving Inventory Builder functionality to Inventory manageiq-providers-kubevirt#115
Final cleanup: Removing of Inventory::Builder #17948

slemrmartin · 2018-08-27T12:46:43Z

What do you think about this, @agrare, @Ladas?
Provider's builder can now look like this:
https://github.com/ManageIQ/manageiq-providers-google/pull/67/files#diff-205dcc8cfa6b66d11c8cd372db0e0713R1

I'm not sure, how to write specs for this PR, should I choose some provider and write specs for it?

Ladas · 2018-08-27T13:03:19Z

app/models/manageiq/providers/inventory/builder.rb

+
+      collector_type = "#{collector_class}::#{manager_type}Manager".safe_constantize
+      persister_type = "#{persister_class}::#{manager_type}Manager".safe_constantize
+      parser_type    = "#{parser_class}::#{manager_type}Manager".safe_constantize


this will not fit the current TargetCollection, that uses more parsers

https://github.com/Ladas/manageiq-providers-amazon/blob/4288763db1b4037076399639a825046e128a270f/app/models/manageiq/providers/amazon/builder.rb#L37

yeah I see, I'll try to extend it, maybe build_inventory can be generic only for some providers, but at least inventory method is repeated every time with the same code.
My question was, if there is some hidden reason why Builder doesn't have class in core (when all other refresh classes have one)

Ladas · 2018-08-28T07:24:43Z

app/models/manageiq/providers/inventory/builder.rb

+    def parser_class
+      "#{inventory_class}::Parser".constantize
+    rescue
+      ManageIQ::Providers::Inventory::Collector


this look like there should be ManageIQ::Providers::Inventory::Parser, and below is the same

but I think it is invalid class anyway? So we should not return it and rather let it fail?

of course, there should be Parser, thanks :)
it's not invalid class, it replaced ManagerRefresh::Inventory::Parser.
I think it's better that it raises NotImplementedError from this class, than nil error from some random place

ah, ok, if it will raise NotImplementedError, then it should be ok

..I'll find this error when writing spec, but I've question about it, please look at 2nd comment

hm ,we should still log.error here, so this place is not hidden

btw. I meant invalid that ManageIQ::Providers::Inventory::Collector base class it not valid for using.

Not sure about specs, we could use mock classes or dummy provider, @agrare ?

Ladas · 2018-08-28T07:36:48Z

app/models/manageiq/providers/inventory/builder.rb

+    # Example:
+    # %w(Cloud Network Infra)
+    def allowed_manager_types
+      raise NotImplementedError


lets provide a message to the exception

agrare · 2018-08-28T14:43:00Z

@slemrmartin why have this in a different Builder class? This looks like it could easily be just in ManageIQ::Providers::Inventory

slemrmartin · 2018-08-29T07:11:40Z

@agrare because there is many Builder classes with copy-pasted code. I think that Builders should inherit from common Builder.

Also Builders instantiates subclasses of ManageIQ::Providers:Inventory so it seems confusing to me if they would inherit from it.

agrare · 2018-08-29T15:32:52Z

because there is many Builder classes with copy-pasted code. I think that Builders should inherit from common Builder.

I agree that the providers should inherit from a common base but I don't see the benefit of a different sub-class when it could just be the top level class

Also Builders instantiates subclasses of ManageIQ::Providers:Inventory so it seems confusing to me if they would inherit from it.

Honestly it is more confusing that a sub class instantiates the top level class. When Builder creates ManageIQ::Providers::Inventory that already has instances of Collector, Parser, and Persister.

Something like ManageIQ::Providers::Inventory.build() makes a lot more sense than some sub-class returning an instance of a parent class.

Ladas · 2018-08-29T15:52:01Z

ManageIQ::Providers::Inventory.build() will be probably enough here

slemrmartin · 2018-08-30T10:02:17Z

@agrare

I agree that the providers should inherit from a common base but I don't see the benefit of a different sub-class when it could just be the top level class

I don't understand what you mean. Provider specific classes cannot be fully removed. For example Amazon with [Parser,Collector,Persister]::StorageManager::[Ebs, S3]'s classes cannot be generalized. In case of Amazon, Storage Parser for TargetCollection cannot be determined automatically.

Honestly it is more confusing that a sub class instantiates the top level class.

What do you mean by that? Provider specific builder instantiates provider specific inventory, parser,collector,persister, not the top level class (top level classes are instantiated only in case of non-existing classes - which then causes NotImplementedError on non-random place (at least Parser, as I see))

Something like ManageIQ::Providers::Inventory.build() makes a lot more sense than some sub-class returning an instance of a parent class.

The same question, what do you mean by returning instance of a parent class there?

Do you suggest to completely remove all Builders from providers and move method Builder.build_inventory -> Inventory.build?

It's definitely possible, but complexity remains the same, not everything can be generalized to top-level class

agrare · 2018-08-30T11:31:50Z

I don't understand what you mean. Provider specific classes cannot be fully removed.

That's not what I'm talking about, I'm saying instead of adding a new top-level class Builder that we put this functionality in ::Inventory.

It's definitely possible, but complexity remains the same, not everything can be generalized to top-level class

Of course not everything can be generalized but the complexity will be less in the sense of someone wanting to create an instance of ManageIQ::Providers::Amazon::Inventory (for example) just needs to call .build on that class. Code-wise it isn't less complex but the interface makes more sense.

slemrmartin · 2018-08-30T13:26:37Z

Thanks @agrare, that makes sense to me, working on it :)

agrare · 2018-08-30T16:46:48Z

app/models/manageiq/providers/base_manager/manager_refresher.rb

+            begin
+              inventory = builder_class_for(ems.class).build_inventory(ems, target)
+            rescue NameError
+              inventory = inventory_class_for(ems.class).build(ems, target)


Hey what if we add an alias for build_inventory -> build so we can call this one way while we refactor the providers?

agrare · 2018-08-30T17:00:11Z

app/models/manageiq/providers/base_manager/manager_refresher.rb

+            begin
+              inventory = builder_class_for(ems.class).build_inventory(ems, target)
+            rescue NameError
+              inventory = inventory_class_for(ems.class).build(ems, target)


Looks like the comment got overwritten but I think we should just alias build_inventory build and save the rescue here, we can remove it once we move all the providers over

Oh just noticed the builder_class_for part, at first I thought the whole line was the same except for build / build_inventory

slemrmartin · 2018-08-31T07:57:26Z

@miq-bot add_label refactoring

slemrmartin · 2018-08-31T08:09:59Z

app/models/manageiq/providers/inventory.rb

+    #
+    # @param klass class of the Provider/Manager
+    # @return [Class] Correct class name of the persister
+    def self.persister_class_for(klass)


@agrare I only changed order of these methods in file, no change there

I'm good with this for now, I think these should all be private and then all of the yardoc comments are unnecessary

Actually where is persister_class_for used?

it's used by old refresher: https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/base_manager/refresher.rb#L168

Ah okay 👍

miq-bot · 2018-08-31T13:31:21Z

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

agrare · 2018-08-31T17:51:57Z

app/models/manageiq/providers/inventory.rb

+    def self.build(ems, target)
+      new(
+        class_for(ems, target, 'Persister').new(ems, target),
+        class_for(ems, target, 'Collector').new(ems, target),


Instead of the general class_for here how about we call collector_class_for and persister_class_for here to give providers a change to override them?

miq-bot · 2018-08-31T17:52:31Z

Checked commits slemrmartin/manageiq@a54b241~...f3fd91d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

agrare · 2018-08-31T17:58:27Z

@slemrmartin looks good overall, I'm going to merge this so we can finish cleaning up the provider plugins and then we can come back and revisit this

slemrmartin mentioned this pull request Aug 27, 2018

Google graph refresh ManageIQ/manageiq-providers-google#67

Merged

3 tasks

miq-bot added the wip label Aug 27, 2018

Ladas reviewed Aug 27, 2018

View reviewed changes

slemrmartin mentioned this pull request Aug 27, 2018

Inventory::Builder functions moved to Inventory ManageIQ/manageiq-providers-amazon#480

Merged

1 task

slemrmartin force-pushed the inventory-builder branch 2 times, most recently from 3cfecc2 to fbabe32 Compare August 27, 2018 19:18

Ladas reviewed Aug 28, 2018

View reviewed changes

agrare self-assigned this Aug 28, 2018

slemrmartin force-pushed the inventory-builder branch from 72a03bc to 25447e0 Compare August 30, 2018 16:37

agrare reviewed Aug 30, 2018

View reviewed changes

slemrmartin force-pushed the inventory-builder branch from 25447e0 to 8eaf563 Compare August 30, 2018 16:52

agrare reviewed Aug 30, 2018

View reviewed changes

slemrmartin force-pushed the inventory-builder branch 2 times, most recently from 2050a30 to 340565c Compare August 31, 2018 07:53

slemrmartin changed the title ~~[WIP] Generic Builder for inventory~~ Moving Inventory::Builder's functions to Inventory class Aug 31, 2018

miq-bot removed the wip label Aug 31, 2018

miq-bot added the refactoring label Aug 31, 2018

slemrmartin commented Aug 31, 2018

View reviewed changes

miq-bot added the unmergeable label Aug 31, 2018

slemrmartin added 3 commits August 31, 2018 13:38

Generic Builder for inventory

a54b241

Inventory Builder for TargetCollection

5fc0e48

Moving Inventory::Builder function to Inventory class

3620846

agrare force-pushed the inventory-builder branch from 340565c to 3620846 Compare August 31, 2018 17:38

Remove trailing whitespace

f3fd91d

agrare reviewed Aug 31, 2018

View reviewed changes

agrare removed the unmergeable label Aug 31, 2018

agrare approved these changes Aug 31, 2018

View reviewed changes

agrare merged commit 4a92a5a into ManageIQ:master Aug 31, 2018

agrare added this to the Sprint 94 Ending Sept 10, 2018 milestone Aug 31, 2018

slemrmartin deleted the inventory-builder branch September 3, 2018 08:34

agrare added the providers/inventory label Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving Inventory::Builder's functions to Inventory class #17907

Moving Inventory::Builder's functions to Inventory class #17907

slemrmartin commented Aug 27, 2018 •

edited

Loading

slemrmartin commented Aug 27, 2018

Ladas Aug 27, 2018

slemrmartin Aug 27, 2018

Ladas Aug 28, 2018

slemrmartin Aug 28, 2018

Ladas Aug 28, 2018

slemrmartin Aug 28, 2018

Ladas Aug 28, 2018

Ladas Aug 28, 2018

Ladas Aug 28, 2018

agrare commented Aug 28, 2018

slemrmartin commented Aug 29, 2018

agrare commented Aug 29, 2018

Ladas commented Aug 29, 2018

slemrmartin commented Aug 30, 2018

agrare commented Aug 30, 2018

slemrmartin commented Aug 30, 2018

agrare Aug 30, 2018

agrare Aug 30, 2018

agrare Aug 30, 2018

slemrmartin commented Aug 31, 2018

slemrmartin Aug 31, 2018

agrare Aug 31, 2018

agrare Aug 31, 2018

slemrmartin Sep 3, 2018

agrare Sep 4, 2018

miq-bot commented Aug 31, 2018

agrare Aug 31, 2018

miq-bot commented Aug 31, 2018

agrare commented Aug 31, 2018

Moving Inventory::Builder's functions to Inventory class #17907

Moving Inventory::Builder's functions to Inventory class #17907

Conversation

slemrmartin commented Aug 27, 2018 • edited Loading

slemrmartin commented Aug 27, 2018

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

agrare commented Aug 28, 2018

slemrmartin commented Aug 29, 2018

agrare commented Aug 29, 2018

Ladas commented Aug 29, 2018

slemrmartin commented Aug 30, 2018

agrare commented Aug 30, 2018

slemrmartin commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slemrmartin commented Aug 31, 2018

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

miq-bot commented Aug 31, 2018

Choose a reason for hiding this comment

miq-bot commented Aug 31, 2018

agrare commented Aug 31, 2018

slemrmartin commented Aug 27, 2018 •

edited

Loading