Client enhancements #2

mbohlool · 2016-11-02T22:47:45Z

Adding kube config support (load_kube_config)
~~Adding SSL support (a fix for python 2.7 to accept ipaddress hosts)~~
Adding streaming watch support with test
Adding preprocessing step to enhance operation IDs
Regenerated client with enhanced swagger.json and various fixes in swagger-codegen [Python] Add support for request timeout swagger-api/swagger-codegen#4173 [Python] Support _preload_content flag in python client swagger-api/swagger-codegen#4166 [Python] Select application/json content-type in python generated client, if */* is in the list of content-types swagger-api/swagger-codegen#4152 and kubernetes fixes Update OpenAPI spec generator to enable all APIs kubernetes/kubernetes#36684
Adding examples
Removing generated tests

fixes #4

sebgoa · 2016-11-10T01:15:32Z

there is a requirement on pyyaml to be added, for the k8sutil to work on python 2.7

mbohlool · 2016-11-11T08:38:23Z

Good catch sebgoa. I will add it. I will update this PR with much better commits soon. Support for watch streaming response is among them.

mbohlool · 2016-11-15T00:38:11Z

@mml @smarterclayton @caesarxuchao Friendly ping.

caesarxuchao · 2016-11-15T05:28:29Z

First 3 commits lgtm.

caesarxuchao · 2016-11-15T18:53:50Z

The logic of the 4th commit makes sense. I'm not familiar with Python, so I can't give comments on the coding.

caesarxuchao · 2016-11-16T05:06:08Z

k8sutil/ssl.py

+    try:
+        # Divergence from upstream: ipaddress can't handle byte str
+        host_ip = _ip_address(hostname)
+        san = cert.get('subjectAltName', ())


What is ()?

Is my understanding correct: this commit is fixing match_hostname function of python version < v3, so that if the hostname is an IP address, it can still match.

() is the default value, it means an empty tuple will return if cert does not have 'subjectAltName'.

>>> x=() >>> type(x) <type 'tuple'> >>> len(x) 0 >>>

Your understanding is correct.

caesarxuchao · 2016-11-16T05:41:01Z

k8sutil/watch.py

+                        j = json.loads(l)
+                        j['raw_object'] = j['object']
+                        if return_type:
+                            obj = SimpleNamespace(data=json.dumps(j['raw_object']))


What does SimpleNamespace do?

ref: https://docs.python.org/3.5/library/types.html#types.SimpleNamespace

It is something that available in python3 not python2. it lets you have types with dynamic attributes. api_client.deserialize expect a type that has 'data' attribute.

Peter Norvig popularized this before py3 on his IAQ (as Struct):

http://norvig.com/python-iaq.html

Thanks for sharing Peter Norvig's IAQ. I enjoyed reading it. Though I like the name struct, I would keep SimpleNamespace for folks familiar with concept in py3.

sebgoa · 2016-11-16T07:41:00Z

Honestly this PR is so big with auto-generated code, you should just merge it and move on in master.

I think it is more important to setup build bots, pip package, proper README and a bit of doc.

Right now what is in master is broken.

marun

Mostly suggestions, but I have concerns about the proposed monkeypatch.

marun · 2016-11-16T06:47:12Z

k8sutil/watch.py

+                        j = json.loads(l)
+                        j['raw_object'] = j['object']
+                        if return_type:
+                            obj = SimpleNamespace(data=json.dumps(j['raw_object']))


Peter Norvig popularized this before py3 on his IAQ (as Struct):

http://norvig.com/python-iaq.html

marun · 2016-11-16T06:50:36Z

k8sutil/watch.py

+
+def _find_return_type(func):
+    for line in pydoc.getdoc(func).splitlines():
+        if line.startswith(":return:"):


Consider defining :return: as a constant and using a name that describes its purpose.

marun · 2016-11-16T06:51:23Z

k8sutil/watch.py

+            return_type = _find_return_type(func)
+            # Hacky assumption that watching `func` return TypeList in non-watch mode and an event with
+            # object of type in Type in watch mode.
+            if return_type.endswith("List"):


Consider defining "List" as a constant...

marun · 2016-11-16T06:51:44Z

k8sutil/watch.py

+            # Hacky assumption that watching `func` return TypeList in non-watch mode and an event with
+            # object of type in Type in watch mode.
+            if return_type.endswith("List"):
+                return_type = return_type[:len(return_type)-4]


...and using len(constant) instead of the magic number 4.

marun · 2016-11-16T06:54:03Z

k8sutil/watch.py

+                else:
+                    prev = ""
+                for l in lines:
+                    if l:


Consider avoiding unnecessary indentation:

if not l: continue

Not relevant anymore because of previous refactoring.

marun · 2016-11-16T07:32:39Z

k8sutil/kube_config.py

+        os.remove(f)
+
+
+def _create_temp_file_with_content(content):


Rather than managing many created tmp files, consider tracking a single directory created using mkdtemp and call mkstemp against that path.

Why that is be better? I mean I am just letting os do what is best. There is no requirement for these files to be in the same folder.

(No action required)

I use 'consider' to suggest a course of action, not demand a change. I think it's cleaner to isolate multiple files created by an application in a directory, but if you don't agree there's no need to change.

marun · 2016-11-16T07:35:40Z

k8sutil/kube_config.py

+        return _create_temp_file_with_content(o[data_key_name])
+    if file_key_name in o:
+        return o[file_key_name]
+    return None


Consider removing this line, since python returns None by default.

marun · 2016-11-16T07:36:22Z

k8sutil/kube_config.py

+    return name
+
+
+def _find_object_with_name(o, name, o_name):


Consider avoiding the use of single-character variables for anything but loop indexes. obj might be more appropriate here. Also, what is the import of name vs o_name? It's non-obvious to me, and likely to be non-obvious to future maintainers.

This whole function was obsolete. sorry for that. removed. (name was the value of attribute 'name' of the object, and o_name was a debug friendly name of the object such as 'user' or 'cluster'. the logic moved to _node.get_with_name)

marun · 2016-11-16T07:37:57Z

k8sutil/kube_config.py

+        with open(o[file_key_name], 'r') as f:
+            data = f.read()
+        return data
+    return None


marun · 2016-11-16T07:52:41Z

k8sutil/ssl.py

+        pass
+    return prev_match_hostname(cert, hostname)
+
+def fix_ssl_hosts_with_ipaddress():


Is this really the best way to solve this problem? Monkeypatches are risky even when the patch is in code intended to be consumed as an application (and therefore packaged with a known good version of the targeted dependency). Patching in a library is generally a bad idea because there is no guarantee that the consumer will have a compatible patch target, and bugs in the patch may prove difficult to track down.

If this is really the only option, consider adding an explicit warning that monkeypatching is being performed to the README to aid in troubleshooting.

I see the ugliness in what I am doing here and I completely agree with you that is not a clean fix. I spend some time on this. basically py2.7 does not support ipaddress hosts and the fix has been added here: https://bugs.python.org/issue23239 for python 3.x. I am open to suggestions here and I will keep looking for a better solution. For now I will go with your suggestion of adding a warning to README file.

Look like this have been fix in newer versions of urllib3. I will confirm that and remove this hack all-together in that case.

mbohlool · 2016-11-16T11:44:36Z

@sebgoa I would like to have at least one person with python background to sign off on this before merging. The auto-generated code can be ignored and the rest of the code should not be hard to review.

mbohlool · 2016-11-16T12:26:05Z

@marun Thanks for the review. I've applied or replied to your comments. Notably I've removed ssl hack and changed dependency to a newer version of urllib3 that fixes the ip address hosts. Please take another look.

DirectXMan12 · 2016-11-16T15:46:18Z

k8sutil/watch.py

+TYPE_LIST_SUFFIX = "List"
+
+
+class SimpleNamespace:


you can probably replace this with a named tuple (see collections.namedtuple), if you know what fields you need (and it looks like there's only one use below, so you probably can).

I think this will work with almost any class except object. even something like

class MyObject(object): pass resp = MyObject() resp.data='...' ... d=resp.data

will work. SimpleNamespace is the simplest solution here as it will accept the attribute dict as the only parameter without any other complication. collections.namedtuple can also work with passing more parameters at constructor and adding some confusion why we used a namedtuple. My understanding is SimpleNamespace is the standard way to do what I want to do in Py3 so I am still in favor of using it.

marun

Good work cleaning things up and removing the monkeypatch!

I think the testing could be better, but I'm really picky. I leave it to your discretion as to whether you want to follow through with my suggestions.

Otherwise, I agree with @sebgoa that this should merge and automated testing should be the next step. Reviewing will only get us so far when generated code is such a substantial component.

marun · 2016-11-16T18:38:49Z

k8sutil/watch_test.py

+
+import unittest
+from .watch import Watch
+from mock import Mock


(No action required)

Regarding my now-lost comment regarding the wisdom of mocking (thanks github), I think that having to use mocks to test code that one has written is often an indication of poorly-factored code. Mocks have their uses when one has to validate behavior against external libraries, but most of the code here is under your control and could be structured to be tested largely without mocks.

Here's a good overview of the philosophy I'm espousing: Stop Mocking, Start Testing

I've suggested some additional refactors of watch.py that would make for simpler testing. Ideally all the lower-level functionality could be tested in isolation, and then a single high-level test (similar in scope to one of the tests here, and using mocks) could ensure that the pieces tie together as intended.

Thanks for the video. It was useful. I agree that complicated Mocks will make things harder to follow and keeping test/mock in sync with code would be harder. My point here is the Mock is a simple request mock and does not have much logic. Maybe if I refactor the test itself, it would be more clear. Nevertheless, you refactoring suggestions are good and I am going to apply them as much as possible.

marun · 2016-11-16T18:39:45Z

k8sutil/watch_test.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest


(No action required)

Consider ordering imports alphabetically. Not sure if there is a policy on this for kube, its just a general convention.

Added a semi-automatic script to do these things.

marun · 2016-11-16T18:42:43Z

k8sutil/watch.py

+        self._stop = True
+
+    def stream(self, func, *args, **kwargs):
+        """Watch an API resource and stream the result back as a generators.


nit: as a generators -> via a generator

marun · 2016-11-16T18:45:37Z

k8sutil/kube_config.py

+        os.remove(f)
+
+
+def _create_temp_file_with_content(content):


(No action required)

I use 'consider' to suggest a course of action, not demand a change. I think it's cleaner to isolate multiple files created by an application in a directory, but if you don't agree there's no need to change.

marun · 2016-11-16T18:46:16Z

k8sutil/kube_config.py

@@ -0,0 +1,148 @@
+# Copyright 2016 The Kubernetes Authors.


Consider writing tests for this module.

marun · 2016-11-16T18:53:32Z

k8sutil/watch.py

+                    watch.stop()
+        """
+
+        if self._return_type:


(No action required)

Lines 90-95 could be factored out into a method (e.g. get_return_type) and unit tested independently.

marun · 2016-11-16T19:22:40Z

k8sutil/watch.py

+            for l in iter_resp_lines(resp):
+                j = json.loads(l)
+                j['raw_object'] = j['object']
+                if return_type:


(No action required)

Consider factoring out the marshaling so it can be isolated for testing, e.g.

def marshall_obj(data, api_client, return_type): j = json.loads(data) j['raw_object'] = j['object'] if return_type: ... return j def wait(...): ... try: for l in iter_resp_lines(resp): obj = marshall_obj(l, api_client, return_type) yield obj ...

…k with ipaddress

mbohlool · 2016-11-17T00:19:49Z

I've applied/replied to all comments and will merge this.

Thanks @marun, @sebgoa, @DirectXMan12, @caesarxuchao for review.

Remove loop symlinks

mbohlool force-pushed the o2 branch 3 times, most recently from 099def5 to 6b15ae9 Compare November 2, 2016 23:10

mbohlool assigned smarterclayton, caesarxuchao and mml Nov 2, 2016

mbohlool force-pushed the o2 branch 2 times, most recently from 7440d2f to e73968e Compare November 4, 2016 17:51

mbohlool mentioned this pull request Nov 7, 2016

PR from original repo #3

Closed

mbohlool force-pushed the o2 branch from 5eee52f to 8405120 Compare November 11, 2016 11:42

mbohlool changed the title ~~Add SSL and kube config support and examples. Remove unnecessary tests~~ Client first real enhancements Nov 11, 2016

mbohlool changed the title ~~Client first real enhancements~~ Client enhancements Nov 11, 2016

mbohlool force-pushed the o2 branch 2 times, most recently from ca25bfe to 9094775 Compare November 12, 2016 00:40

Remove empty generated tests

a37bc3a

mbohlool force-pushed the o2 branch from 693e675 to 7c8e2a9 Compare November 15, 2016 00:35

mbohlool force-pushed the o2 branch 2 times, most recently from 1a2ac90 to 52d481f Compare November 15, 2016 11:34

mbohlool force-pushed the o2 branch 2 times, most recently from c171fa4 to 51512ca Compare November 16, 2016 00:24

caesarxuchao reviewed Nov 16, 2016

View reviewed changes

marun reviewed Nov 16, 2016

View reviewed changes

mbohlool force-pushed the o2 branch from 0e1178f to 87f7c00 Compare November 16, 2016 12:24

mbohlool force-pushed the o2 branch 6 times, most recently from cada429 to 16ce003 Compare November 16, 2016 14:19

DirectXMan12 reviewed Nov 16, 2016

View reviewed changes

marun reviewed Nov 16, 2016

View reviewed changes

mbohlool added 9 commits November 16, 2016 16:16

Add generation preprocess step to make OperationIDs simpler

117cceb

Update generated client

634a1b9

Add kube config support

033e180

Add watch stream support

750c9ea

Add watch test

3ad3c5d

Add examples

db2f4fb

Add github repo information to the generator config

4e8cca0

Add dependency to a newer version of urllib3 that fixes ssl host chec…

98e675e

…k with ipaddress

Add semi-auto code formatting script

273684c

mbohlool force-pushed the o2 branch from 16ce003 to 273684c Compare November 17, 2016 00:17

mbohlool added kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 17, 2016

mbohlool added this to the v1.0 milestone Nov 17, 2016

mbohlool merged commit 62dc349 into master Nov 17, 2016

klarose mentioned this pull request Nov 28, 2018

CRD watch stream starts processing very old deleted resources #693

Closed

yliaog mentioned this pull request Jul 16, 2019

get all yamls based on a label selector #879

Closed

yliaog pushed a commit to yliaog/client-python that referenced this pull request Jan 8, 2022

Merge pull request kubernetes-client#2 from mbohlool/loopfix

7c8e59b

Remove loop symlinks

matthewfischl mentioned this pull request May 17, 2024

Port forward a service #2195

Closed

Client enhancements #2

Client enhancements #2

Conversation

mbohlool commented Nov 2, 2016 • edited Loading

sebgoa commented Nov 10, 2016

mbohlool commented Nov 11, 2016

mbohlool commented Nov 15, 2016

caesarxuchao commented Nov 15, 2016

caesarxuchao commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebgoa commented Nov 16, 2016

marun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Nov 16, 2016

mbohlool commented Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Nov 17, 2016

mbohlool commented Nov 2, 2016 •

edited

Loading

mbohlool Nov 16, 2016 •

edited

Loading

mbohlool Nov 16, 2016 •

edited

Loading

mbohlool commented Nov 16, 2016 •

edited

Loading

mbohlool Nov 16, 2016 •

edited

Loading

marun Nov 16, 2016 •

edited

Loading