Refresh service catalog entities #276
Conversation
Add refresh of service catalog entities
|
cc @gtanzillo |
Cleanup entities iterator and use it for every collection
| end | ||
|
|
||
| def pods | ||
| @pods ||= connection.get_pods |
There was a problem hiding this comment.
@agrare hm, you are memoizing this because you then fetch the collection.,resourceVersion from it, right?
Would it make sense to just memoize resourceVersion value? Because I would like this to be an iterator doing paginated query later
Memoize the collections for now, since we are getting the resourceVersion from it later.
| end | ||
|
|
||
| def pods | ||
| @pods ||= connection.get_pods | ||
| @pods ||= entities_iterator(connection, :pods) |
There was a problem hiding this comment.
@agrare we'll need to figure out how it works when we have paginated query and we would want the collection resourceVersion
I think for now, we should have e.g. attr_accessor :namespaces_resource_version and just write to that in the entities_iterator, what do you think?
There was a problem hiding this comment.
interesting.
A slightly open question in kubeclient iterator interface was how it should expose whole-collection resource_version (and other metadata). Should iterator yield it with every item, or return it at the end?
With streaming parse (ManageIQ/kubeclient#254), this depends on order of json. k8s puts items last, so version is known by time we start yielding; this is unlikely to change but I feel weird hardcoding this assumption...
For chunking (ManageIQ/kubeclient#283), we'll have metadata in each chunk, so either API works. Chunking also brings risk of getting 410 Gone if we wait too long, not sure how to handle that.
| @@ -68,6 +68,15 @@ def kubernetes_auth_options(options) | |||
| def kubernetes_version | |||
| 'v1' | |||
| end | |||
|
|
|||
| def kubernetes_service_catalog_connect(hostname, port, options) | |||
| options = {:path => '/apis/servicecatalog.k8s.io', :version => service_catalog_api_version}.merge(options) | |||
There was a problem hiding this comment.
we have too many ways to connect, what with ems.connect(:service => ...), also I think supporting ems.connect(:path => ...), and code somewhere I can't find now deriving path & version dynamically.
Nothing wrong with this method however 👍. Just wish I had time to clean up some of the other ways...
| def entities_iterator(client, entity) | ||
| # TODO(lsmola) change to iterator, that will fetch paginated response from the server, never fetching everything | ||
| # at once | ||
| client.send("get_#{entity}") |
There was a problem hiding this comment.
nit: it's important here that entity is plural, e.g. :pods not :pod, but this is unobvious looking at an arg named entity.
Ladas
force-pushed
the
refresh_service_catalog_entities
branch
2 times, most recently
from
e0155fb
to
623a825
Compare
Add TODO for iterator, so we can properly add it later
Ladas
force-pushed
the
refresh_service_catalog_entities
branch
from
b8b4678
to
a9937e2
Compare
|
Some comments on commits Ladas/manageiq-providers-kubernetes@4f97cdb~...a9937e2 app/models/manageiq/providers/kubernetes/inventory/collector/container_manager.rb
|
|
Checked commits Ladas/manageiq-providers-kubernetes@4f97cdb~...a9937e2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@agrare ugh, I've miss clicked, wanted to merge your AWS PR :-/ |
|
@Ladas no worries, LGTM was going to merge it when I saw it was green anyway |
Refresh service catalog entities
Depends on: