Skip to content
This repository has been archived by the owner on Feb 3, 2021. It is now read-only.

Feature: pure python ssh #577

Merged
merged 18 commits into from
Jun 5, 2018
Merged

Conversation

jafreck
Copy link
Member

@jafreck jafreck commented May 24, 2018

Fix #296

To do:

  • Add support for --internal flag

while True:
time.sleep(1)
except KeyboardInterrupt:
pass
Copy link
Member

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

Copy link
Member Author

@jafreck jafreck May 30, 2018

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

Copy link
Member

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

@@ -39,6 +99,21 @@ def connect(hostname,
return client


def forward_ports(client, port_forward_list):
threads = []
if port_forward_list:
Copy link
Member

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

from . import helpers


g_verbose = False
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

@timotheeguerin timotheeguerin May 30, 2018

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

Copy link
Member Author

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

Copy link
Member Author

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):
Copy link
Member

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?

Copy link
Member Author

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.

@jafreck jafreck removed the needs docs label Jun 4, 2018
]
plugin_ports.extend(ports)

print("Press ctrl+c to exit...")
Copy link
Member

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

Copy link
Member Author

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":
Copy link
Member

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

Copy link
Member Author

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.

@jafreck jafreck merged commit f16aac0 into Azure:master Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants