1
\$\begingroup\$

This is my second Ruby coding attempt. Also - this is my first OOP usage in Ruby.

I'm using if __FILE__ == $0 form here - same as it is in Python because it's really comfortably as for me. Is there is any objections/restrictions for such usage?

Any other tips/links about this code appreciated as well.

Script intended to create Consul raft-database backup before run destroy.rb from my previous review, or restore database after provisioning.

#!/usr/bin/env ruby

require 'net/ssh'

$me = File.basename(__FILE__)

class CheckIn

    USER = 'knife';
    RSA_KEY = 'ssh/id_rsa';

    def initialize(host)

        @@host = host;

        if File.exist?(RSA_KEY) then
            File.chmod(0600, RSA_KEY);
        else
            abort "[#{$me}] ERROR: no RSA file #{RSA_KEY} found. Exit.\n";
        end

    end
end


class BackupRestore < CheckIn

    def backup()

        _consul_raft_db = '/opt/consul/data/raft/raft.db'
        _root_backup_dir = '/root/vaul_devops_kdbx/backups'
        _raft_backup_file = "#{Time.now.strftime("%H_%M_%S_%d_%m_%y")}_bkp_raft.db"

        Net::SSH.start(@@host, USER, :keys => RSA_KEY) do |ssh|

            backuped = ssh.exec!("

            if test -e #{_consul_raft_db}; then
                echo \"\"
                echo \"OK: Consul DB found - #{_consul_raft_db}.\"
                echo \"\"
            else
                echo \"ERROR: no #{_consul_raft_db} file found. Exit.\"
                exit 1
            fi

            if sudo [ -d #{_root_backup_dir} ]; then
                echo \"OK: #{_root_backup_dir} found.\"
                echo \"\"
            else
                echo \"ERROR: no #{_root_backup_dir} directory found. Exit.\"
                exit 1
            fi

            if sudo cp #{_consul_raft_db} #{_root_backup_dir}/#{_raft_backup_file}; then
                echo \"OK: backup created:\"
                echo \"\"
                sudo ls -l #{_root_backup_dir}/#{_raft_backup_file}
                echo \"\"
                echo \"All backups present on the #{@@host} in the #{_root_backup_dir}:\"
                echo \"\"
                sudo ls -l #{_root_backup_dir}
                echo \"\"
            else
                echo \"\"
                echo \"ERROR: can not complete backup from #{_consul_raft_db} to #{_root_backup_dir}/#{_raft_backup_file}. Exit.\"
                exit 1
            fi

            ")

            puts backuped

        end
    end

    def restore()

        puts "\nRestore will be here\n\n"

    end

end


if __FILE__ == $0

    puts "\n[#{$me}] Consul backup/restore started.\n"

    if ARGV.length == 2 and (ARGV[1] == 'backup' or ARGV[1] == 'restore')
        _host = ARGV[0]
        _mode = ARGV[1]

        puts "\n[#{$me}] Working on the #{_host} in #{_mode} mode.";

        run = BackupRestore.new(_host)
        run.send(_mode)
    else
        abort "\n[#{$me}] ERROR: HOST must be specified as first argument and MODE (backup or restore) - as second one. Exit..\n"
    end

    puts "[#{$me}] Consul backup/restore finished.\n\n"

end

And it's execution - backup:

$ ./scripts/backup.rb setevoy.vault.local backup

[backup.rb] Consul backup/restore started.

[backup.rb] Working on the setevoy.vault.local in backup mode.

OK: Consul DB found - /opt/consul/data/raft/raft.db.

OK: /root/vaul_devops_kdbx/backups found.

OK: backup created:

-rw------- 1 root root 2097152 Feb 17 16:11 /root/vaul_devops_kdbx/backups/16_11_57_17_02_16_bkp_raft.db

All backups present on the setevoy.vault.local in the /root/vaul_devops_kdbx/backups:

total 20248
-rw------- 1 root root 1048576 Feb 17 14:21 14_21_50_17_02_16_bkp_raft.db
[...[
-rw------- 1 root root 2097152 Feb 17 16:11 16_11_57_17_02_16_bkp_raft.db

[backup.rb] Consul backup/restore finished.

and "restore":

$ ./scripts/backup.rb setevoy.vault.local restore

[backup.rb] Consul backup/restore started.

[backup.rb] Working on the setevoy.vault.local in restore mode.

Restore will be here

[backup.rb] Consul backup/restore finished.
\$\endgroup\$
  • \$\begingroup\$ Why the rush in accepting? I feel honored that you think nobody can do a better job than me, but rest assured: you are wrong ;-) \$\endgroup\$ – Jörg W Mittag Feb 17 '16 at 19:04
  • \$\begingroup\$ @JörgWMittag Just was fascinated with your answer :-) If the first point with ; - just stupid habit and few others - came here from my previous Python experience (not to big, although) - every other one really sobered me up. Will see, what others thinking :-) \$\endgroup\$ – setevoy Feb 17 '16 at 19:17
2
\$\begingroup\$

Unfortunately, I don't have time for a proper review, so I'll stick to the more obvious points:

  • Indentation in Ruby is 2 spaces, not 4.
  • Not only is your use of semicolons inconsistent (sometimes you use them, sometimes you don't), every single one of those you do have is superfluous. Leave them out. You can use either newline or semicolon as "statement" separator, using both is not necessary.
  • The same applies to your usage of the then keyword: sometimes you use it, sometimes you don't, and all uses are superfluous. Leave them out. You can use either the then keyword or a "statement" separator (newline or semicolon) to separate the condition from the then body, using both is not necessary.
  • There is a large number of superfluous newlines. You'd generally have one before a new class, module or method definition, but not within – if your method is so long that it needs to be broken down in sections, consider refactoring it.
  • You can save yourself a lot of trouble escaping all the double quotes in the giant string by using either some other delimiter (e.g. backuped = ssh.exec!(%Q[ … ]) or a heredoc.
  • Leave out empty parameter lists, e.g. def backup instead of def backup().
  • By convention, variable names that start with an underscore _ are used for variables that are ignored. So, you would really only use them for multiple assignment or unused block parameters. In fact, it is more than just a convention: Ruby will warn you about unused local variables, but these warnings are suppressed for variables starting with an underscore, because they are intended as "ignored" variables.
  • You should prefer public_send over send. The latter circumvents access protection, the former honors it.
  • Prefer &&/|| over and/or. The former actually have the precedence you would expect from them, the latter don't.
  • For the string inside the interpolation within your raft_backup_file string, you could use single quotes instead of double quotes. Personally, I always use single quotes unless I actually need to do interpolation or use escape sequences (or there is a single quote in the string). This makes it clear just when looking at the beginning of the string whether or not there is going to be interpolation.

Note that since you use a class variable to store part of your state (@@host), it is impossible to create more than one instance of your class, so it doesn't make sense to have a class at all.

#!/usr/bin/env ruby

require 'net/ssh'

$me = File.basename(__FILE__)

class CheckIn
  USER = 'knife'
  RSA_KEY = 'ssh/id_rsa'

  def initialize(host)
    @@host = host

    if File.exist?(RSA_KEY)
      File.chmod(0600, RSA_KEY)
    else
      abort "[#$me] ERROR: no RSA file #{RSA_KEY} found. Exit.\n"
    end
  end
end

class BackupRestore < CheckIn
  def backup
    consul_raft_db = '/opt/consul/data/raft/raft.db'
    root_backup_dir = '/root/vaul_devops_kdbx/backups'
    raft_backup_file = "#{Time.now.strftime('%H_%M_%S_%d_%m_%y')}_bkp_raft.db"

    Net::SSH.start(@@host, USER, keys: RSA_KEY) do |ssh|
      backuped = ssh.exec!(%Q[
        if test -e #{consul_raft_db}; then
            echo ""
            echo "OK: Consul DB found - #{consul_raft_db}."
            echo ""
        else
            echo "ERROR: no #{consul_raft_db} file found. Exit."
            exit 1
        fi

        if sudo [ -d #{root_backup_dir} ]; then
            echo "OK: #{root_backup_dir} found."
            echo ""
        else
            echo "ERROR: no #{root_backup_dir} directory found. Exit."
            exit 1
        fi

        if sudo cp #{consul_raft_db} #{root_backup_dir}/#{raft_backup_file}; then
            echo "OK: backup created:"
            echo ""
            sudo ls -l #{root_backup_dir}/#{raft_backup_file}
            echo ""
            echo "All backups present on the #@@host in the #{root_backup_dir}:"
            echo ""
            sudo ls -l #{root_backup_dir}
            echo ""
        else
            echo ""
            echo "ERROR: can not complete backup from #{consul_raft_db} to #{root_backup_dir}/#{raft_backup_file}. Exit."
            exit 1
        fi
      ])

      puts backuped
    end
  end

  def restore
    puts "\nRestore will be here\n\n"
  end
end

if __FILE__ == $0
  puts "\n[#$me] Consul backup/restore started.\n"

  if ARGV.length == 2 && (ARGV[1] == 'backup' || ARGV[1] == 'restore')
    host = ARGV[0]
    mode = ARGV[1]

    puts "\n[#$me] Working on the #{host} in #{mode} mode."

    run = BackupRestore.new(host)
    run.public_send(mode)
  else
    abort "\n[#$me] ERROR: HOST must be specified as first argument and MODE (backup or restore) - as second one. Exit..\n"
  end

  puts "[#$me] Consul backup/restore finished.\n\n"
end
\$\endgroup\$
  • 2
    \$\begingroup\$ Spot on, except: there's no improper quoting for that string. The syntax highlighting is not interpolation-aware, making it look broken. \$\endgroup\$ – Phrogz Feb 18 '16 at 6:53
  • 1
    \$\begingroup\$ Hah. You're right. Apparently, not only is SO's syntax highlighter crappy, my brain's is, too. I removed that section. \$\endgroup\$ – Jörg W Mittag Feb 18 '16 at 8:13
  • \$\begingroup\$ @JörgWMittag Just came to work and started to edit my script - thanks for your answer :-) One more Q, please: > You'd generally have one before a new class, module or method definition, but not within // why it looks so bad for you - add newlines before/after variables definitions, before/after loops, if/else, methods? As for me - it makes the code more readable... Correct me, please, if I'm wrong. \$\endgroup\$ – setevoy Feb 18 '16 at 10:21
  • 1
    \$\begingroup\$ @setevoy It's personal preference, but consider this: the more blank lines you have, the less code you can see on screen at once, and thus the less context you have in your vision. If a method is getting longer than a certain limit (which is not a hard number, but consider half a screen) you should probably break it up into smaller methods with logical and helpful names, and clear, targeted goals. \$\endgroup\$ – Phrogz Feb 18 '16 at 14:00
  • \$\begingroup\$ I said that it's a general rule, and as you can see in my answer, I did sprinkle some whitespace around for better readability. But other than that, what Phrogz said. \$\endgroup\$ – Jörg W Mittag Feb 18 '16 at 19:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.