Lookup plugin for the OpenShift Container Platform #31525
Conversation
ansibot
added
affects_2.5
bcoca
removed
the
needs_triage
|
@kevensen FYI, lookup plugins now support embedded docs, see any of the existing ones for examples |
ansibot
added
the
needs_revision
|
@bcoca Understood. I will take a look and add some documentation. |
|
The test |
|
@emonty sorry, i confuse my 'opens' .... stack!! not shift ... |
lib/ansible/plugins/lookup/oc.py
Outdated
| url += self.port | ||
| url += "/" | ||
| url += api | ||
| url += "/v1" |
There was a problem hiding this comment.
Much better to do this as url = "https://{0}:{1}/{2}/v1".format(self.host, self.port, api)
lib/ansible/plugins/lookup/oc.py
Outdated
| firstParam = True | ||
| self.kind = kind | ||
|
|
||
| url = self.kinds[self.kind]['baseurl'] |
There was a problem hiding this comment.
In building up url in this function, it would be more pythonic to add each portion to a list and then use join() at the end to turn the list into a string. Something like this:
url = [self.kinds[self.kind]['baseurl']]
if self.kinds[self.kind]['namespaced'] is True:
url.append('/namespaces/')
if namespace is None:
raise AnsibleError('Kind %s requires a namespace. \
None provided' % self.kind)
url.append(namespace)
[...]
return "".join(url)
lib/ansible/plugins/lookup/oc.py
Outdated
| "and message %s against url %s" % | ||
| str(response.code, response.read(), self.url())) | ||
|
|
||
| return json.loads(response.read()) |
There was a problem hiding this comment.
Does this work on python3? I can't remember for certain but you may need to convert from bytes type to text type:
return json.loads(to_text(response.read(), errors='surrogate_or_strict'))There was a problem hiding this comment.
According to the docs, it looks like json.loads still accepts a string in Python3.
lib/ansible/plugins/lookup/oc.py
Outdated
| if response.code >= 300: | ||
| raise AnsibleError("OC Query raised exception with code %s" + | ||
| "and message %s against url %s" % | ||
| str(response.code, response.read(), self.url())) |
There was a problem hiding this comment.
I think this str is in the wrong place (and the string addition is unneeded there might be a bytes type that you want to convert on python3). You probably need something more like:
raise AnsibleError("OC Query raised exception with code %s"
"and message %s against url %s" %
(response.code, to_native(response.read()), to_native(self.url()))There was a problem hiding this comment.
Also, I think self.url() should be url
lib/ansible/plugins/lookup/oc.py
Outdated
| from ansible.module_utils.six.moves.urllib.parse import urlencode | ||
|
|
||
|
|
||
| class OC(object): |
There was a problem hiding this comment.
Could you name this class a little more descriptively?
lib/ansible/plugins/lookup/oc.py
Outdated
| __metaclass__ = type | ||
|
|
||
| DOCUMENTATION = """ | ||
| lookup: ocp |
There was a problem hiding this comment.
documented name does not match lookup, in any case oc/ocp seem too short, something longer and more descriptive might help users find this plugin
There was a problem hiding this comment.
@bcoca Does ocp_query make sense? openshift_query? I'm open to suggestions.
There was a problem hiding this comment.
naming is hard ... i think openshift should be good enough and descriptive enough as a lookup
lib/ansible/plugins/lookup/oc.py
Outdated
| version_added: "2.5" | ||
| short_description: Returns the JSON definition of an object in OpenShift | ||
| description: | ||
| - The Ansible OC module performs Create, Update, and Delete operations for |
There was a problem hiding this comment.
this is not a module ... also lookups should not create/update/delete anything, just query
There was a problem hiding this comment.
Right. I was pointing out that the OC Ansible module does the Create, Update and Delete. This lookup does the Read for CRUD. If this is misleading or unnecessary I'll remove it.
lib/ansible/plugins/lookup/oc.py
Outdated
| bearer = "Bearer " + self.token | ||
| self.headers = {"Authorization": bearer} | ||
|
|
||
| def build_facts(self): |
There was a problem hiding this comment.
Is there a reason this isn't a part of (could be called from) __init__()? It seems to only be used right after the class is instantiated.
lib/ansible/plugins/lookup/oc.py
Outdated
|
|
||
| def url(self, kind=None, namespace=None, resource_name=None, pretty=False, labelSelector=None, fieldSelector=None, resourceVersion=None): | ||
| firstParam = True | ||
| self.kind = kind |
There was a problem hiding this comment.
This doesn't appear to need to be an attribute as it's only referenced inside of this method. So you can just use kind without assigning it to self.kind.
lib/ansible/plugins/lookup/oc.py
Outdated
| if response.code >= 300: | ||
| raise AnsibleError("OC Query raised exception with code %s" + | ||
| "and message %s against url %s" % | ||
| str(response.code, response.read(), self.url())) |
There was a problem hiding this comment.
Same notes here as for line 215.
|
The test |
|
I think you still need to fix json.loads() If you're new to python2 vs python3 programming you might have missed that "str" itself changed between python2 and python3. I believe that response.read() is returning a byte string (str type in python2, bytes type in python3) since it is a response from the network. (That's the part that I could be wrong about though.) |
| body = e.read() | ||
| except AttributeError: | ||
| body = '' | ||
| raise AnsibleError("OC Query raised exception with code {0} and message {1} against url {2}".format(e.code, body, url)) |
There was a problem hiding this comment.
Probably need to use to_native() (from ansible.modoule_utils._text) to make sure that body is a native string type on both python2 and python3.
| body = '' | ||
| raise AnsibleError("OC Query raised exception with code {0} and message {1} against url {2}".format(e.code, body, url)) | ||
|
|
||
| for resource in json.loads(response.read())['resources']: |
There was a problem hiding this comment.
you should use to_text() (also from ansible.module_utils._text) around response.read() here. Pretty sure that returns a byte string on both python2 and python3.
There was a problem hiding this comment.
Since this is setting data, you also want to use an error handler like this:
from ansible.module_utils._text import to_text
[...]
for resource in json.loads(to_text(response.read(), errors='surrogate_or_strict'))['resources']:The default error handler is replace which is good for things displayed to the user but not good for preserving data.
| url.append('/namespaces/') | ||
| if namespace is None: | ||
| raise AnsibleError('Kind %s requires a namespace. \ | ||
| None provided' % kind) |
There was a problem hiding this comment.
This is wrong. Do it like this:
raise AnsibleError('Kind %s requires a namespace.'
' None provided' % kind)
| body = '' | ||
| raise AnsibleError("OC Query raised exception with code {0} and message {1} against url {2}".format(e.code, body, url)) | ||
|
|
||
| return json.loads(response.read()) |
There was a problem hiding this comment.
Think you need to transform to text here as well.
| search_response['item_list'] = search_response.pop('items') | ||
|
|
||
| values = [] | ||
| values.append(search_response) |
There was a problem hiding this comment.
You could just do this as:
values = [search_response]
|
We're diagnosing the integration test failures now. We think it's that a new cryptography was just released which does not work with the pip installed on ubuntu-16.04. |
|
@abadger RE: integration tests, understood. I am new to Python2 vs. Python3. I've incorporated your changes and have tested them. I am seeing consistent, expected output so I'm comfortable using the to_native and to_text. Thanks for additional review. |
|
The test |
ansibot
removed
the
ci_verified
ansibot
added
stale_ci
|
Im already using this plugin, and would love to see it get merged. |
| version_added: "2.5" | ||
| short_description: Returns the JSON definition of an object in OpenShift | ||
| description: | ||
| - This lookup plugin provides the ability to query an OpenShift Container \ |
There was a problem hiding this comment.
description is a list, you can do list items, no need for \ continuations, im not even sure they'll display correctly
There was a problem hiding this comment.
@bcoca Understood. Think I've run into that problem before.
| a valid user or service account token. | ||
| options: | ||
| _terms: | ||
| description: |
There was a problem hiding this comment.
I'm not proficient in openshift nomeclature, but it seems that resource_name makes more sense as a list an object_type as a parameter? i.e i want to get data for project1 and project2 which are Project type ... this seems to be reversed to me. (_terms vs resource_name)
There was a problem hiding this comment.
or maybe you just don't accept a 'list of terms' but only named parameters, specially cause you only take 'the first term' in your code below vs looping over them.
There was a problem hiding this comment.
@bcoca Your second suggestion is most appropriate. I didn't know enough about "_terms" and used it probably inappropriately. I've removed the terms and name the parameters consistent with the OC plugin.
ansibot
removed
the
stale_review
ansibot
removed
the
stale_ci
|
@lucastheisen Just FYI, I've made some minor updates per bcoca. I've tested this and it works, but if you have time to give it another look, it would be appreciated. Specifically, the "resource kind" is a named parameter: kind. I've updated the module docs accordingly. |
ansibot
removed
the
needs_revision
|
@abadger I think this is pretty well polished. Did you have any remaining thoughts or comments you’d like me to address? |
It appears I am already using the most recent version from your repo: So, unless i am missing something here... werks fer me... |
SUMMARY
The OC plugin allows for the creation, deletion and modification of OpenShift resources in a cluster. There are use cases where one may wish to simply read resources from a cluster. This functionality could be built into the existing OC module. But, given the paradigm for Ansible, it seems more appropriate to have this read functionality in a lookup plugin.
ISSUE TYPE
COMPONENT NAME
plugins/lookup/openshift.py
ANSIBLE VERSION
ADDITIONAL INFORMATION
The following is an example of how this could be used.