current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I'm working on a script to backup S3 to GC everyday. I'm not sure if my script has any potential bug or error that might have destroyed anything from both sides? The data is quite sensitive and I don't want to mess it up. Anything is welcomed. 

#!/bin/sh

if hash aws 2>/dev/null; then
    echo 'awscli is installed.'
else
    echo 'Please install awscli by running sudo pip install awscli'
    exit
fi

if hash gsutil 2>/dev/null; then
    echo 'gsutil is installed.'
else
    echo 'Please install gsutil by running sudo pip install gsutil'
fi

if env | grep -q ^BACKUP_TO_EMAIL=
then
    echo "After backup is done email will be send to $BACKUP_TO_EMAIL"
else
    echo "BACKUP_TO_EMAIL is not set please set it"
    exit
fi

if env | grep -q ^S3_BUCKET=
then
    echo "Checking if $S3_BUCKET exists"
    if aws s3 ls "s3://$S3_BUCKET" 2>&1 | grep -q 'AllAccessDisabled'   
    then
        echo "bucket $S3_BUCKET doesn't exist please check again."
        exit
    fi
else
    echo 'S3_BUCKET is not set please set it'
    exit
fi

if env | grep -q ^GC_BUCKET=
then
    echo "Checking if $GC_BUCKET exists"
    if gsutil ls "gs://$GC_BUCKET" 2>&1 | grep -q 'AccessDeniedException'
    then
        echo "bucket $GC_BUCKET doesn't exist please check again."
        exit
    fi
else
    echo 'GC_BUCKET is not set please set it'
    exit
fi

echo "Backing up now..."
#`gsutil -m rsync -r s3://$S3_BUCKET gs://$GC_BUCKET`

echo "Creating new backup folder"
datestamp=$(date +%m%d%y)
mkdir $datestamp

echo "Downloading backup"
aws s3 sync "s3://$S3_BUCKET" $datestamp

echo "Compressing site backup"
tar -zcvf $datestamp.tar.gz $datestamp
file=$datestamp.tar.gz
rm -rf $datestamp

echo "Uploading to GC"
gsutil cp $file "gs://$GC_BUCKET"

echo "Deleting temporary files"
rm $file

echo "Sending email"
body="Backup is complete and the file is in gs://$GC_BUCKET/$file"

echo $body | mail $BACKUP_TO_EMAIL -s "S3 to GS backup"
share|improve this question

1 Answer 1

up vote 3 down vote accepted

Looks mostly pretty good.

When exiting with error, it's a good practice to specify a non-zero exit code, for example exit 1.

This looks odd:

if env | grep -q ^S3_BUCKET=
then
    echo "Checking if $S3_BUCKET exists"
    if aws s3 ls "s3://$S3_BUCKET" 2>&1 | grep -q 'AllAccessDisabled'

It's odd to check environment variables using env | grep. And in this example, it looks error-prone too, because S3_BUCKET might be defined empty, with no values.

I don't know aws, but I have a strong feeling that in case of S3_BUCKET= (empty) you would rather raise an error than run execute aws s3 ls "s3://".

I suggest to replace with a simple variable check:

if test "$S3_BUCKET"; then

In this code:

tar -zcvf $datestamp.tar.gz $datestamp
file=$datestamp.tar.gz

It would be better to set file first, and then use it in the tar

I like to avoid nested if statements when possible. For example here:

if env | grep -q ^S3_BUCKET=
then
    echo "Checking if $S3_BUCKET exists"
    if aws s3 ls "s3://$S3_BUCKET" 2>&1 | grep -q 'AllAccessDisabled'   
    then
        echo "bucket $S3_BUCKET doesn't exist please check again."
        exit
    fi
else
    echo 'S3_BUCKET is not set please set it'
    exit
fi

I would flatten by inverting the outer if:

if test ! "$S3_BUCKET"
then
    echo 'S3_BUCKET is not set please set it'
    exit 1
fi

echo "Checking if $S3_BUCKET exists"
if aws s3 ls "s3://$S3_BUCKET" 2>&1 | grep -q 'AllAccessDisabled'   
then
    echo "bucket $S3_BUCKET doesn't exist please check again."
    exit 1
fi
share|improve this answer
    
Can you explain a bit more what test ! "$S3_BUCKET" do? –  toy Jun 29 at 4:23
    
It will be true if the variable is undefined, or defined but empty. In bash an empty string is "false" –  janos♦ Jun 29 at 4:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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