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

Change utility functions to use newly generated client#34

Merged
mbohlool merged 1 commit into
masterfrom
b1
Oct 10, 2017
Merged

Change utility functions to use newly generated client#34
mbohlool merged 1 commit into
masterfrom
b1

Conversation

@mbohlool

@mbohlool mbohlool commented Oct 3, 2017

Copy link
Copy Markdown
Contributor

@mbohlool

mbohlool commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

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
Comment thread config/kube_config.py Outdated
config = type.__call__(Configuration)
loader.load_and_set(config)
Configuration.set_default(config)
print("ZZZ1: %s" % config.api_key)

Copy link
Copy Markdown

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
Copy Markdown
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 :)

Comment thread config/kube_config.py
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
Copy Markdown

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
Copy Markdown
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.

Comment thread stream/ws_client.py Outdated
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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

mbohlool commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

Now this is ready to review.

@mbohlool

mbohlool commented Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

@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

@dims dims left a comment

Copy link
Copy Markdown

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

dims commented Oct 9, 2017

Copy link
Copy Markdown

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