Updates from code-review comments

Author: jeckhart

modules/bash-commons/src/array.sh

@@ -20,18 +20,18 @@ function array_contains {
 #
 # Examples:
 #
-# array_split "a,b,c", ","
+# array_split "a,b,c" ","
 #   Returns: ("a" "b" "c")
 #
 # Hint:
-# When calling this function, use the following construction: ary=( $(array_split "a,b,c", ",") )
+# When calling this function, use the following construction: ary=( $(array_split "a,b,c" ",") )
 #
 # Sources:
 # - https://stackoverflow.com/a/15988793/2308858
 function array_split {
   local -r separator="$1"
   local -r str="$2"
-  local ary=()
+  local -a ary=()
 
   IFS="$separator" read -a ary <<<"$str"
 
@@ -48,7 +48,7 @@ function array_split {
 function array_join {
   local -r separator="$1"
   shift
-  local -ra values=("$@")
+  local -ar values=("$@")
 
   local out=""
   for (( i=0; i<"${#values[@]}"; i++ )); do
@@ -77,7 +77,7 @@ function array_join {
 function array_prepend {
   local -r prefix="$1"
   shift 1
-  local -r ary=($@)
+  local -ar ary=($@)
 
   updated_ary=( "${ary[@]/#/$prefix}" )
   echo ${updated_ary[*]}

modules/bash-commons/src/assert.sh

@@ -70,30 +70,29 @@ function assert_value_in_list {
   fi
 }
 
+# Reads in a list of keys and values and asserts that one and only one of the values is set.
+# This is useful for command line options that are mutually exclusive.
+# Example:
+#   assert_exactly_one_of "--opt1" "" "--opt2" "val2" "--opt3" "" "--opt4" ""
+# Examples that assert failure:
+#   assert_exactly_one_of "--opt1" "val1" "--opt2" "val2" "--opt3" "" "--opt4" ""
+#   assert_exactly_one_of "--opt1" "" "--opt2" "" "--opt3" "" "--opt4" ""
 function assert_exactly_one_of {
-  local -r arg_name1="$1"
-  local -r arg_val1="$2"
-  local -r arg_name2="$3"
-  local -r arg_val2="$4"
-  local -r arg_name3="$5"
-  local -r arg_val3="$6"
+  local -ra args=("$@")
+  local -r num_args="${#args[@]}"
 
   local num_non_empty=0
-  local arg_names=()
-  local arg_vals=()
-
-  if [[ ! -z "$arg_name1" ]]; then arg_names+=("$arg_name1"); arg_vals+=("$arg_val1"); fi
-  if [[ ! -z "$arg_name2" ]]; then arg_names+=("$arg_name2"); arg_vals+=("$arg_val2"); fi
-  if [[ ! -z "$arg_name3" ]]; then arg_names+=("$arg_name3"); arg_vals+=("$arg_val3"); fi
+  local -a arg_names=()
 
   # Determine how many arg_vals are non-empty
-  for (( i=0; i<${#arg_vals[@]}; i++ )); do
-    if [[ ! -z "${arg_vals[i]}" ]]; then
+  for (( i=0; i<$((num_args+1)); i+=2 )); do
+    arg_names+=("${args[i]}")
+    if [[ ! -z "${args[i+1]}" ]]; then
       num_non_empty=$((num_non_empty+1))
     fi
   done
 
-  if [[ ${num_non_empty} != 1 ]]; then
+  if [[ "$num_non_empty" -ne 1 ]]; then
     log_error "Exactly one of ${arg_names[*]} must be set."
     exit 1
   fi

modules/bash-commons/src/aws.sh

@@ -3,6 +3,11 @@
 # (a) it's more convenient to fetch specific info you need, such as an EC2 Instance's private IP and (b) so you can
 # replace these helpers with mocks to do local testing or unit testing.
 
+# shellcheck source=./modules/bash-commons/src/assert.sh
+source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/assert.sh"
+# shellcheck source=./modules/bash-commons/src/log.sh
+source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/log.sh"
+
 # Look up the given path in the EC2 Instance metadata endpoint
 function aws_lookup_path_in_instance_metadata {
   local -r path="$1"

modules/bash-commons/src/file.sh

@@ -55,7 +55,7 @@ function file_replace_text_in_files {
   local -r original_text_regex="$1"
   local -r replacement_text="$2"
   shift 2
-  local -r files=("$@")
+  local -ar files=("$@")
 
   for file in "${files[@]}"; do
     file_replace_text "$original_text_regex" "$replacement_text" "$file"

modules/bash-commons/src/java.sh

@@ -169,6 +169,21 @@ load "test-helper"
   assert_failure
 }
 
+@test "assert_exactly_one_of list of length 4, with value in pos 2" {
+  run assert_exactly_one_of "--foo" "" "--bar" "bar" "--baz" "" "--qux" ""
+  assert_success
+}
+
+@test "assert_exactly_one_of list of length 4, with value in pos 2 and 4" {
+  run assert_exactly_one_of "--foo" "" "--bar" "bar" "--baz" "" "--qux" "qux"
+  assert_failure
+}
+
+@test "assert_exactly_one_of list of length 4, with no value set" {
+  run assert_exactly_one_of "--foo" "" "--bar" "" "--baz" "" "--qux" ""
+  assert_failure
+}
+
 @test "assert_uid_is_root_or_sudo as root" {
   run assert_uid_is_root_or_sudo
   assert_success