Set Sec-WebSocket-Protocol header from cfg param

[jraby]

Follow up to:

• command exec: Adding support for streamProtocolV4 #297
• Add ws_streaming_protocol and use v4 by default python-base#20

(missing submodule bump)

[mbohlool]

you should set ws_streaming_protocol to None in configuration object (with a comment explaining what it is) and then here, check if it is not None, set the header.

I see your other PR now. But please check for None here.

[jraby]

I'll add the none check.
Are you ok with keeping the default value in configuration.py or would you rather have it here?

[mbohlool]

I think we should keep it in configuration.

[jraby]

Hmmm so read_all() returns ._all, which is a concat of all channels.
When using the V4 proto, the error channel always contains something, whether there's an error or not:

Success:

{"metadata":{},"status":"Success"}

Fail:

{
    "metadata": {},
    "status": "Failure",
    "message": "command terminated with non-zero exit code: Error executing in Docker Container: 4",
    "reason": "NonZeroExitCode",
    "details": {
        "causes": [
            {
                "reason": "ExitCode",
                "message": "4"
            }
        ]
    }
}

I guess I could exclude the error channel from the concatenation into _all, or maybe we should only concat the stdout and stderr channel?

That last option would keep compatibility with existing code, while future proofing it a bit. (I wouldn't be surprised if more channels are added to the proto).

What do you think?

[mbohlool]

I like the later option that keeps compatibility.

[mbohlool]

Tests are still failing, I guess this changed the behaviour of exec so exec tests need to be updated. Also need to make sure those behaviour changes are expected.

[jraby]

Yeah I'm looking into that, the test expects the websocket's is_open() function to return False, somehow my change broke that.

I'm having a hard time running the tests locally, is the required setup documented somewhere?

[mbohlool]

Run a minikube locally and also make sure you installed all test-requirements.txt as well as requirements.txt with pip install -r .... It is a good idea to document this somewhere.

[jraby]

Updated the test to show an example of how to get the status message on the error channel.

The test was failing because it expected the channel to be closed right after the 'exit', but now there's always a final status message that is sent down the error channel.

Since calling resp.update() reads only one message at the time, we need to call it twice:

• once to get the last status message
• once to trigger the EOF if clause.

[mbohlool]

LGTM. I can squash all commits into one and merge it or you can squash them all into one (or as many it make sense logically). Thanks for your contribution to kubernetes python client.

[jraby]

I think it should be good to go now.

[mbohlool]

This looks good but I am going to suggest a change. If you are not willing to do the change in this PR, I can merge it and send another PR for it.

Instead of configuration.ws_streaming_protocol can we use the headers passed to WSClient and check if Sec-WebSocket-Protocol exists on not? If it does not exists, set it to the default and remove the use of configuration.ws_streaming_protocol. Users can always set those default headers in api client. this will remove a generated file change (configuration). I am trying to remove all changes to configuration.py so we can use the auto-generated one.

[jraby]

I think your suggestion makes sense, however I will not have time to adjust the PR until next week.

[mbohlool]

That is OK. I will create an issue for these backward incompatible changes with swagger-codegen. If I didn't pick up the issue before next week, you can pick it up. For now, I will merge this.