SSH Class with a method for downloading via SCP

Question

I am looking for a code review on this class. I found myself writing this logic a lot, so trying to save myself some time.

require 'etc'
require 'net/ssh'
require 'net/scp'
class SSHOps
 def initialize(host, user=Etc.getlogin, *args)
 @host = host
 @user = user
 @keys = args.empty? \
 ? args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa' \
 : args
 end
 ##
 # Download a file over SCP.
 #
 # ==== Attributes
 #
 # * +hostname+ - Target hostname to download the file from.
 # * +filename+ - Filename to download.
 def download_file(remote_path, local_path)
 ssh = Net::SSH.start(@host, @user, :keys => @keys)
 ssh.scp.download(remote_path, local_path, :preserve => true)
 ssh.close
 end
end

Answer 1

Some comments:

• def initialize(host, user=Etc.getlogin, *args). Ugly signature. I'd write def initialize(host, options = {})
• I had never seen that multiline way of writing a conditional. It does not look too good...
• Use File::join instead of joining paths by hand.
• The arguments in the documentation are wrong.
• args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'. It's not considered good practice to update an argument.

I'd write:

require 'etc'
require 'net/ssh'
require 'net/scp'
class SSHOps
 def initialize(host, options = {})
 @host = host
 @user = options[:user] || Etc.getlogin
 id_rsa_path = File.join(Etc.getpwnam(@user).dir, ".ssh/id_rsa")
 @keys = options[:keys].empty? ? [id_rsa_path] : options[:keys]
 end
 def download_file(remote_path, local_path)
 ssh = Net::SSH.start(@host, @user, :keys => @keys)
 ssh.scp.download(remote_path, local_path, :preserve => true)
 ssh.close
 end
end

Note that the library active_support has a nice abstraction for the pattern @keys = options[:keys].empty? ? [id_rsa_path] : options[:keys]: @keys = options[:keys].presence || [id_rsa_path].

• fr00z1: This also is great feedback. Thank you. I plan to take items from both the 1st, and 2nd review here.
• Flambino: +1 for mentioning the updating of arguments. I hadn't noticed, but of course it has side-effects
• fr00z1: QQ.. what is the rule of thumb of when to use options = {} vs. **kwargs, Your approach is definitely cleaner than my first one, but would definitely appreciate your guidance on this one. Thank you!
• fr00z1: Remarking this as the answer as I use more code from this response. Thank you again! Take care.
• Flambino: @fr00z My 2 cents: When a lot of your arguments are optional, they're, well, options! :) Besides, in your case, it'd be hard to leave out the user arg, but still pass the keys. You'd have to duplicate the logic in the method just to set user to the default value. If your default was nil it'd be a little easier, but still not great: new(host, nil, *keys). And mixing optional args and a * splat makes the signature even more cumbersome and potentially confusing.

Answer 2

Some notes:

• Your documentation doesn't match... well, it doesn't match anything in your code! Wrong documentation is the only thing worse than no documentation.

• A ternary should not span multiple lines. Thankfully, a regular if..else is also an expression in Ruby.

  @keys = if args.empty?
   Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'
  else
   args
  end
  

  While you're at it, don't name the argument args: Name it keys, since that's what it is.

• I haven't played around with Net::SCP, but if I'm reading the docs right, download will return immediately (i.e. before the file's been downloaded). And your next line closes the connection. As far as I can guess, you, in effect, just cancel all of your downloads as soon as you start them. What you want is probably download! (note the bang).

• Incidentally, it's perhaps overkill to make a class for this. Net::SCP already has a download! class method. If we rewrite your class with this, we get:

  class SSHOps
   def initialize(host, user=Etc.getlogin, *args)
   @host = host
   @user = user
   @keys = if args.empty?
   args << Etc.getpwnam(Etc.getlogin).dir + '/.ssh/id_rsa'
   else
   args
   end
   end
   def download_file(remote_path, local_path)
   options = {
   ssh: { keys: @keys },
   preserve: true
   } 
   Net::SCP.download!(@host, @user, remote_path, local_path, options)
   end
  end
  

  At this point, the class is only adding defaults for username and keys. That's it. So it might be simpler to write a few simple helper methods:

  module SSHOps
   module_function
   def default_key
   File.join(Etc.getpwnam(default_user).dir, ".ssh", "id_rsa")
   end
   def default_user
   Etc.getlogin
   end
   def download!(host, remote_file, local_file)
   options = {
   ssh: { keys: [default_key] }
   preserve: true
   }
   Net::SCP.download!(host, default_user, remote_file, local_file, options)
   end
  end
  

  Which you can then use like:

  SSHOps.download!("example.com", "/some/remote/file", "/some/local/file")
  

  I've intentionally left out some stuff. My hunch is that you almost always want to use your current username and keys, because that's the functionality that your current class adds. Anything else can basically be achieved just as simply with plain Net::SCP.download!, possibly using the two other helper methods in the module above.

• Of course, if you plan to download multiple files in a row, then it'd make sense to use a class if you at least reuse your SSH session rather than opening one for each download.

• fr00z1: Crud.. I forgot to change the documentation. I completely changed the method, so I will fix that.
• fr00z1: Also.. yes.. the goal is to download multiple files from a single host. :-) Thank you so much for the review.
• fr00z1: This would have definitely been the answer if I were using a module, and all your points were valid. Wish I could mark this as an answer as well. I use most of the answer below, but still took tidbits from your review. I appreciate the time you spent on this. Thank you again!
• Flambino: @fr00z No prob; I upvoted tokland's answer myself :)