-
Notifications
You must be signed in to change notification settings - Fork 140
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
Replicate base resource for lists functionality #89
Conversation
Codecov Report
@@ Coverage Diff @@
## main #89 +/- ##
=======================================
Coverage 24.02% 24.02%
=======================================
Files 1 1
Lines 154 154
Branches 29 29
=======================================
Hits 37 37
- Misses 111 112 +1
+ Partials 6 5 -1
Continue to review full report at Codecov.
|
@@ -9,4 +9,8 @@ molecule/default/roles/helm/files/appversionless-chart/templates/configmap.yaml | |||
molecule/default/roles/helm/files/test-chart-v2/templates/configmap.yaml yamllint!skip | |||
molecule/default/roles/helm/files/test-chart/templates/configmap.yaml yamllint!skip | |||
plugins/module_utils/k8sdynamicclient.py import-2.7!skip | |||
plugins/module_utils/k8sdynamicclient.py import-3.7!skip |
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 surprised with cannot import with Python 3.7. Do we use some python 3.8+ features?
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 think this is needed because of the bare imports in the new files. We had to do the same thing previously for the k8sdynamicclient.py.
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 would suggest adding some unit tests, what do you think about? https://github.com/openshift/openshift-restclient-python/blob/master/test/unit/test_discoverer.py
@alinabuzachis good suggestion. I had to make a few changes to get the unit tests to work. |
@@ -0,0 +1,49 @@ | |||
# Copyright [2017] [Red Hat, Inc.] |
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.
2021?
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.
To be honest, no idea. I'm going off what I think @Akasurde said in earlier PRs for this client migration work.
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.
Nothing really important, LGTM.
This replicates specific functionality from the openshift client to more reliably retrieve the base resource from a resource list.
SUMMARY
This replicates specific functionality from the openshift client to more
reliably retrieve the base resource from a resource list.
This addresses part of #34. The specific change to the underlying client being made is here: https://github.com/kubernetes-client/python-base/pull/186/files. The rest of the changes are made to deal with the inheritance chain and hardcoding of the class.
ISSUE TYPE
COMPONENT NAME
N/A
ADDITIONAL INFORMATION