Authenticating to Amazon EC2 Container Registry

Question

New to bash. The script I wrote works, but I don't believe it is written in the best way possible.

The script only assesses how to execute the aws ecr get-login command depending on the parameters and versions of the aws-cli and docker currently installed on the host machine.

#!/usr/bin/env bash
set -e
declare aws_region="${AWS_REGION:?''}"
declare registry_id="${REGISTRY_ID:?''}"
declare docker_version=$(docker version -f {{.Client.Version}} | sed 's/[^0-9.]//g')
if ! [[ -z $aws_region ]]; then
 $aws_region = "--region $aws_region"
else
 echo "Need to set AWS_REGION (eg. us-west-2)"
fi
if ! [[ -z $registry_id ]]; then
 $registry_id = "--registy-id $registry_id"
fi
if [[ $docker_version < '17.06' ]]; then
 options = "--no-include_email"
fi
query="aws ecr get-login $aws_region $registry_id $options"
echo $query
$($query)
if ! [[ $? == 0 ]]; then
 echo "---ERROR! Please login to AWS with myob-auth or ADFS."
 exit 1
fi

Any ideas for improvements? I'm personally getting bad smells in the code from the 3 if statements and the way the $query variable works.

• Mathieu Guindon: A better post title would tell us about what "this bash script" does.

Answer 1

Syntax errors

$aws_region = "--region $aws_region"

This is not how you set a variable in Bash. The correct syntax is

aws_region="--region $aws_region"

Likewise for the later statements. Note that there are no spaces before and after the =, and you drop the sigil before the variable name when you're assigning to it.

Use the appropriate shell parameter expansion

If you consult the Shell Parameter Expansion section of the Bash manual, you'll see that

declare registry_id="${REGISTRY_ID:?''}"
if ! [[ -z $registry_id ]]; then
 $registry_id = "--registy-id $registry_id"
fi

can be expressed more succinctly as

declare registry_id=${REGISTRY_ID:+--registy-id "${REGISTRY_ID}"}

and similarly for aws_region.

Use debugging prints

If you don't like the way the $query variable works, you can just execute the command without assigning it to a variable and still have Bash print the executed command (to stderr) with set -x in a subshell environment.

(set -x; aws ecr get-login "$aws_region" "$registry_id" "$options")

Redirect error messages to stderr

It is customary to print error messages to stderr for easy filtering, so write the last echo as

>&2 echo "---ERROR! Please login to AWS with myob-auth or ADFS."

Other notes

• I personally prefer writing typeset over declare because the latter is a bashism whereas the former would also work in ksh, and it is the same amount of typing.

• Looking at the Docker Engine API, it seems that the Docker version follows the common major.minor.build-patch format, so I would choose the more light-weight and simpler in syntax cut over sed and move the self-evident version-getting command into the test conditional.

  if [[ $(docker version -f {{.Client.Version}} | cut -d'-' -f1) < 17.06 ]]
  then
   options="--no-include_email"
  fi
  

• Don't write if ! [[ $? == 0 ]] and if ! [[ -z $aws_region ]]. Write if [[ $? != 0 ]] and if [[ -n $aws_region ]]. Okay, more leeway to the second one because I believe fewer people know about test -n than test -z, but at least it is not an inferior option.

Answer 2

To start with, you might want to modularize the script. Put your if conditions in separate functions doing the corresponding task of getting AWS region, registry_id and options. Infact, you can put your functions in a separate file and include that in your bash script, if you see the use of those functions in scripts that you might write in future. You can do the processing for the specific fields by passing appropriate args to the function and returning the value. Use command substitution to get the response from the function for a clean processing.