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

[WIP] Use SDK4 for refreshing #13742

Closed
wants to merge 10 commits into from

Conversation

borod108
Copy link

@borod108 borod108 commented Feb 2, 2017

this is WIP only implamented for full refresh and still a lot of refactoring to do.

@borod108
Copy link
Author

cc @durandom would love to hear your thoughts.

@durandom
Copy link
Member

Thats an interesting approach.
So as I understand it, you have the need to do a refresh for v3 and v4 of the ovirt API.
And the refresh part of the platform should handle that transparently.
But thats only for parsing the inventory coming from ems.rhevm_inventory

Then you have parsing strategies.
If I get it right, parse/strategies/api4.rb is the api specific parser and parse/strategies/vm_inventory.rb would be the common stuff for vm as a target - api3 and api4 agnostic.

Thats a lot of guessing - but its also quite some code :)

So, I think we are solving similar requirements with the current graph refresh. Please have a look at #13728 which has a nice description of the code layout. It will change slightly with #13907 - but the concept stays the same.
Building upon this would give you the seperation of inventory collection, parsing and persistance. Which leaves you focusing on the parsing stuff - as this is what really differs.
It'll also make refesh/refresher.rb much easier - to the point it can use the stock refresher.

Although the API version switches, that you require, are not baked into this, it would be easy to extend. You can apply your strategy pattern to the parsers in the graph refresh.

So my advice would be to collaborate on using the platform approach above - work out what shortcomings we have, that you need.

Also you might look at the ansible refresh which is a small straight forward implementation:
https://github.com/ManageIQ/manageiq/tree/9e2b41dd6d381c458eb29907b9ed9f6af9887091/app/models/manageiq/providers/ansible_tower/inventory

cc @oourfali @Ladas @pkliczewski

@borod108 borod108 force-pushed the rfe/new_inventory4 branch 2 times, most recently from 6de9a85 to 8a037cf Compare February 22, 2017 12:45
@miq-bot
Copy link
Member

miq-bot commented Feb 22, 2017

Some comments on commits borod108/manageiq@64a0518~...d972728

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_1_spec.rb

  • ⚠️ - 22 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 24 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 26 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 28 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 30 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 32 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 34 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 36 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 37 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Feb 22, 2017

Checked commits borod108/manageiq@64a0518~...d972728 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 11 offenses detected

app/models/manageiq/providers/redhat/infra_manager/refresh/parse/strategies/vm_inventory.rb

app/models/manageiq/providers/redhat/infra_manager/refresh/strategies/api4.rb

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_1_spec.rb

@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2017

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

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.

6 participants