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

Reimplement list kinds to be more generic #240

Merged
merged 3 commits into from
Dec 18, 2018

Conversation

fabianvf
Copy link
Member

@fabianvf fabianvf commented Nov 28, 2018

This implementation will support both typed and untyped List kinds, and we now automatically add the fake v1.List resource during discovery.

Should fix one of the issues brought up in redhat-cop/openshift-applier#39, also recorded in ansible/ansible#49100, and will close #209

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2018
@fabianvf
Copy link
Member Author

fabianvf commented Dec 13, 2018

@willthames or @asetty any reservations about this one going in?

@@ -916,7 +987,7 @@ def main():
item = {}
item[key] = {k: v for k, v in resource.__dict__.items() if k not in ('client', 'subresources', 'resource')}
if isinstance(resource, ResourceList):
item[key]["resource"] = '{}.{}'.format(resource.resource.group_version, resource.resource.kind)
item[key]["resource"] = '{}.{}'.format(resource.group_version, resource.kind)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be resource.base_kind instead of resource.kind?
Maybe you don't need this at all because base_kind is already an attribute of the resource.

@@ -738,7 +809,7 @@ def __iter__(self):
prefix, group, version, rg.preferred)
self._cache['resources'][prefix][group][version] = rg
self.__update_cache = True
for resource in rg.resources:
for _, resource in rg.resources.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Maybe use iteritems instead so you get an iterator rather than create a list.

Copy link
Contributor

@asetty asetty left a comment

Choose a reason for hiding this comment

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

I think this is a good change so ResourceLists can be treated more like an actual Resource.
Looks good to me overall, commented on a couple things I noticed.

@fabianvf fabianvf merged commit 2bfc077 into openshift:master Dec 18, 2018
willthames pushed a commit to willthames/openshift-restclient-python that referenced this pull request Feb 3, 2019
* Reimplement list kinds to generically support typed and untyped lists

* Fix various issues from rebase

* Use iteritems from six
willthames pushed a commit to willthames/openshift-restclient-python that referenced this pull request Nov 22, 2019
* Reimplement list kinds to generically support typed and untyped lists

* Fix various issues from rebase

* Use iteritems from six
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to patch lists
3 participants