8000 Add close method to websocket client by mbohlool · Pull Request #146 · kubernetes-client/python · GitHub
[go: up one dir, main page]

Skip to content

Add close method to websocket client #146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 28, 2017

Conversation

mbohlool
Copy link
Contributor

fixes #145

@mbohlool mbohlool added this to the v1.0 milestone Feb 27, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 27, 2017
Copy link
@mrmcmuffinz mrmcmuffinz left a comment

Choose a reason for hiding this comment

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

Just a few questions...

"""
close websocket connection.
"""
self._connected = False

Choose a reason for hiding this comment

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

Should we check that the socket is open before closing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

close method is safe to call even if the socket is closed.

Choose a reason for hiding this comment

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

Got it, thanks.

@@ -191,6 +191,14 @@ def run_forever(self, timeout=None):
while self.is_open():
self.update(timeout=None)

def close(self, **kwargs):

Choose a reason for hiding this comment

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

What are some of the variable arguments you envision being used here? Just trying to get a feel for this change...

Choose a reason for hiding this comment

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

Also, just out of curiosity, should we return True/False if we were able to close successfully the socket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at WebScoket close method for more information: https://github.com/websocket-client/websocket-client/blob/master/websocket/_core.py#L372
that method does not return any status.

Choose a reason for hiding this comment

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

Got it, thanks.

@mbohlool mbohlool merged commit dc35eee into kubernetes-client:master Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive Exec on WebSocket api Close() requirement
3 participants
0