Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Change utility functions to use newly generated client #34

Merged
merged 1 commit into from
Oct 10, 2017
Merged

Conversation

mbohlool
Copy link
Contributor

@mbohlool mbohlool commented Oct 3, 2017

@mbohlool
Copy link
Contributor Author

mbohlool commented Oct 3, 2017

We can ignore CI failure on this PR as the upstream client is updated to this change in kubernetes-client/python#353 and if CI passes there, it would be good enough.

@mbohlool mbohlool force-pushed the b1 branch 2 times, most recently from 8b386bb to ca29238 Compare October 3, 2017 22:14
@mbohlool mbohlool changed the title Change utility functions to use newly generated client [WIP] Change utility functions to use newly generated client Oct 4, 2017
config = type.__call__(Configuration)
loader.load_and_set(config)
Configuration.set_default(config)
print("ZZZ1: %s" % config.api_key)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print statement should be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is still a Work In Progress :)

@@ -368,8 +372,8 @@ def new_client_from_config(
"""Loads configuration the same as load_kube_config but returns an ApiClient
to be used with any API object. This will allow the caller to concurrently
talk with multiple clusters."""
client_config = ConfigurationObject()
client_config = type.__call__(Configuration)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the call required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a fresh configuration object not the default one. With the change in Configuration class in swagger-codegen (that we did), creating a configuration object will give you the default object that has been set before. But here if we get that, we would have mixed old/new configuration. We rather want a fresh Configuration object with __init__ default values not the set_default 'ed object.

header.append("Sec-WebSocket-Protocol: %s" %
configuration.ws_streaming_protocol)
if headers and 'sec-websocket-protocol' in headers:
header.append("sec-websocket-protocol: %s" % headers['authorization'])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not autorization but 'sec-websocket-protocol'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it can be use get with default value:

headers.get('sec-websocket-procotol', 'v4.channel.k8s.io')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't help much because if headers is None we still want a default value.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mehdy, we are checking for

'sec-websocket-protocol' in headers

BUT then using on line 52

headers['authorization']

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 52 should be

header.append("sec-websocket-protocol: %s" % headers['sec-websocket-protocol'])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Fixed.

@mbohlool mbohlool changed the title [WIP] Change utility functions to use newly generated client Change utility functions to use newly generated client Oct 6, 2017
@mbohlool
Copy link
Contributor Author

mbohlool commented Oct 6, 2017

Now this is ready to review.

@mbohlool
Copy link
Contributor Author

mbohlool commented Oct 9, 2017

@pokoli @dims Can you help reviewing this PR? Please ignore CI failure. That won't fix until I submit this and merge kubernetes-client/python#353

Copy link

@dims dims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see inline request

@dims
Copy link

dims commented Oct 9, 2017

LGTM 👍

@mbohlool mbohlool merged commit a41c447 into master Oct 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants