Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
grayside opened this issue Jan 27, 2020 · 1 comment · Fixed by #1438
Closed

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

grayside opened this issue Jan 27, 2020 · 1 comment · Fixed by #1438
Assignees
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@grayside
Copy link
Contributor

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 tbpg added the type: cleanup An internal cleanup or hygiene concern. label Jan 27, 2020
@tbpg
Copy link
Contributor

tbpg commented Jan 27, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants