testing: BASH syntax - Add shellcheck to CI or as a bot

[grayside]

In #1168, a later comment came up that I was using camel case instead of snake case for a variable. As far as I can tell, we do not currently apply linting checks to shell scripts. Let's start that, either as part of our Kokoro pipeline or consider if we have an interest in using/enhancing a bot.

Related issues: GoogleCloudPlatform/nodejs-docs-samples#1386

Shellcheck (github, tryit) is the current front-runner in shell script linting. While it doesn't enforce variable name style it does enforce many other good practices.

Usage Example

Run with Docker:

docker run --rm -v "$PWD:/mnt" koalaman/shellcheck:stable \
    testing/kokoro/system_tests.sh

Click to expand output

In system_tests.sh line 32:
for i in `find . -name go.mod`; do
         ^-------------------^ SC2044: For loops over find output are fragile. Use find -exec or a while read loop.
         ^-------------------^ SC2006: Use $(...) notation instead of legacy backticked `...`.

Did you mean:
for i in $(find . -name go.mod); do


In system_tests.sh line 33:
  pushd `dirname $i` > /dev/null;
        ^----------^ SC2046: Quote this to prevent word splitting.
        ^----------^ SC2006: Use $(...) notation instead of legacy backticked `...`.
                 ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  pushd $(dirname "$i") > /dev/null;


In system_tests.sh line 35:
    git diff go.mod | tee /dev/stderr | (! read)
                                           ^--^ SC2162: read without -r will mangle backslashes.


In system_tests.sh line 36:
    [ -f go.sum ] && git diff go.sum | tee /dev/stderr | (! read)
                                                            ^--^ SC2162: read without -r will mangle backslashes.


In system_tests.sh line 50:
export STORAGE_HMAC_ACCESS_KEY_ID="$($KOKORO_KEYSTORE_DIR/71386_golang-samples-kokoro-gcs-hmac-secret)"
       ^------------------------^ SC2155: Declare and assign separately to avoid masking return values.
                                     ^------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
export STORAGE_HMAC_ACCESS_KEY_ID="$("$KOKORO_KEYSTORE_DIR"/71386_golang-samples-kokoro-gcs-hmac-secret)"


In system_tests.sh line 51:
export STORAGE_HMAC_ACCESS_SECRET_KEY="$($KOKORO_KEYSTORE_DIR/71386_golang-samples-kokoro-gcs-hmac-id)"
       ^----------------------------^ SC2155: Declare and assign separately to avoid masking return values.
                                         ^------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
export STORAGE_HMAC_ACCESS_SECRET_KEY="$("$KOKORO_KEYSTORE_DIR"/71386_golang-samples-kokoro-gcs-hmac-id)"


In system_tests.sh line 67:
export GOLANG_SAMPLES_PROJECT_ID=$(gimmeproj -project golang-samples-tests lease $TIMEOUT);
       ^-----------------------^ SC2155: Declare and assign separately to avoid masking return values.


In system_tests.sh line 76:
trap "go clean -modcache; gimmeproj -project golang-samples-tests done $GOLANG_SAMPLES_PROJECT_ID" EXIT
                                                                       ^------------------------^ SC2064: Use single quotes, otherwise this expands now rather than when signalled.


In system_tests.sh line 102:
if [ -z ${KOKORO_GITHUB_PULL_REQUEST_NUMBER:-} ]; then
        ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
if [ -z "${KOKORO_GITHUB_PULL_REQUEST_NUMBER:-}" ]; then


In system_tests.sh line 108:
SIGNIFICANT_CHANGES=$(git --no-pager diff --name-only HEAD..master | egrep -v '(\.md$|^\.github)' || true)
                                                                     ^---^ SC2196: egrep is non-standard and deprecated. Use grep -E instead.


In system_tests.sh line 111:
CHANGED_DIRS=$(echo $SIGNIFICANT_CHANGES | tr ' ' '\n' | grep "/" | cut -d/ -f1 | sort -u | tr '\n' ' ')
                    ^------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
CHANGED_DIRS=$(echo "$SIGNIFICANT_CHANGES" | tr ' ' '\n' | grep "/" | cut -d/ -f1 | sort -u | tr '\n' ' ')


In system_tests.sh line 113:
if echo $SIGNIFICANT_CHANGES | tr ' ' '\n' | grep "^go.mod$" || [[ $CHANGED_DIRS =~ "testing" || $CHANGED_DIRS =~ "internal" ]]; then
        ^------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
if echo "$SIGNIFICANT_CHANGES" | tr ' ' '\n' | grep "^go.mod$" || [[ $CHANGED_DIRS =~ "testing" || $CHANGED_DIRS =~ "internal" ]]; then


In system_tests.sh line 132:
  TARGET=$(printf "./%s/... " $TARGET_DIRS)
                              ^----------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  TARGET=$(printf "./%s/... " "$TARGET_DIRS")


In system_tests.sh line 137:
if [ $GOLANG_SAMPLES_GO_VET ]; then
     ^--------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
if [ "$GOLANG_SAMPLES_GO_VET" ]; then


In system_tests.sh line 143:
  files=$(find $target_dir \( -exec [ -f {}/go.mod ] \; -prune \) -o -name "*.go" -print)
               ^---------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  files=$(find "$target_dir" \( -exec [ -f {}/go.mod ] \; -prune \) -o -name "*.go" -print)


In system_tests.sh line 152:
    go vet $TARGET
           ^-----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
    go vet "$TARGET"


In system_tests.sh line 157:
  for i in $(find $target_dir -name "go.mod")
           ^-- SC2044: For loops over find output are fragile. Use find -exec or a while read loop.
                  ^---------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  for i in $(find "$target_dir" -name "go.mod")


In system_tests.sh line 159:
    mod="$(dirname $i)"
                   ^-- SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
    mod="$(dirname "$i")"


In system_tests.sh line 170:
2>&1 go test -timeout $TIMEOUT -v . $TARGET | tee $OUTFILE
                                    ^-----^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
2>&1 go test -timeout $TIMEOUT -v . "$TARGET" | tee $OUTFILE


In system_tests.sh line 174:
cat $OUTFILE | /go/bin/go-junit-report -set-exit-code > sponge_log.xml
    ^------^ SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In system_tests.sh line 181:
  gcloud auth activate-service-account --key-file $KOKORO_KEYSTORE_DIR/71386_kokoro-golang-samples-tests
                                                  ^------------------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean:
  gcloud auth activate-service-account --key-file "$KOKORO_KEYSTORE_DIR"/71386_kokoro-golang-samples-tests

For more information:
  https://www.shellcheck.net/wiki/SC2044 -- For loops over find output are fr...
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2064 -- Use single quotes, otherwise this...

Considering a bot approach

If we think this should be approached as a bot, I've had https://github.com/googleapis/repo-automation-bots recommended, though there's also DPEBot.

[tbpg]

+1 to repo-automation-bots if something doesn't already exist. It will most likely supersede DPEBot -- new bots should be made there.