5
\$\begingroup\$

This is a script that I've written for personal use, and to educate myself. The script is a bit comment heavy to help future me remember all the details & design choices made at the time of writing.

The use case is simply to synchronize/copy files to an AWS S3 bucket using AWS CLI. There's a separate CDK part of the project, which sets up the AWS infrastructure, but that isn't really relevant here. There are some configuration items in a properties file, which are read, and then the script checks whether everything is in place on the AWS end, and if so, it reads through a config folder structure which folders to backup, and how (include & exclude patterns in respective files).

Going with Bash instead of a basic shell script was a deliberate choice, since this wouldn't be run on any production server, and extreme portability wasn't the main point here.

Folder structure of the overall project is:

- aws-infra
-- [various things here, that are out of the scope of the question]
- config
-- backup
--- Documents
---- includes.txt
--- Pictures
---- includes.txt (example at the end)
---- excludes.txt (example at the end)
--- [more files/folders following the same structure]
-- configuration.properties
- scripts
-- sync.sh

Theoretically I could've just run aws s3 sync on the base path, but since it's a recursive command, and there are a lot (about 500k) of unnecessary files, it would take a lot of time to go through each of them separately.

#!/bin/bash

# Get the current directory where this file is, so that the script can
# called from other directories without breaking up.
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd)"

CONFIG_FOLDER="$DIR/../config"
PROP_FILE='configuration.properties'

# This is an associated array, i.e., keys can be strings or variables
# think Java HashMap or JavaScript Object
declare -A properties

# These are Bash arrays, i.e., with auto-numbered keys
# think Java or JavaScript array
declare -a includes excludes params

function loadProperties {
    local file="$CONFIG_FOLDER/$PROP_FILE"

    if [[ ! -f "$file" ]]; then
        echo "$PROP_FILE not found!"
        return 2
    fi

    while IFS='=' read -r origKey value; do
        local key="$origKey"
        # Replace all non-alphanumerical characters (except underscore)
        # with an underscore
        key="${key//[!a-zA-Z0-9_]/_}"

        if [[ "$origKey" == "#"* ]]; then
            local ignoreComments
        elif [[ -z "$key" ]]; then
            local emptyLine
        else
            properties["$key"]="$value"
        fi
    done < "$file"

    if [[ "${properties[debug]}" = true ]]; then
        declare -p properties
    fi
}

function getBucketName {
    # Declare inside a function automatically makes the variable a local
    # variable.
    declare -a params
    params+=(--name "${properties[bucket_parameter_name]}")
    params+=(--profile="${properties[aws_profile]}")

    # Get the bucket name from SSM Parameter Store, where it's stored.
    # Logic is:
    # 1) run the AWS CLI command
    # 2) grab 5th line from the output with sed
    # 3) grab the 2nd word of the line with awk
    # 4) substitute first all double quotes with empty string,
    #    and then all commas with empty string, using sed
    local bucketName=$(aws ssm get-parameter "${params[@]}" | \
                       sed -n '5p' | \
                       awk '{ print $2 }' | \
                       sed -e 's/"//g' -e 's/,//g')

    properties[s3_bucket]="$bucketName"
}

function checkBucket {
    declare -a params
    params+=(--bucket "${properties[s3_bucket]}")
    params+=(--profile="${properties[aws_profile]}")

    # Direct stderr to stdout by using 2>&1
    local bucketStatus=$(aws s3api head-bucket "${params[@]}" 2>&1)
    
    # The 'aws s3api head-bucket' returns an empty response, if
    # everything's ok or an error message, if something went wrong.
    if [[ -z "$bucketStatus" ]]; then
        echo "Bucket \"${properties[s3_bucket]}\" owned and exists";
        return 0
    elif echo "${bucketStatus}" | grep 'Invalid bucket name'; then
        return 1
    elif echo "${bucketStatus}" | grep 'Not Found'; then
        return 1
    elif echo "${bucketStatus}" | grep 'Forbidden'; then
        echo "Bucket exists but not owned"
        return 1
    elif echo "${bucketStatus}" | grep 'Bad Request'; then
        echo "Bucket name specified is less than 3 or greater than 63 characters"
        return 1
    else
        return 1
    fi
}

function create_params {
    local local_folder="$HOME/$1"
    local bucket_folder="s3://${properties[s3_bucket]}$local_folder"

    params+=("$local_folder" "$bucket_folder")

    if [[ ${excludes[@]} ]]; then
        params+=("${excludes[@]}")
    fi
    
    if [[ ${includes[@]} ]]; then
        params+=("${includes[@]}")
    fi

    params+=("--profile=${properties[aws_profile]}")

    if [[ "${properties[dryrun]}" = true ]]; then
        params+=(--dryrun)
    fi

    if [[ "${properties[debug]}" = true ]]; then
        declare -p params
    fi
}

# Sync is automatically recursive, and it can't be turned off. Sync
# checks whether any files have changed since latest upload, and knows
# to avoid uploading files, which are unchanged.
function sync {
    aws s3 sync "${params[@]}"
}

# Copy can be ran for individual files, and recursion can be avoided,
# when necessary. Copy doesn't check whether the file in source has
# changed since the last upload to target, but will always upload
# the files. Thus, use only when necessary to avoid sync.
function copy {
    local basePath="${params[0]}*"

    # Loop through files in given path.
    for file in $basePath; do
        # Check that file is not a folder or a symbolic link.
        if [[ ! -d "$file" && ! -L "$file" ]]; then
            # Remove first parameter, i.e., local folder, since with
            # copy, we need to specify individual files instead of the
            # base folder.
            unset params[0]
            aws s3 cp "$file" "${params[@]}"
        fi
    done
}

function process_patterns {
    # If second parameter is not defined, then pointless to even read
    # anything, since there's no guidance on what to do with the data.
    if [[ -z "$2" ]]; then
        return 1;
    fi

    # If the file defined in the first parameter exists, then loop
    # through its content line by line, and process it.
    if [[ -f "$1" ]]; then
        while read line; do
            if [[ $2 == "include" ]]; then
                includes+=(--include "$line")
            elif [[ $2 == "exclude" ]]; then
                excludes+=(--exclude "$line")
            fi
        done < $1
    fi
}

# Reset the variables used in global scope.
# To be called after each cycle of the main loop.
function reset {
    unset includes excludes params
}

# The "main loop" that goes through folders that need to be
# backed up.
function handleFolder {
    process_patterns "${1}/${properties[exclude_file_name]}" exclude
    process_patterns "${1}/${properties[include_file_name]}" include
    
    # Remove the beginning of the path until the last forward slash.
    create_params "${1##*/}"
    
    if [[ "$2" == "sync" ]]; then
        sync
    elif [[ "$2" == "copy" ]]; then
        copy
    else
        echo "Don't know what to do."
    fi

    reset
}

function usage {
    cat << EOF
Usage: ${0##*/} [-dDh]

    -d, --debug   enable debug mode
    -D, --dryrun  execute commands in dryrun mode, i.e., don't upload anything
    -h, --help    display this help and exit

EOF
}

while getopts ":dDh-:" option; do
    case "$option" in
        -)
            case "${OPTARG}" in
                debug)
                    properties[debug]=true
                    ;;
                dryrun)
                    properties[dryrun]=true
                    ;;
                help)
                    # Send output to stderr instead of stdout by
                    # using >&2.
                    usage >&2
                    exit 2
                    ;;
                *)
                    echo "Unknown option --$OPTARG" >&2
                    usage >&2
                    exit 2
                    ;;
            esac
            ;;
        d)
            properties[debug]=true
            ;;
        D) 
            properties[dryrun]=true
            ;;
        h)
            usage >&2
            exit 2
            ;;
        *)
            echo "Unknown option -$OPTARG" >&2
            usage >&2
            exit 2
            ;;
    esac
done

# set -x shows the actual commands executed by the script. Much better
# than trying to run echo or printf with each command separately.
if [[ "${properties[debug]}" = true ]]; then
    set -x
fi

loadProperties

# $? gives the return value of previous function call, non-zero value
# means that an error of some type occured
if [[ $? != 0 ]]; then
    exit
fi

getBucketName

if [[ $? != 0 ]]; then
    exit
fi

checkBucket

if [[ $? != 0 ]]; then
    exit
fi

# Add an asterisk in the end for the loop to work, i.e.,
# to loop through all files in the folder.
backup_config_path="$CONFIG_FOLDER/${properties[backup_folder]}*"

# Change shell options (shopt) to include filenames beginning with a
# dot in the file name expansion.
shopt -s dotglob

# Loop through files in given path, i.e., subfolders of home folder.
for folder in $backup_config_path; do
    # Check that file is a folder, and that it's not a symbolic link.
    if [[ -d "$folder" && ! -L "$folder" ]]; then
        handleFolder "$folder" "sync"
    fi
done

# Also include the files in home folder itself, but use copy to avoid
# recursion. Home folder & all subfolders contain over 500k files,
# and takes forever to go through them all with sync, even with an
# exclusion pattern.
# Remove the last character (asterisk) from the end of the config path.
handleFolder "${backup_config_path::-1}" "copy"

Properties file (SSM parameter name censored):

# AWS profile to be used
aws_profile=personal

# Bucket to sync files to
bucket_parameter_name=[my_ssm_parameter_name]

# Config folder where backup folders & files are found.
backup_folder=backup/

# Names of the files defining the include & exclude patterns for each folder.
include_file_name=includes.txt
exclude_file_name=excludes.txt

Example include file:

*.gif
*.jpg

Example exclude file to pair with above:

*

The script works, but I'm interested on how to improve it. For instance, the error handling feels a bit clumsy.

Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

Use case rather than an elif chain

In checkBucket we have:

    # The 'aws s3api head-bucket' returns an empty response, if
    # everything's ok or an error message, if something went wrong.
    if [[ -z "$bucketStatus" ]]; then
        echo "Bucket \"${properties[s3_bucket]}\" owned and exists";
        return 0
    elif echo "${bucketStatus}" | grep 'Invalid bucket name'; then
        return 1
    elif echo "${bucketStatus}" | grep 'Not Found'; then
        return 1
    elif echo "${bucketStatus}" | grep 'Forbidden'; then
        echo "Bucket exists but not owned"
        return 1
    elif echo "${bucketStatus}" | grep 'Bad Request'; then
        echo "Bucket name specified is less than 3 or greater than 63 characters"
        return 1
    else
        return 1
    fi

This kind of string matching is exactly what case is designed for, instead of repeatedly launching grep processes:

case "${bucketStatus}" in
    '')
        echo >&2 "Bucket \"${properties[s3_bucket]}\" owned and exists"
        return 0
        ;;
    *'Invalid bucket name'*|*'Not Found'*)
        # Why no diagnostic here?
        ;;
    *Forbidden*)
        echo >&2 "Bucket exists but not owned"
        ;;
    *'Bad Request'*)
        echo >&2 "Bucket name specified is less than 3 or greater than 63 characters"
        ;;
    # Why no diagnostic in the default case?
esac
return 1

Some of those * could be removed if we know that the message is at the beginning of the output.

The comment is worrying - a command can only output a string - the return status is a number. And shouldn't we be using that return status rather than the output to determine whether it was successful? And just let the command's error message go directly to the error stream (more informative than our own, especially in the "invalid name" or "not found" cases):

checkBucket() {
    aws s3api head-bucket \
        --bucket "${properties[s3_bucket]}" \
        --profile="${properties[aws_profile]}"
}

Use set -e instead of testing each command individually

Consider this sequence:

loadProperties

# $? gives the return value of previous function call, non-zero value
# means that an error of some type occured
if [[ $? != 0 ]]; then
    exit
fi

getBucketName

if [[ $? != 0 ]]; then
    exit
fi

checkBucket

if [[ $? != 0 ]]; then
    exit
fi

The comment is pointless, as every shell programmer knows what $? is.

We should simply use || exit to use the failing command's exit status for the whole program's result. Even better, we can have the shell do that for all our commands:

set -e
loadProperties
getBucketName
checkBucket

Much simpler!

Improve this answer
\$\endgroup\$
1
1
\$\begingroup\$

Exit scripts with a failure code

The intention is good here, unfortunately a bit flawed:

someCommand

if [[ $? != 0 ]]; then
    exit
fi

exit without arguments uses the exit code of the last command. In this example the last command is the [[ conditional, which is successful when exit is reached, so the script exits with 0, indicating success.

When exiting from a script when something went wrong, it's good to signal it to caller scripts by using a non-zero exit code. One possible fix is to use exit 1 in the above example. Alternatively:

someCommand || exit

Note that this will use the exit code of someCommand, since that's the last command that was executed. If you don't want that for some reason, you can pass an explicit exit code to exit.

Beware of correct double-quoting

In most of the scripts the variables are correctly double-quoted, nicely done!

This part is not safe:

local basePath="${params[0]}*"

# Loop through files in given path.
for file in $basePath; do

If ${params[0]} contains spaces, the above will not work. Even if that's not a real concern, it's good to make this safe anyway:

local basePath="${params[0]}"

# Loop through files in given path.
for file in "$basePath"*; do

That is, I removed the * from the first assignment, and used it in the loop instead, this way $basePath is correctly double-quoted, and the shell will correctly expand * in the loop statement.

The same goes for this bit:

# Add an asterisk in the end for the loop to work, i.e.,
# to loop through all files in the folder.
backup_config_path="$CONFIG_FOLDER/${properties[backup_folder]}*"

# Loop through files in given path, i.e., subfolders of home folder.
for folder in $backup_config_path; do

The correct, safe way is:

# Add an asterisk in the end for the loop to work, i.e.,
# to loop through all files in the folder.
backup_config_path="$CONFIG_FOLDER/${properties[backup_folder]}"

# Loop through files in given path, i.e., subfolders of home folder.
for folder in "$backup_config_path"*; do

And then the last line of the script should not delete the last character:

handleFolder "${backup_config_path::-1}" "copy"

Change to:

handleFolder "$backup_config_path" "copy"

Beware of modifying global variables

This function looks a bit suspicious:

function copy {
    local basePath="${params[0]}*"

    # Loop through files in given path.
    for file in $basePath; do
        # Check that file is not a folder or a symbolic link.
        if [[ ! -d "$file" && ! -L "$file" ]]; then
            # Remove first parameter, i.e., local folder, since with
            # copy, we need to specify individual files instead of the
            # base folder.
            unset params[0]
            aws s3 cp "$file" "${params[@]}"
        fi
    done
}

My concern is the unset params[0] in a loop. Running this repeatedly is not harmful, just a waste. My bigger concern is that params is a global variable, and it makes a reader wonder:

  • Won't this cause problems later in the script?
  • If it's safe to remove the first value, is it ever needed?
    • Well yes we see it's used at the top of the function. Now we are concerned that this function is not safe to call again until params[0] is restored first.

Instead of modifying the global variable, a safer option would be to make a local copy:

local cp_params=("${params[@]}")
unset cp_params[0]

And @TobySpeight had an even better idea of passing "${params[@]}" as arguments to this function call as copy "${params[@]}", and then use shift in the function:

local basePath=$1
shift

# ...
for file in "$basePath"*; do

    # ...
    aws s3 cp "$file" "$@"

Give descriptive names to parameters

In process_patterns and handleFolder, the parameters are referenced throughout the function body as $1 and $2. It would be good to assign them to local variables with descriptive names to make it easier to read.

Use consistent naming scheme

Some function names use camelCase (handleFolder) others use snake_case (process_patterns). It would be better to use a consistent naming scheme (whichever is fine).

Find substrings using Bash native features

In this code:

elif echo "${bucketStatus}" | grep 'Invalid bucket name'; then

The condition is a bit expensive, because it has to invoke a grep process. It's faster to use Bash native features:

elif [[ "${bucketStatus}" == *'Invalid bucket name'* ]]; then

Empty statements

I guess the purpose of the local ... statements here is just to make the syntax valid without doing anything:

if [[ "$origKey" == "#"* ]]; then
    local ignoreComments
elif [[ -z "$key" ]]; then
    local emptyLine
else
    properties["$key"]="$value"
fi

That's a creative solution, but an abuse of local. In situations like this you can use true or : as the statement:

if [[ "$origKey" == "#"* ]]; then
    # ignore comments
    :
elif [[ -z "$key" ]]; then
    # ignore empty lines
    :
else
    properties["$key"]="$value"
fi

Print error messages to stderr

When something goes wrong, some functions print error messages. It's a good idea to redirect error messages to stderr instead of stdout:

echo "$PROP_FILE not found!" >&2

In some places you did this, but not all.

Print usage help to stdout

The script prints the usage help on stderr, and exits with code 2. When the help is explicitly invoked by the user, it's customary to print it on stdout and exit with code 0.

Make error messages more user friendly

In particular this one:

if [[ ! -f "$file" ]]; then
    echo "$PROP_FILE not found!"
    return 2
fi

I think it would be better if the message included $file in it:

if [[ ! -f "$file" ]]; then
    echo "File does not exist: $file"
    return 2
fi

Consider the efficient ordering of loops

In this code:

while read line; do
    if [[ $2 == "include" ]]; then
        includes+=(--include "$line")
    elif [[ $2 == "exclude" ]]; then
        excludes+=(--exclude "$line")
    fi
done < $1

Instead of evaluating the conditions for every line in the input file, you could rearrange the statements to only evaluate once, and skip reading the file when it wouldn't match any condition:

if [[ $2 == "include" ]]; then
    while read line; do
        includes+=(--include "$line")
    done < "$1"
elif [[ $2 == "exclude" ]]; then
    while read line; do
        excludes+=(--exclude "$line")
    done < "$1"
fi

It's not a big issue though, so if you don't like repeating the loop, then the original version is fine too.

Combining sed and awk pipelines

Not an issue in this script because most command pipelines are not in a loop, but mentioning it here anyway for reference that this kind of pipeline is possible with fewer commands:

local bucketName=$(aws ssm get-parameter "${params[@]}" | \
                   sed -n '5p' | \
                   awk '{ print $2 }' | \
                   sed -e 's/"//g' -e 's/,//g')

Like this:

local bucketName=$(aws ssm get-parameter "${params[@]}" | \
                   awk 'NR == 5 { print $2 }' | \
                   tr -d '",')

That is, I combined the sed and the awk, and replaced the last sed with a lighter tr.

Using modern function declaration

This style of function declaration is old:

function getBucketName {
    ...
}

The recommended style is:

getBucketName() {
    ...
}
Improve this answer
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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