Substantive shell script cleanups

Running "shellcheck", and attempting to comply with the shell script
style guide, required hundreds of trivial changes, and a small number of
nontrivial ones.  For ease of review, this PR includes only the
nontrivial changes.

src/metrics_server/test_integration_action.sh

@@ -85,14 +85,14 @@ EOF
 
 echo "Connections request:"
 cat $CONNECTIONS_REQUEST
-curl -X POST -H "Content-Type: application/json" -d @$CONNECTIONS_REQUEST $METRICS_URL/connections && echo
+curl -X POST -H "Content-Type: application/json" -d @$CONNECTIONS_REQUEST $METRICS_URL/$CONNECTIONS_PATH && echo
 sleep 5
 bq --project_id $BIGQUERY_PROJECT --format json query --nouse_legacy_sql 'SELECT serverId, userId, bytesTransferred, countries FROM `'"$BIGQUERY_DATASET.$CONNECTIONS_TABLE"'` WHERE serverId = "'"$SERVER_ID"'" ORDER BY bytesTransferred DESC LIMIT 2' > $CONNECTIONS_RESPONSE
 diff $CONNECTIONS_RESPONSE $CONNECTIONS_EXPECTED_RESPONSE
 
 echo "Features request:"
 cat $FEATURES_REQUEST
-curl -X POST -H "Content-Type: application/json" -d @$FEATURES_REQUEST $METRICS_URL/features && echo
+curl -X POST -H "Content-Type: application/json" -d @$FEATURES_REQUEST $METRICS_URL/$FEATURES_PATH && echo
 sleep 5
 bq --project_id $BIGQUERY_PROJECT --format json query --nouse_legacy_sql 'SELECT serverId, serverVersion, dataLimit FROM `'"$BIGQUERY_DATASET.$FEATURES_TABLE"'` WHERE serverId = "'"$SERVER_ID"'" ORDER BY timestamp DESC LIMIT 1' > $FEATURES_RESPONSE
 diff $FEATURES_RESPONSE $FEATURES_EXPECTED_RESPONSE

src/server_manager/electron_app/build_action.sh

@@ -19,8 +19,6 @@
 readonly OUT_DIR=$BUILD_DIR/server_manager/electron_app
 rm -rf $OUT_DIR
 
-readonly NODE_MODULES_BIN_DIR=$ROOT_DIR/src/server_manager/node_modules/.bin
-
 # Build the Web App.
 do_action server_manager/web_app/build
 
@@ -30,7 +28,6 @@ do_action server_manager/web_app/build
 tsc -p src/server_manager/electron_app/tsconfig.json --outDir build/server_manager/electron_app/js
 
 # Assemble everything together.
-readonly MODULE_DIR=$(dirname $0)
 readonly STATIC_DIR=$OUT_DIR/static
 mkdir -p $STATIC_DIR
 mkdir -p $STATIC_DIR/server_manager

src/server_manager/install_scripts/do_install_server.sh

@@ -1,3 +1,5 @@
+#!/bin/bash
+#
 # Copyright 2018 The Outline Authors
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -34,11 +36,11 @@ export SHADOWBOX_DIR="${SHADOWBOX_DIR:-${HOME:-/root}/shadowbox}"
 mkdir -p $SHADOWBOX_DIR
 
 # Save output for debugging
-exec 2>&1 >$SHADOWBOX_DIR/install-shadowbox-output
+exec >$SHADOWBOX_DIR/install-shadowbox-output 2>&1
 
 # Initialize sentry log file.
 export SENTRY_LOG_FILE="$SHADOWBOX_DIR/sentry-log-file.txt"
-> $SENTRY_LOG_FILE
+true > $SENTRY_LOG_FILE
 function log_for_sentry() {
   echo [$(date "+%Y-%m-%d@%H:%M:%S")] "do_install_server.sh" "$@" >>$SENTRY_LOG_FILE
 }
@@ -147,13 +149,13 @@ export SB_PUBLIC_IP=$(cloud::public_ip)
 
 log_for_sentry "Initializing ACCESS_CONFIG"
 export ACCESS_CONFIG="$SHADOWBOX_DIR/access.txt"
-> $ACCESS_CONFIG
+true > $ACCESS_CONFIG
 
 # Set trap which publishes an error tag and sentry report only if there is an error.
 function finish {
   INSTALL_SERVER_EXIT_CODE=$?
   log_for_sentry "In EXIT trap, exit code $INSTALL_SERVER_EXIT_CODE"
-  if [[ -z $(grep apiUrl $ACCESS_CONFIG) ]] || [[ -z $(grep certSha256 $ACCESS_CONFIG) ]]; then
+  if ! ( grep -q apiUrl $ACCESS_CONFIG && grep -q certSha256 $ACCESS_CONFIG ); then
     echo "INSTALL_SCRIPT_FAILED: $INSTALL_SERVER_EXIT_CODE" | cloud::add_kv_tag "install-error"
     # Post error report to sentry.
     post_sentry_report

src/server_manager/install_scripts/install_server.sh

@@ -1,3 +1,5 @@
+#!/bin/bash
+#
 # Copyright 2018 The Outline Authors
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -65,18 +67,18 @@ function log_command() {
 function log_error() {
   local -r ERROR_TEXT="\033[0;31m"  # red
   local -r NO_COLOR="\033[0m"
-  printf "${ERROR_TEXT}${1}${NO_COLOR}\n"
+  echo -e "${ERROR_TEXT}$1${NO_COLOR}"
   echo "${1}" >> ${FULL_LOG}
 }
 
 # Pretty prints text to stdout, and also writes to sentry log file if set.
 function log_start_step() {
   log_for_sentry "$@"
-  str="> $@"
+  str="> $*"
   lineLength=47
   echo -n "$str"
-  numDots=$(expr $lineLength - ${#str} - 1)
-  if [[ $numDots > 0 ]]; then
+  numDots=$(( lineLength - ${#str} - 1 ))
+  if (( numDots > 0 )); then
     echo -n " "
     for i in $(seq 1 "$numDots"); do echo -n .; done
   fi
@@ -102,7 +104,7 @@ function confirm() {
   echo -n "> $1 [Y/n] "
   local RESPONSE
   read RESPONSE
-  RESPONSE=$(echo "$RESPONSE" | tr '[A-Z]' '[a-z]')
+  RESPONSE=$(echo "$RESPONSE" | tr '[:upper:]' '[:lower:]')
   if [[ -z "$RESPONSE" ]] || [[ "$RESPONSE" = "y" ]] || [[ "$RESPONSE" = "yes" ]]; then
     return 0
   fi
@@ -176,7 +178,7 @@ function handle_docker_container_conflict() {
   local readonly CONTAINER_NAME=$1
   local readonly EXIT_ON_NEGATIVE_USER_RESPONSE=$2
   local PROMPT="The container name \"$CONTAINER_NAME\" is already in use by another container. This may happen when running this script multiple times."
-  if $EXIT_ON_NEGATIVE_USER_RESPONSE; then
+  if [[ "${EXIT_ON_NEGATIVE_USER_RESPONSE}" == 'true' ]]; then
     PROMPT="$PROMPT We will attempt to remove the existing container and restart it. Would you like to proceed?"
   else
     PROMPT="$PROMPT Would you like to replace this container? If you answer no, we will proceed with the remainder of the installation."
@@ -221,8 +223,8 @@ function get_random_port {
 
 function create_persisted_state_dir() {
   readonly STATE_DIR="$SHADOWBOX_DIR/persisted-state"
-  mkdir -p --mode=770 "${STATE_DIR}"
-  chmod g+s "${STATE_DIR}"
+  mkdir -p "${STATE_DIR}"
+  chmod ug+rwx,g+s,o-rwx "${STATE_DIR}"
 }
 
 # Generate a secret key for access to the Management API and store it in a tag.
@@ -390,8 +392,8 @@ install_shadowbox() {
 
   log_for_sentry "Creating Outline directory"
   export SHADOWBOX_DIR="${SHADOWBOX_DIR:-/opt/outline}"
-  mkdir -p --mode=770 $SHADOWBOX_DIR
-  chmod u+s $SHADOWBOX_DIR
+  mkdir -p $SHADOWBOX_DIR
+  chmod u+s,ug+rwx,o-rwx $SHADOWBOX_DIR
 
   log_for_sentry "Setting API port"
   API_PORT="${FLAGS_API_PORT}"
@@ -412,11 +414,13 @@ install_shadowbox() {
     exit 1
   fi
 
-  # If $ACCESS_CONFIG already exists, copy it to backup then clear it.
-  # Note we can't do "mv" here as do_install_server.sh may already be tailing
-  # this file.
+  # If $ACCESS_CONFIG is already populated, make a backup before clearing it.
   log_for_sentry "Initializing ACCESS_CONFIG"
-  [[ -f $ACCESS_CONFIG ]] && cp $ACCESS_CONFIG $ACCESS_CONFIG.bak && > $ACCESS_CONFIG
+  if [[ -s $ACCESS_CONFIG ]]; then
+    # Note we can't do "mv" here as do_install_server.sh may already be tailing
+    # this file.
+    cp $ACCESS_CONFIG $ACCESS_CONFIG.bak && true > $ACCESS_CONFIG
+  fi
 
   # Make a directory for persistent state
   run_step "Creating persistent state dir" create_persisted_state_dir

src/server_manager/scripts/fill_packaging_opts.sh

@@ -22,7 +22,9 @@
 # Note that you MUST use "source" in order to run the script in the same process as the calling
 # script, allowing fill_packaging_opts.sh to fill variables for the caller.
 
-readonly YARN_COMMAND="yarn do $(echo "${1}" | sed 's:.*\(src/server_manager/electron_app/.*\)_action.sh:\1:')"
+readonly ELECTRON_PATH='server_manager/electron_app/'
+readonly RELATIVE="${1#*/src/${ELECTRON_PATH}}"
+readonly YARN_COMMAND="yarn do ${ELECTRON_PATH}${RELATIVE%_action.sh}"
 shift
 
 function usage () {

src/server_manager/web_app/build_install_script_action.sh

@@ -23,7 +23,7 @@ rm -rf $OUT_DIR
 mkdir -p $OUT_DIR
 
 pushd $ROOT_DIR/src/server_manager/install_scripts > /dev/null
-tar --create --gzip -f $OUT_DIR/scripts.tgz *.sh
+tar --create --gzip -f $OUT_DIR/scripts.tgz ./*.sh
 
 # Node.js on Cygwin doesn't like absolute Unix-style paths.
 # So, we use a relative path as input.

src/shadowbox/scripts/update_mmdb.sh

@@ -15,7 +15,7 @@ for monthdelta in $(seq 10); do
     newdate=$(date --date="-$monthdelta months" +%Y-%m)
     ADDRESS="https://download.db-ip.com/free/dbip-country-lite-${newdate}.mmdb.gz"
     curl --fail --silent "${ADDRESS}" -o "$TMPDIR/$FILENAME.gz" > /dev/null && break  
-    if [[ $monthdelta -eq 10 ]]; then
+    if [ $monthdelta -eq 10 ]; then
         # A weird exit code on purpose -- we should catch this long before it triggers
         exit 2
     fi