Skip to content
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

Qhughes/adding scp support #8

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

quincy2112
Copy link
Contributor

Client now supports copying files via scp to and from instance

@quincy2112 quincy2112 requested a review from ohinds July 20, 2020 22:03
Copy link

@ohinds ohinds left a comment

Choose a reason for hiding this comment

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

This looks really good! I know some of these are picky, sorry about that :P Very close though, I think we can get this merged soon.

@@ -66,6 +66,42 @@ def args_instance_connect(parser):
parser.add_argument( "instance", type=lambda name: query("Instance", "instance", name),)
parser.add_argument('connection_args', nargs=argparse.REMAINDER)

def args_instance_scp(parser):
Copy link

Choose a reason for hiding this comment

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

I think put might be better here, as it's consistent with the get below.

Copy link

Choose a reason for hiding this comment

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

this is left over from the old ass ftp terminology. get and put.

instance.send_scp(source, destination, scp_args)

@f0cal.entrypoint(["farm", "instance", "get"], args=args_instance_scp)
def instance_get(parser, core, instance, source, scp_args, *args, **kwargs):
Copy link

Choose a reason for hiding this comment

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

I like the get terminology, but it seems like the args here should be args_instance_get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually removed args_instance_get, both get and put use args_instance_scp

@f0cal.entrypoint(["farm", "instance", "send"], args=args_instance_scp)
def instance_send(parser, core, instance, source, scp_args, *args, **kwargs):
if '-destination' in scp_args:
i = scp_args.index('-destination')
Copy link

Choose a reason for hiding this comment

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

I think we need two dashes, like --destination

destination = scp_args.pop(i + 1)
del scp_args[i]
else:
destination = "~/"
Copy link

Choose a reason for hiding this comment

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

Because it's not a string that will be printed for the user, these should be single quotes

def instance_send(parser, core, instance, source, scp_args, *args, **kwargs):
if '-destination' in scp_args:
i = scp_args.index('-destination')
if len(scp_args) == i+1:
Copy link

Choose a reason for hiding this comment

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

Need a space on both sides of + here

@@ -24,6 +24,7 @@ def connect(self, connection_type, connection_args):
if not _fn:
print(f'{connection_type} connections not supported')
exit(1)
print((connection_args))
Copy link

Choose a reason for hiding this comment

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

Leftover from debugging?

def send_scp(self, source, destination, send_args):
scp_bin = '/usr/bin/scp'
send_args = self._format_send_args(source, destination, send_args)
print('*'*80)
Copy link

Choose a reason for hiding this comment

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

Need a space on either side of the * here.

Copy link

Choose a reason for hiding this comment

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

Sorry, the second one.

Copy link

@ohinds ohinds left a comment

Choose a reason for hiding this comment

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

This looks great! Just one more suggestion.

print('Destination directory must follow -destination flag, or remove the -destination flag to scp into the home directory.')
i = scp_args.index('--destination')
if len(scp_args) == i + 1:
print('Destination directory must follow --destination flag, or remove the -destination flag to scp into the home directory.')
Copy link

Choose a reason for hiding this comment

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

Here update the second -destination to have two dashes, also?

Copy link

@ohinds ohinds left a comment

Choose a reason for hiding this comment

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

Looks great! Let's do a walkthrough the next time you are at standup.

@ohinds ohinds self-requested a review July 30, 2020 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants