Make list from kind resource item #272
Conversation
openshift-ci-robot
added
size/M
|
Hi @machacekondra. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@fabianvf please review |
machacekondra
force-pushed
the
resource_kind_list
branch
3 times, most recently
from
3339632 to
a05828f
Compare
|
This seems like a massively breaking change - every user of this library would need to change how it dealt with resources. I'm really not in favour of such a huge change for one tiny use case. I'd rather return a list only when multiple resources are found and let the clients handle that weirdness. |
|
It does exactly that. It did it even previously. You could construct search which returned multiple object and user got a list, or one object or exception in case of |
|
Thanks @machacekondra, I misunderstood the scope of this change. |
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
|
@machacekondra thanks a ton for the submission! @willthames did you test this with the Ansible suite already? |
|
I haven't tested it yet - I thought I'd let the CI do the initial testing |
openshift/dynamic/client.py
Outdated
| elif isinstance(resourcePart, dict): | ||
| return self.__search(parts[1:], resourcePart) | ||
| else: | ||
| if isinstance(resourcePart, ResourceList): |
There was a problem hiding this comment.
I'm not sure I understand what this if block is doing, why would you need to wrap this in a list value only when it's a ResourceList?
There was a problem hiding this comment.
That's because you add ResourceList to resources as non-list on line 645:
groups[DISCOVERY_PREFIX] = {'': {
'v1': ResourceGroup(True, resources = {"List": ResourceList(self.client)})
}}Maybe it would be more reasonable to remove this condition, and change the 645 line to
groups[DISCOVERY_PREFIX] = {'': {
'v1': ResourceGroup(True, resources = {"List": [ResourceList(self.client)]})
}}There was a problem hiding this comment.
What do you think?
There was a problem hiding this comment.
Ah I see, that makes sense. I think it would be better to change 645 and have a consistent data structure
machacekondra
force-pushed
the
resource_kind_list
branch
from
a05828f to
5acb3f4
Compare
Previously when creating a resource list, kind was a dictionary meaning that there could be only one resource for specific kind. But for example Template kind can have to different resources: $ oc api-resources | grep Template$ processedtemplates template.openshift.io true Template templates template.openshift.io true Template So The client previsously found just one resource for kind Template. In order to work with both resources for that kind this patch introduces the change that kind will be list by default.
machacekondra
force-pushed
the
resource_kind_list
branch
from
5acb3f4 to
3691860
Compare
|
@fabianvf Any chance this could go to 0.8.x? |
5 participants
Previously when creating a resource list, kind was a dictionary meaning
that there could be only one resource for specific kind. But for example
Template kind can have two different resources:
So The client previsously found just one resource for kind Template.
In order to work with both resources for that kind this patch introduces
the change that kind will be list by default.