Add Websocket streaming support to base #33

mbohlool · 2017-09-19T03:11:22Z

as part of kubernetes-client/python#341 I moved the websocket support in the base here as well as adding a stream package to support websocket calls instead of hacking api_client.py. User facing change would be instead of calling api directly like this:

resp = api.connect_get_namespaced_pod_exec( name, 'default',
              command=exec_command,
              stderr=True, stdin=False,
              stdout=True, tty=False)

user need to add a stream before it (and import it of course):

resp = stream(api.connect_get_namespaced_pod_exec, name, 'default',
              command=exec_command,
              stderr=True, stdin=False,
              stdout=True, tty=False)

The actual change that make this happen will be send to the main repo but we need this here first.

Note to reviewers: stream/ws_client.py and stream/ws_client_test.py are just copied from main repo and do not need reviewing.

codecov-io · 2017-09-19T03:21:05Z

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #33   +/-   ##
=======================================
  Coverage   93.93%   93.93%           
=======================================
  Files          11       11           
  Lines         824      824           
=======================================
  Hits          774      774           
  Misses         50       50

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84d1284...86361f0. Read the comment docs.

pokoli · 2017-09-19T08:25:36Z

stream/stream.py

+    def _request(self, _, url, query_params=None, headers=None,
+                 post_params=None, body=None, _preload_content=True,
+                 _request_timeout=None):
+        #pylint: disable=unused-argument


Maybe it's worth a comment about the second argument and why it's unused.

pokoli · 2017-09-19T08:29:26Z

stream/stream.py

+    def __init__(self):
+        self._config = None
+
+    def _request(self, _, url, query_params=None, headers=None,


Is really required to make a class method? I'm wondering if it's possible to use an anonymous function inside the intercept_request_call

This will have the benefit that we don't need to store the config object as class property but can be referenced as variable directly. Something like:

def intercept_request_call(...): config = ... def request(...): ws_client.websocket_call(config, url, ...)

Indeed, if we follow this pattern the _streamer class can be also removed.

mbohlool · 2017-09-19T08:43:29Z

@pokoli comments addressed. PTAL.

pokoli · 2017-09-19T08:50:52Z

stream/__init__.py

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


I will prefer to use a relative import here to indicate that stream is from current pacakg (like we do for ws_client)

Make sense. Done.

pokoli · 2017-09-19T09:07:54Z

stream/stream.py

+            post_params=None, body=None, _preload_content=True,
+            _request_timeout=None):
+        #pylint: disable=unused-argument
+        # Not all of the arguments in request call is necessary for


After reading this comment I'm wondering if it won't be better to use **kwargs for the optional arguments and pass them directly to websocket_call

Of course the websocket_call must be adapted to define the default values for this parameters (so they are not required).

I think this will easy the maintenance of new parameters in websocket_call as it won't be required to be added here which is now.

Websocket call is not using all request arguments. The list need to be mutated to remove undefined variables or call to websocket_call will fail or they need to be added to websocket_call that will be unused there.

more context: this code is basically try to remove this manual change from api_client.py:

python-base/api_client.py

Line 360 in 168c5e7

return ws_client.websocket_call(self.config,

On python there is not problem to pass extra args or kwargs as far as the function accepts args or kwargs: See

def function(a=1, *args, **kwargs):
... print a
...
function(b=12)
1

So what I propose is not to pass all the parameters, but manage only args and kwargs.

Yes but I wasn't sure I need to change websocket parameters. But I did it because I have no strong opinion either way. The code is certainly simpler now. Please take another look.

pokoli · 2017-09-19T09:10:43Z

stream/stream.py

+            method, url, query_params=None, headers=None,
+            post_params=None, body=None, _preload_content=True,
+            _request_timeout=None):
+        #pylint: disable=unused-argument


Il will prefer to remove this pylint line. If method and url are required arguments it can be done using args.

def _intercept_request_call(*args, **kwargs): _, url = args[:2] ....

Note that the slicing is required in case additional arguments are passed.

Unused argument is not only method but other named arguments. I can use _ for method in the argument itself but other named argument (that I cannot remove or convert to kwarg as I explained in the next comment) will be unused.

mbohlool · 2017-09-20T18:24:03Z

@pokoli PTAL

pokoli · 2017-09-21T07:32:31Z

stream/ws_client.py

+    headers = kwargs.get("headers")
+
+    # Extract the command from the list of tuples
+    commands = None


can be written as comands = query_params.pop('command', None)

This is an already reviewed code. I will fix it anyway :) I am blocked on this. so I will create a new PR for improvements on ws_client.py and merge this one. I assume you are done with the other files?

query_params is a list of tuples. pop does not work the way you expected it to work. It is possible to probably make it work, but I would like to ask you to ignore ws_client* files as they are copied from main repo. If we need to improve them, we need to create a separate PR/issue for it.

@mbohlool yes done with other files. Sorry for the mess.

No mess. My main repo PR was blocked on this. I will send it your (and @dims) way.

pokoli · 2017-09-21T07:32:54Z

stream/ws_client.py

+            break
+
+    # drop command from query_params as we will be processing it separately
+    query_params = [(key, value) for key, value in query_params if


Not required if using pop

dims · 2017-09-21T20:27:21Z

LGTM, we could add a test for the new proposed api soon-ish

pokoli · 2017-09-22T09:15:07Z

LGTM

I don't think it's worth to add a test for the new API as it's simply a wrapper.

mbohlool force-pushed the websocket branch from 9cbeb69 to ba6e3ad Compare September 19, 2017 03:13

mbohlool assigned dims and pokoli Sep 19, 2017

pokoli reviewed Sep 19, 2017

View reviewed changes

mbohlool force-pushed the websocket branch from ba6e3ad to 7d315c4 Compare September 19, 2017 08:43

pokoli reviewed Sep 19, 2017

View reviewed changes

mbohlool force-pushed the websocket branch from 7d315c4 to 98b22ce Compare September 19, 2017 08:55

pokoli reviewed Sep 19, 2017

View reviewed changes

Add Websocket streaming support to base

86361f0

mbohlool force-pushed the websocket branch from 98b22ce to 86361f0 Compare September 20, 2017 18:22

pokoli reviewed Sep 21, 2017

View reviewed changes

pokoli approved these changes Sep 22, 2017

View reviewed changes

mbohlool merged commit 31e13b1 into kubernetes-client:master Sep 22, 2017

mbohlool mentioned this pull request Sep 22, 2017

Use stream module to call exec/attach calls kubernetes-client/python#344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Websocket streaming support to base #33

Add Websocket streaming support to base #33

mbohlool commented Sep 19, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading

pokoli Sep 19, 2017

mbohlool Sep 19, 2017

pokoli Sep 19, 2017

mbohlool Sep 19, 2017

mbohlool commented Sep 19, 2017

pokoli Sep 19, 2017

mbohlool Sep 19, 2017

pokoli Sep 19, 2017

mbohlool Sep 19, 2017 •

edited

Loading

mbohlool Sep 19, 2017

pokoli Sep 20, 2017

mbohlool Sep 20, 2017

pokoli Sep 19, 2017

mbohlool Sep 19, 2017 •

edited

Loading

mbohlool commented Sep 20, 2017

pokoli Sep 21, 2017

mbohlool Sep 21, 2017 •

edited

Loading

mbohlool Sep 21, 2017 •

edited

Loading

pokoli Sep 22, 2017

mbohlool Sep 22, 2017

pokoli Sep 21, 2017

dims commented Sep 21, 2017

pokoli commented Sep 22, 2017

Add Websocket streaming support to base #33

Add Websocket streaming support to base #33

Conversation

mbohlool commented Sep 19, 2017 • edited Loading

codecov-io commented Sep 19, 2017 • edited Loading

Codecov Report

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 Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool Sep 19, 2017 • 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

mbohlool Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

mbohlool commented Sep 20, 2017

Choose a reason for hiding this comment

mbohlool Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

mbohlool Sep 21, 2017 • 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

dims commented Sep 21, 2017

pokoli commented Sep 22, 2017

mbohlool commented Sep 19, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading

mbohlool Sep 19, 2017 •

edited

Loading

mbohlool Sep 19, 2017 •

edited

Loading

mbohlool Sep 21, 2017 •

edited

Loading

mbohlool Sep 21, 2017 •

edited

Loading