Use urlparse to generate websocket url#247
Conversation
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import unittest |
There was a problem hiding this comment.
nit: add empty line above.
kubernetes/client/ws_client_test.py
Outdated
| @@ -0,0 +1,36 @@ | |||
| # Copyright 2016 The Kubernetes Authors. | |||
|
I will wait for @foklepoint to test this before merging. If you didn't change anything else in this PR, ignore "nit" comments. Thanks. |
| url = url.replace('https://', 'wss://') | ||
|
|
||
| # patch extra / | ||
| url = url.replace('//api', '/api') |
There was a problem hiding this comment.
Is this logic present in the new code? I can't find the context in the history as to why we need it
There was a problem hiding this comment.
No, it's not present on the new code, but I don't see why it should be required.
Do you have some sample url that uses this replace?
There was a problem hiding this comment.
Sorry, nope. But I would be in favor of removing it if we can't track down why it was added
|
@dims I think you added the original code. Can you answer the question? |
|
I am fine with the new code doesn't have the logic to replacing "//api" with "/api". I would say we can merge this (after rebase) and if @dims has objection we can always patch it. |
| if parsed_url.scheme == 'http': | ||
| parts[0] = 'ws' | ||
| else: | ||
| parts[0] = 'wss' |
There was a problem hiding this comment.
nit: should we also check if it is https, just to cover all cases.
|
I've fixed the comment and rebased it. Thank you all for your time. |
Add support for dryRun parameter
Fixes #246