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].

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.