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

rsync_project w/ ssh key #63

Closed
bitprophet opened this Issue Aug 19, 2011 · 13 comments

Comments

Projects
None yet
1 participant
@bitprophet
Member

bitprophet commented Aug 19, 2011

Description

I submitted a pull request via git hub, but figure this is a better place to discuss the feature/update.

I added in a conditional to contrib/project.py to check for a non-empty env.key_filename. If it had a filename present it then added this string:
"--rsh=='ssh -i %s'" % env.key_filename
To the options string.

I notice that the env.key_filename can be a list of key files. I didn't account for that in this patch, but I also couldn't think of a case where more than one key would be specified for an rsync.


Originally submitted by Morgan Goose (goosemo) on 2009-09-18 at 10:00am EDT


Closed as Done on 2009-12-13 at 03:51pm EST

@ghost ghost assigned bitprophet Aug 19, 2011

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


--- a/fabric/contrib/project.py
+++ b/fabric/contrib/project.py
@@ -69,6 +69,10 @@ def rsync_project(remote_dir, local_dir=None, exclude=(), delete=False,
         "extra"   : extra_opts
     }
     options = "%(delete)s%(exclude)s -pthrvz %(extra)s" % options_map
+
+    if env.key_filename:
+        options += "--rsh='ssh -i %s'" % env.key_filename
+
     # Get local directory
     if local_dir is None:
         local_dir = '../' + getcwd().split(sep)[-1]

on 2009-09-18 at 10:04am EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


--- a/fabric/contrib/project.py
+++ b/fabric/contrib/project.py
@@ -69,6 +69,10 @@ def rsync_project(remote_dir, local_dir=None, exclude=(), delete=False,
         "extra"   : extra_opts
     }
     options = "%(delete)s%(exclude)s -pthrvz %(extra)s" % options_map
+
+    if env.key_filename:
+        options += "--rsh='ssh -i %s'" % env.key_filename
+
     # Get local directory
     if local_dir is None:
         local_dir = '../' + getcwd().split(sep)[-1]

on 2009-09-18 at 10:04am EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Yea, the multiple keys thing is (IIRC) a Paramiko feature, in that it has logic which will try each key in the list until one works.

This isn't very common and obviously it's not going to apply to an external tool like rsync. I'll probably amend the patch to see if the value of env.key_filename is not a string, and print a warning or something.

Thanks for the patch! I don't use rsync_project much so it's good that someone who does use it is interested in updating the functionality.


on 2009-09-18 at 01:11pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Yea, the multiple keys thing is (IIRC) a Paramiko feature, in that it has logic which will try each key in the list until one works.

This isn't very common and obviously it's not going to apply to an external tool like rsync. I'll probably amend the patch to see if the value of env.key_filename is not a string, and print a warning or something.

Thanks for the patch! I don't use rsync_project much so it's good that someone who does use it is interested in updating the functionality.


on 2009-09-18 at 01:11pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


Since you said it was a Paramiko feature, I made the assumption that ssh can handle multiple -i flags, which in my test it was able to. So perhaps this would be better for a list case:
if env.key_filename:
options += "--rsh='ssh -i %s'" % env.key_filename

elif type(env.key_filename) == list:
    options += "--rsh='ssh -i %s'" % (" -i ".join(env.key_filename))

No problem adding this btw. I like your project, but have only just started using it on my most recent one, so as I find it needs things I need, I'll patch if I can.


on 2009-09-18 at 01:59pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


Since you said it was a Paramiko feature, I made the assumption that ssh can handle multiple -i flags, which in my test it was able to. So perhaps this would be better for a list case:
if env.key_filename:
options += "--rsh='ssh -i %s'" % env.key_filename

elif type(env.key_filename) == list:
    options += "--rsh='ssh -i %s'" % (" -i ".join(env.key_filename))

No problem adding this btw. I like your project, but have only just started using it on my most recent one, so as I find it needs things I need, I'll patch if I can.


on 2009-09-18 at 01:59pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


That's good to know, guess my assumption was off -- I thought the "try >1 key if given" was something extra Paramiko provided, not part of the general suite. Cool stuff.

So yea, your amended patch would probably be fine. I'll let you know when I get around to pulling it down from your Github fork.


on 2009-09-18 at 05:41pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


That's good to know, guess my assumption was off -- I thought the "try >1 key if given" was something extra Paramiko provided, not part of the general suite. Cool stuff.

So yea, your amended patch would probably be fine. I'll let you know when I get around to pulling it down from your Github fork.


on 2009-09-18 at 05:41pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


Ok I pushed my changes to the fork I made on github. I also resubmited the pull request, since I don't know if that pull request is for the repo's tip or the tip's commit # at the time of the request.

I ended up choosing to do:

if env.key_filename:
    if type(env.key_filename) == str:
        key_string = env.key_filename

    elif type(env.key_filename) == list:
        key_string = " -i ".join(env_key_filename)

    options += " --rsh='ssh -i %s'" % key_string

on 2009-09-18 at 09:56pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


Ok I pushed my changes to the fork I made on github. I also resubmited the pull request, since I don't know if that pull request is for the repo's tip or the tip's commit # at the time of the request.

I ended up choosing to do:

if env.key_filename:
    if type(env.key_filename) == str:
        key_string = env.key_filename

    elif type(env.key_filename) == list:
        key_string = " -i ".join(env_key_filename)

    options += " --rsh='ssh -i %s'" % key_string

on 2009-09-18 at 09:56pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Two notes:

  1. No need to resubmit multiple pull requests, especially if you have a ticket open here. One's fine, or even just pasting a link to your fork in the ticket -- all I really need is to know which repo to make a remote for :)
  2. I suck at typechecking myself (and honestly it's bad practice to even have to typecheck, but we're given no choice here as this is a Paramiko level decision) but I'm pretty sure we want to use isintance(env.key_filename, types.StringTypes), then drop the explicit list check in the elif. Thoughts?

on 2009-09-19 at 10:19am EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Two notes:

  1. No need to resubmit multiple pull requests, especially if you have a ticket open here. One's fine, or even just pasting a link to your fork in the ticket -- all I really need is to know which repo to make a remote for :)
  2. I suck at typechecking myself (and honestly it's bad practice to even have to typecheck, but we're given no choice here as this is a Paramiko level decision) but I'm pretty sure we want to use isintance(env.key_filename, types.StringTypes), then drop the explicit list check in the elif. Thoughts?

on 2009-09-19 at 10:19am EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


Looking into it more I think its really my fault that there is a string case. In testing the -i flag for fab, even a solitary key will result in env.key_filname being a list. So when I set it as a string manually in my fabfile I went against the convention. So I can really just probably check for a non-empty list and assume that it should be a list, and If we wanted to just make a string case and report a warning/error for that instance.


on 2009-09-19 at 01:41pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


Looking into it more I think its really my fault that there is a string case. In testing the -i flag for fab, even a solitary key will result in env.key_filname being a list. So when I set it as a string manually in my fabfile I went against the convention. So I can really just probably check for a non-empty list and assume that it should be a list, and If we wanted to just make a string case and report a warning/error for that instance.


on 2009-09-19 at 01:41pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


So I've tweaked the rsync_project code to match this last update, and also tried to address bug number 44 since it also deals with the --rsh flag. Its pushed to my github branch. A side effect to my updates being that that --rsh flag is now always specifying the port number, even when the default of 22 is being used.


on 2009-09-21 at 12:13pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


So I've tweaked the rsync_project code to match this last update, and also tried to address bug number 44 since it also deals with the --rsh flag. Its pushed to my github branch. A side effect to my updates being that that --rsh flag is now always specifying the port number, even when the default of 22 is being used.


on 2009-09-21 at 12:13pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


Let me know if I need to change anything more. I threw in a check for string types that'll raise an error if people attempt to set the key_filename var like I had as a plain string, instead of the expected list of strings.


on 2009-10-07 at 12:25pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


Let me know if I need to change anything more. I threw in a check for string types that'll raise an error if people attempt to set the key_filename var like I had as a plain string, instead of the expected list of strings.


on 2009-10-07 at 12:25pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Sorry I haven't had time to pull this in, what time I have had for Fab I've been putting into getting the docs rolled out.

Re: type checking: shouldn't a single string key_filename be valid? My understanding of Paramiko says it can be either a single string or a list of strings.


on 2009-10-07 at 01:24pm EDT

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Sorry I haven't had time to pull this in, what time I have had for Fab I've been putting into getting the docs rolled out.

Re: type checking: shouldn't a single string key_filename be valid? My understanding of Paramiko says it can be either a single string or a list of strings.


on 2009-10-07 at 01:24pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


In my first look through their docs/code I though I saw that they only used lists of string. They really seem to be able to use both, because they force the input into lists in paramiko/client.py:
if key_filename is None:
key_filenames = []
elif isinstance(key_filename, (str, unicode)):
key_filenames = [ key_filename ]
else:
key_filenames = key_filename

I can make my bit force a single string into a list len 1 as they have.


on 2009-10-07 at 02:02pm EDT

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


In my first look through their docs/code I though I saw that they only used lists of string. They really seem to be able to use both, because they force the input into lists in paramiko/client.py:
if key_filename is None:
key_filenames = []
elif isinstance(key_filename, (str, unicode)):
key_filenames = [ key_filename ]
else:
key_filenames = key_filename

I can make my bit force a single string into a list len 1 as they have.


on 2009-10-07 at 02:02pm EDT

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Morgan Goose (goosemo) posted:


submitted a pull request for this.


on 2009-12-11 at 10:59am EST

Member

bitprophet commented Aug 19, 2011

Morgan Goose (goosemo) posted:


submitted a pull request for this.


on 2009-12-11 at 10:59am EST

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 19, 2011

Member

Jeff Forcier (bitprophet) posted:


Applied in changeset commit:f27fb518b37d45e40de590f1a0e43b87dc015363.


on 2009-12-13 at 03:51pm EST

Member

bitprophet commented Aug 19, 2011

Jeff Forcier (bitprophet) posted:


Applied in changeset commit:f27fb518b37d45e40de590f1a0e43b87dc015363.


on 2009-12-13 at 03:51pm EST

@bitprophet bitprophet closed this Aug 19, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment