-
Notifications
You must be signed in to change notification settings - Fork 898
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
Fix of inventory building and refactoring #17942
Conversation
rescue ManageIQ::Providers::Inflector::ObjectNotNamespacedError => _err | ||
nil | ||
def self.persister_class_for(ems, target = nil) | ||
target = ems if target.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a need for one-param method from there: https://github.com/ManageIQ/manageiq/blob/master/app/models/ems_refresh.rb#L133
@agrare pls look&merge |
@agrare change of persister_class_for and parser_class_for also tested against all providers specs |
@slemrmartin looks like a legitimate spec failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - just the one comment (only if you need to re-push to make travis pass)
|
||
# Multiple parser classes | ||
# Can be implemented in subclass when custom set needed (mainly for TargetCollection) | ||
def self.parser_classes_for(ems, target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if you need to change the code to fix the travis failure...
Is it possible to keep the method back here?
if you moved it for a reason, that is good, but if there is no reason, then it is easier to navigate fewer changes.
thnx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem
Also unifies _class_for helpers.
@agrare I forget to test against ansible and it doesn't have namespacing like others, so this err is caused by this PR |
92b93bd
to
3ee3c8b
Compare
Checked commits slemrmartin/manageiq@85306be~...3ee3c8b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
persister = ManageIQ::Providers::Inventory.persister_class_for(target.class).new(ems, target) | ||
parser = ManageIQ::Providers::Inventory.parser_class_for(target.class).new | ||
persister = ManageIQ::Providers::Inventory.persister_class_for(ems, target, target.class.name.demodulize).new(ems, target) | ||
parser = ManageIQ::Providers::Inventory.parser_class_for(ems, target, target.class.name.demodulize).new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding target.class.name.demodulize
makes the same behaviour as old parser_class_for
method.
I choose this solution rather than some old_parser_class_for
method
Problem is that Amazon's Storage manager's parser and AnsibleTower::AutomationManager::ConfigurationScriptSource's parser doesn't have compatible "suffix"
ManageIQ::Providers::Inventory.build()
creates persister and collector classes in bad order, which causes problem with non-collected data in persister.Next provides helpers
collector_class_for
,parser_class_for
andpersister_class_for
.Existing ones are using
class_for
method now.