This repository has been archived by the owner on Mar 3, 2023. It is now read-only.
Proper check of the existence of SSH tunnel attribute #1380
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
objmagic
changed the title
| @@ -122,7 +122,7 @@ def establish_ssh_tunnel(self): | |||
| return localport | |||
|
|
|||
| def terminate_ssh_tunnel(self): | |||
| if self.tunnel: | |||
| if hasattr(self, 'tunnel'): | |||
There was a problem hiding this comment.
What if it has the attribute but the value is None? Should we be doing this?
if hasattr(self, 'tunnel') and self.tunnel:
or is that redundant? Not sure if hasattr also covers self.tunnel not None
There was a problem hiding this comment.
Good question. Since self.tunnel is only set at this line and I tried a meaningless command to open a subprocess, the Popen still gave me back an object instead of a None, I think not checking None should be fine here.
There was a problem hiding this comment.
import subprocess
p = subprocess.Popen(['ssh', 'twitter.com']) # should failp is still an object instead of a None.
There was a problem hiding this comment.
but I think we can add this check just to be safe...
|
👍 once ci passes. |
|
@objmagic - it will be good to add the check as @billonahill suggested. |
|
It works! 👍 |
moomou
pushed a commit
to moomou/heron
that referenced
this pull request
Sep 15, 2016
nicknezis
pushed a commit
that referenced
this pull request
Sep 14, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The attribute
tunnelis only added to the (Zookeeper) StateManager object when socket connection cannot be established and SSH is used instead.When one is trying to shutdown the tracker, the (Zookeeper) state manager will be shutdown first. However, the original way of check if SSH tunnel has been establish is wrong. If one wants to check if the attribute
tunnelexists in the StateManager object, he should usehasattrinstead ofif self.tunnel. The latter one assumes the attributetunnelalready exists in the object.This PR should fix #1379