-
Notifications
You must be signed in to change notification settings - Fork 28
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
Lazy load endpoints. #188
Lazy load endpoints. #188
Conversation
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.
LGTM, there are some improvements that can be done but they might be out of scope of this PR.
@@ -47,13 +31,37 @@ def method_missing(name, *args, &block) | |||
delegate_client.__send__(method, *args, &block) | |||
end | |||
|
|||
def inventory | |||
@inventory ||= Inventory::InventoryClient.create(entrypoint: "#{@state[:entrypoint]}/hawkular/inventory", |
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.
Why does Metrics
has a different class name? Can we change the others to be like it? I like it better than to have Inventory::Inventory
and Alerts::Alerts
.
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.
Also the initialization is different on Inventory
but not on the others.
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.
I'm not sure, but at this point i don't want to break stuff like this: https://github.com/ManageIQ/manageiq/pull/13814/files#diff-7873cbd67d037fe54151ac4d277ccdd4R44
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.
You can have this at the end of the file and keep it compatible:
Inventory::InventoryClient = Inventory::Client
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.
Will open a separate PR to uniform it.
dd34ed3
to
4a2e14c
Compare
@rubenvp8510 could you please merge? |
Lazy loading the endpoints. This will keep our users for having to do stuff like ManageIQ/manageiq#13814
@cfcosta could you please review ?
// cc @pilhuhn