Conversation
| while True: | ||
| time.sleep(1) | ||
| except KeyboardInterrupt: | ||
| pass |
There was a problem hiding this comment.
doesn't this prevent ctrl + d
There was a problem hiding this comment.
no, it will just intercept it so the stacktrace isn't printed
There was a problem hiding this comment.
Can you add a comment about it so we don't forget why its needed
aztk/utils/ssh.py
Outdated
|
|
||
| def forward_ports(client, port_forward_list): | ||
| threads = [] | ||
| if port_forward_list: |
There was a problem hiding this comment.
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.
log.debug? and --verbose flag
There was a problem hiding this comment.
yeah that would be better
There was a problem hiding this comment.
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.
Cloud you separate in another file?
There was a problem hiding this comment.
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.
shouldn't that be ctrl + d? or not as we are not giving shell access
There was a problem hiding this comment.
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.
I think we should have an enum(or const) for those error codes
There was a problem hiding this comment.
agreed, but I think that should probably come in a single PR for the entire repo.
2 participants
Fix #296
To do:
--internalflag