I'm new to bash scripting. I have pieced together a bash script to rsync my website files and databases.

Any guidance on ways I could improve or make my script more secure would be greatly appreciated.

#!/bin/bash

set -e

site_host=(
 "test1@grid.co.uk"
 "test2@grid.co.uk"
)

backup_dest=(
 "/Users/computername/Desktop/rsync/site1.co.uk"
 "/Users/computername/Desktop/rsync/site2.co.uk"
)

now=`date "+%d/%m/%Y %H:%M:%S"`
today=`date +"%d-%m-%Y"`
yesterday=`date -v -1d +"%d-%m-%Y"`
backup_old=`date -v -2d +"%d-%m-%Y"`

log="/Users/computername/Desktop/rsync/rsync.log"

site_count=$#site_host[@]

db_creds=()
db_creds+=("")
db_creds+=("localhost,user,pass,dbname")

for (( i = 0; i < site_count; i++ )); do

 # Source and destination
 site_source="$site_host[$i]:~/public_html"
 site_dest="$backup_dest[$i]/$today/"

 # DB creds
 for val in $db_creds[$i]; do
 subset=($(echo $val | tr ',' ' '))
 db_host=$subset[0]
 db_user=$subset[1]
 db_pass=$subset[2]
 db_name=$subset[3]
 done

 if [ "$db_host" ]; then

 # Set DB seperate directory
 mkdir -p $backup_dest[$i]/$today/db

 # Database backup
 ssh -p22 $site_host[$i] "mysqldump 
 --host=$db_host 
 --user=$db_user 
 --password=$db_pass 
 --databases $db_name 
 --lock-tables 
 | bzip2" > $backup_dest[$i]/$today/db/$today.sql.bz2

 echo "$now - SUCCESS - DB Backup - $backup_dest[$i]/$today/db/$today.sql.bz2" >> $log 

 fi

 # Remove old backups 
 if [ -d "$backup_dest[$i]/$backup_old" ]; then 

 rm -Rf $backup_dest[$i]/$backup_old; 

 echo "$now - DELETED - Full Backup - $backup_dest[$i]/$backup_old" >> $log 

 fi

 if rsync -zavx -e 'ssh -p22' 
 --numeric-ids 
 --delete -r 
 --link-dest=../$yesterday $site_source $site_dest;
 then

 echo "$now - SUCCESS - File Backup - $backup_dest[$i]/$today" >> $log 

 else

 echo "$now - FAILED - File Backup - $backup_dest[$i]/$today" >> $log 

 fi

done

good

• yay for specifying bash in your #! line.


• nice indentation of loops and conditionals


• good variables names


• Thanks for splitting long commands onto multiple lines, which are also nicely indented. (Another way to to do this is to built up the command options in a variable and then pass that as the command argument.)

suggestions

• I'm a big fan of https://www.shellcheck.net/ which is basically a linter for shell scripts. Don't worry about fixing all of its suggestions, but most of them are worth considering. My next few suggestions are based on its output.


• Using backticks (`) for command substitution is not the current best practice. Using $() for command substitution is nicer since it matches the () used for subshells and it stands out a bit more. Lots of folks were confused by quotes being used for non-quoting things. SC2006 mentions a few other reasons.


• When you do mkdir -p $backup_dest[$i]/$today/db on line 46, if one of the backup_dest had spaces in it you'd end up making two directories instead of one. It is best when doing variable substitutions in the shell to enclose them in double quotes. mkdir -p "$backup_dest[$i]/$today/db" is safer. SC2086 explains more.


• Are you expecting more db_creds in the future? The for loop at line 35 seems superfluous. Even if you had 99 entries in db_creds it is only going to set the db_* variables for the last one. If you want to do something for each one you'd have to put more of the work inside the loop. Or am I missing some intent here?


• checking to make sure you got a db_host in line 43 is a good idea, but should there be an else do this that warns about it being missing?


• does it matter if any of the stuff before the rsync fails? Do you want to exit with FAILED for anything before the rsync?


• putting a bit more documentation in the code would be better, but the comments that are there are good.

Overall, an excellent job for someone new to shell scripting.

As the other review pointed out, nicely done! I have a few minor things to add on top.

It seems that site_host, backup_dest and db_creds are closely related:
they will always have the same number of entries,
and the i-th values of these arrays are treated together.
db_creds is defined a bit further away from the other two.
It would be better if it was right next to the others.
Note that you can write db_creds in the same style as the others:

db_creds=(
 ""
 "localhost,user,pass,dbname"
)

The value $now is used at multiple places but does not actually represent "now". It is set once at the beginning of the script.
If you really wanted to use "now" in the log messages,
you could use a function instead:

now() 
 date "+%d/%m/%Y %H:%M:%S"


echo "$(now) - SUCCESS - DB Backup - ..."

The echo $val | tr ',' ' ' is not necessary to split the value.
It would be more efficient to use substitution:

subset=($val//,/ /)

Or, alternatively, you could define the values directly with spaces instead of commas:

db_creds=(
 ""
 "localhost user pass dbname"
)

subset=($val)