-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
aztk/utils/ssh.py
Outdated
while True: | ||
time.sleep(1) | ||
except KeyboardInterrupt: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this prevent ctrl + d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it will just intercept it so the stacktrace isn't printed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment about it so we don't forget why its needed
aztk/utils/ssh.py
Outdated
@@ -39,6 +99,21 @@ def connect(hostname, | |||
return client | |||
|
|||
|
|||
def forward_ports(client, port_forward_list): | |||
threads = [] | |||
if port_forward_list: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe do
if not port_forward_list:
return []
to have less nesting
aztk/utils/ssh.py
Outdated
from . import helpers | ||
|
||
|
||
g_verbose = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.debug? and --verbose flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, since this is in the SDK, we don't current have a logger. Maybe that would be nice to have though.
allow_reuse_address = True | ||
|
||
# pylint: disable=no-member | ||
class Handler(SocketServer.BaseRequestHandler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloud you separate in another file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this belongs here since this is the ssh tunnel request handler and this file is meant to have the utilities necessary to make ssh connections and commands.
] | ||
plugin_ports.extend(ports) | ||
|
||
print("Press ctrl+c to exit...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't that be ctrl + d? or not as we are not giving shell access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't giving shell access, the process just hangs so the ports stay open until killed (with crl + c
).
log.info("\t%s", ssh_cmd) | ||
|
||
except batch_error.BatchErrorException as e: | ||
if e.error.code == "PoolNotFound": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have an enum(or const) for those error codes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, but I think that should probably come in a single PR for the entire repo.
Fix #296
To do:
--internal
flag