Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inventory abstraction for data fetching and storing in inventory collections #98

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Jan 6, 2017

Inventory abstraction for data fetching and storing in inventory collections

@Ladas Ladas changed the title Inventory abstraction for data fetching and storing in inventory collections [WIP](Depends on #95) Inventory abstraction for data fetching and storing in inventory collections Jan 6, 2017
@Ladas
Copy link
Contributor Author

Ladas commented Jan 6, 2017

@durandom @agrare inventory abstraction, so in this way, the parsing code will be the same for everything, then the whole refresh is defined in Inventory::Targets::XY

AWS Inventory base class, serving for defining connections to
AWS API
AWS inventory Factory serving for getting a correct Target
Inventory based on a target or EmsEvent.
HashCollection wrapper for AWS API results allowing us to
convert API result into general hash format, we use also
when parsing event's payload.
A helper for default InventoryCollection attributes, in most
cases it's enough to change one attribute when creating
InventoryCollection with a different source.
Contains a helper for adding an InventoryCollection into
inventory. And a helper for parsing AWS event payload.
A target for refreshing all objects under a cloud manager,
formerly known as a cloud manager full refresh.
A target for refreshing all objects under a network manager,
formerly known as a network manager full refresh.
EventPayloadVm server for refreshing of a VM based on AWS
event payload.
Helpers for adding multiple inventory collections
Use helper for adding multiple inventory collections for targets
Add hardware's custom finder and vm's check_changed
@Ladas Ladas force-pushed the inventory_abstraction_for_data_fetching_and_storing_in_inventory_collections branch from c8c0ee2 to b6ce29e Compare January 9, 2017 20:33
@Ladas Ladas changed the title [WIP](Depends on #95) Inventory abstraction for data fetching and storing in inventory collections Inventory abstraction for data fetching and storing in inventory collections Jan 9, 2017
@Ladas Ladas requested review from durandom and agrare January 9, 2017 20:41
Fix rubocop issues
NetworkManager target fix, typo in stacks name
Use resourceId so the code works also for instance delete
Add default targets into the factory, in the case that target
is noit recognized.
@miq-bot
Copy link
Member

miq-bot commented Jan 11, 2017

Checked commits Ladas/manageiq-providers-amazon@3021bfc~...f1d228c with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 9 offenses detected

app/models/manageiq/providers/amazon/inventory/inventory_collection_default_init_data.rb

app/models/manageiq/providers/amazon/inventory/targets.rb

app/models/manageiq/providers/amazon/inventory/targets/cloud_manager.rb

app/models/manageiq/providers/amazon/inventory/targets/event_payload_vm.rb

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general direction of smaller methods and non-linear build up of InventoryObjects.

But I'm not sure wether this makes it easier to understand how to write the provider side of a refresher. It still feels there is a lot of boiler plate and repeated words. E.g. cloud_subnets appears 4 times. In a perferct world there would be only one method to retrieve them 😄

Maybe inventory_collection_default_init_data can be more generalized and build itself from a common base and then have some overrides at the provider level?

But as it does not affect the current refresh and could be a base to gradually move forward, I'd be ok to merge.

@agrare ?

@@ -0,0 +1,25 @@
class ManageIQ::Providers::Amazon::Inventory::Targets::EventPayloadVm < ManageIQ::Providers::Amazon::Inventory::Targets
def initialize_inventory_collections
instance_ems_ref = target[:full_data]["configurationItem"]["resourceId"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this whole Target class only fills the ems_ref and the rest is coming from the db?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ems_ref is for InventoryCollections init, so we know how to compare it. Then the data itself comes from event, in the instances method.

Which will result in create/update/delete based on the state of our db and the event

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure this makes anything drastically simpler but I'm not against testing it on amazon

@durandom durandom merged commit 8a1cfad into ManageIQ:master Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants