build: add shellcheck workflow for PRs and master

Additionally, this brings all existing shell scripts into compliance with ShellCheck. This implements part of an ADR, proposed here: openedx/.github#60
openedx · Feb 24, 2023 · aa782d0 · aa782d0
1 parent 83e4795
commit aa782d0
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 33 deletions.
diff --git a/.github/workflows/shellcheck.yml b/.github/workflows/shellcheck.yml
@@ -0,0 +1,65 @@
+# ShellCheck is a linter for your shell scripts:
+#   https://www.shellcheck.net/
+# This workflow runs it on PRs and pushes to $default-branch
+
+name: ShellCheck
+
+on:
+  pull_request:
+  push:
+    branches:
+    - master
+
+permissions:
+  contents: read
+
+jobs:
+
+  shellcheck:
+    name: Run ShellCheck
+    runs-on: ubuntu-latest
+
+    steps:
+
+    - name: Check out branch
+      uses: actions/checkout@v3
+
+    # Run ShellCheck using a predefine action:
+    # https://github.com/marketplace/actions/shellcheck
+    - name: Run ShellCheck
+      uses: ludeeus/action-shellcheck@master
+      env:
+
+        # We pin to a specific version of ShellCheck so that your build doesn't
+        # break as newer versions with more warnings are released.
+        # Maintainers: Keep an eye out for newer ShellCheck versions and consider
+        # upgrading to them when they are released:
+        #    https://github.com/koalaman/shellcheck/tags
+        version: v0.9.0
+
+        # Severity levels, in increasing order of strictness:
+        #   error
+        #   warning
+        #   info
+        #   style
+        # We recommend `style` for maximum coverage, but adjust as you see fit.
+        severity: style
+
+        # Add any custom shellcheck CLI options here.
+        # For example, use `-e SC2059` to ignore a certain warning.
+        # (However, it's usually to ignore individual warnings inline: `# shellcheck: disable=SC2059`)
+        SHELLCHECK_OPTS:
+
+        # Ignore filepaths or filenames.
+        # Each is a single string, space-separated.
+        ignore_paths:
+        ignore_names:
+
+        # By default, your whole repo is scanned for shell scripts.
+        # Uncomment the next line if you want to limit to a certain directory.
+        #scandir: './scripts'
+
+        # This ensures that all .sh files are passed to shellcheck in one go, making
+        # ShellCheck aware of "include" logic (`source ./constants.sh`) between scripts.
+        check_together: 'yes'
+
diff --git a/common/test/data/static/contains.sh b/common/test/data/static/contains.sh
@@ -1,2 +1,4 @@
 #!/usr/bin/env zsh
+# shellcheck disable=all
+# ^ This is zsh, which shellcheck doesn't support.
 git log --all ^opaque-keys-merge-base --format=%H $1 | while read f; do git branch --contains $f; done | sort -u
diff --git a/scripts/all-tests.sh b/scripts/all-tests.sh
@@ -13,13 +13,13 @@ set -e
 # Violations thresholds for failing the build
 source scripts/thresholds.sh
 
-XSSLINT_THRESHOLDS=`cat scripts/xsslint_thresholds.json`
+XSSLINT_THRESHOLDS=$(cat scripts/xsslint_thresholds.json)
 export XSSLINT_THRESHOLDS=${XSSLINT_THRESHOLDS//[[:space:]]/}
 
 
-# Run appropriate CI system script
-if [ -n "$SCRIPT_TO_RUN" ] ; then
-    $SCRIPT_TO_RUN
+# Run appropriate CI system script (with args if provided)
+if [ -n "${SCRIPT_TO_RUN[*]}" ] ; then
+    "${SCRIPT_TO_RUN[@]}"
 
     # Exit with the exit code of the called script
     exit $?

diff --git a/scripts/generic-ci-tests.sh b/scripts/generic-ci-tests.sh
@@ -55,7 +55,7 @@ git clean -qxfd
 
 function emptyxunit {
 
-    cat > reports/$1.xml <<END
+    cat > "reports/$1.xml" <<END
 <?xml version="1.0" encoding="UTF-8"?>
 <testsuite name="$1" tests="1" errors="0" failures="0" skip="0">
 <testcase classname="pavelib.quality" name="$1" time="0.604"></testcase>
@@ -82,19 +82,18 @@ else
 fi
 
 PAVER_ARGS="-v"
-PARALLEL="--processes=-1"
 export SUBSET_JOB=$JOB_NAME
 
 function run_paver_quality {
     QUALITY_TASK=$1
     shift
     mkdir -p test_root/log/
-    LOG_PREFIX=test_root/log/$QUALITY_TASK
-    $TOX paver $QUALITY_TASK $* 2> $LOG_PREFIX.err.log > $LOG_PREFIX.out.log || {
+    LOG_PREFIX="test_root/log/$QUALITY_TASK"
+    "$TOX" paver "$QUALITY_TASK" "$@" 2> "$LOG_PREFIX.err.log" > "$LOG_PREFIX.out.log" || {
         echo "STDOUT (last 100 lines of $LOG_PREFIX.out.log):";
-        tail -n 100 $LOG_PREFIX.out.log;
+        tail -n 100 "$LOG_PREFIX.out.log"
         echo "STDERR (last 100 lines of $LOG_PREFIX.err.log):";
-        tail -n 100 $LOG_PREFIX.err.log;
+        tail -n 100 "$LOG_PREFIX.err.log"
         return 1;
     }
     return 0;
@@ -128,11 +127,11 @@ case "$TEST_SUITE" in
                 echo "Finding pycodestyle violations and storing report..."
                 run_paver_quality run_pep8 || { EXIT=1; }
                 echo "Finding ESLint violations and storing report..."
-                run_paver_quality run_eslint -l $ESLINT_THRESHOLD || { EXIT=1; }
+                run_paver_quality run_eslint -l "$ESLINT_THRESHOLD" || { EXIT=1; }
                 echo "Finding Stylelint violations and storing report..."
                 run_paver_quality run_stylelint || { EXIT=1; }
                 echo "Running xss linter report."
-                run_paver_quality run_xsslint -t $XSSLINT_THRESHOLDS || { EXIT=1; }
+                run_paver_quality run_xsslint -t "$XSSLINT_THRESHOLDS" || { EXIT=1; }
                 echo "Running PII checker on all Django models..."
                 run_paver_quality run_pii_check || { EXIT=1; }
                 echo "Running reserved keyword checker on all Django models..."
@@ -144,7 +143,7 @@ case "$TEST_SUITE" in
         # Need to create an empty test result so the post-build
         # action doesn't fail the build.
         emptyxunit "stub"
-        exit $EXIT
+        exit "$EXIT"
         ;;
 
     "js-unit")
@@ -167,6 +166,6 @@ case "$TEST_SUITE" in
         # Note that by default the value of this variable EXIT is not set, so if
         # neither command fails then the exit command resolves to simply exit
         # which is considered successful.
-        exit $EXIT
+        exit "$EXIT"
         ;;
 esac
diff --git a/scripts/paver_autocomplete.sh b/scripts/paver_autocomplete.sh
@@ -1,3 +1,9 @@
+# shellcheck disable=all
+# ^ Paver in edx-platform is on the way out
+#   (https://github.com/openedx/edx-platform/issues/31798)
+#   so we're not going to bother fixing these shellcheck
+#   violations.
+
 # Courtesy of Gregory Nicholas
 
 _subcommand_opts()
@@ -13,7 +19,7 @@ BEGIN {
 
 {
     for (i = 1; i <= NF; i = i + 1) {
-        # Match short options (-a, -S, -3)
+        # Match short options (-a, -S, -3:)
         # or long options (--long-option, --another_option)
         # in output from paver help [subcommand]
         if ($i ~ /^(-[A-Za-z0-9]|--[A-Za-z][A-Za-z0-9_-]*)/) {

diff --git a/scripts/reset-test-db.sh b/scripts/reset-test-db.sh
@@ -62,23 +62,23 @@ calculate_migrations() {
     output_file="common/test/db_cache/bok_choy_${db}_migrations.yaml"
     # Redirect stdout to /dev/null because the script will print
     # out all migrations to both stdout and the output file.
-    ./manage.py lms --settings $SETTINGS show_unapplied_migrations --database $db --output_file $output_file 1>/dev/null
+    ./manage.py lms --settings "$SETTINGS" show_unapplied_migrations --database "$db" --output_file "$output_file" 1>/dev/null
 }
 
 run_migrations() {
     echo "Running the lms migrations on the $db bok_choy DB."
-    ./manage.py lms --settings $SETTINGS migrate --database $db --traceback --noinput
+    ./manage.py lms --settings "$SETTINGS" migrate --database "$db" --traceback --noinput
     echo "Running the cms migrations on the $db bok_choy DB."
-    ./manage.py cms --settings $SETTINGS migrate --database $db --traceback --noinput
+    ./manage.py cms --settings "$SETTINGS" migrate --database "$db" --traceback --noinput
 }
 
 load_cache_into_db() {
     echo "Loading the schema from the filesystem into the $db MySQL DB."
-    mysql $MYSQL_HOST -u root "${databases["$db"]}" < $DB_CACHE_DIR/bok_choy_schema_$db.sql
+    mysql "$MYSQL_HOST" -u root "${databases["$db"]}" < "$DB_CACHE_DIR/bok_choy_schema_$db.sql"
     echo "Loading the fixture data from the filesystem into the $db MySQL DB."
-    ./manage.py lms --settings $SETTINGS loaddata --database $db $DB_CACHE_DIR/bok_choy_data_$db.json
+    ./manage.py lms --settings "$SETTINGS" loaddata --database "$db" "$DB_CACHE_DIR/bok_choy_data_$db.json"
     echo "Loading the migration data from the filesystem into the $db MySQL DB."
-    mysql $MYSQL_HOST -u root "${databases["$db"]}" < $DB_CACHE_DIR/bok_choy_migrations_data_$db.sql
+    mysql "$MYSQL_HOST" -u root "${databases["$db"]}" < "$DB_CACHE_DIR/bok_choy_migrations_data_$db.sql"
 }
 
 rebuild_cache_for_db() {
@@ -87,13 +87,13 @@ rebuild_cache_for_db() {
 
     # Dump the schema and data to the cache
     echo "Using the dumpdata command to save the $db fixture data to the filesystem."
-    ./manage.py lms --settings $SETTINGS dumpdata --database $db > $DB_CACHE_DIR/bok_choy_data_$db.json --exclude=api_admin.Catalog
+    ./manage.py lms --settings "$SETTINGS" dumpdata --database "$db" > "$DB_CACHE_DIR/bok_choy_data_$db.json" --exclude=api_admin.Catalog
     echo "Saving the schema of the $db bok_choy DB to the filesystem."
-    mysqldump $MYSQL_HOST -u root --no-data --skip-comments --skip-dump-date "${databases[$db]}" > $DB_CACHE_DIR/bok_choy_schema_$db.sql
+    mysqldump "$MYSQL_HOST" -u root --no-data --skip-comments --skip-dump-date "${databases[$db]}" > "$DB_CACHE_DIR/bok_choy_schema_$db.sql"
 
     # dump_data does not dump the django_migrations table so we do it separately.
     echo "Saving the django_migrations table of the $db bok_choy DB to the filesystem."
-    mysqldump $MYSQL_HOST -u root --no-create-info --skip-comments --skip-dump-date "${databases["$db"]}" django_migrations > $DB_CACHE_DIR/bok_choy_migrations_data_$db.sql
+    mysqldump $MYSQL_HOST -u root --no-create-info --skip-comments --skip-dump-date "${databases["$db"]}" django_migrations > "$DB_CACHE_DIR/bok_choy_migrations_data_$db.sql"
 }
 
 for db in "${database_order[@]}"; do
@@ -107,7 +107,7 @@ for db in "${database_order[@]}"; do
         # or a jenkins worker environment) that already ran tests on another commit that had
         # different migrations that created, dropped, or altered tables.
         echo "Issuing a reset_db command to the $db bok_choy MySQL database."
-        ./manage.py lms --settings $SETTINGS reset_db --traceback --router $db
+        ./manage.py lms --settings "$SETTINGS" reset_db --traceback --router "$db"
     fi
 
     if ! [[ $CALCULATE_MIGRATIONS ]]; then

diff --git a/scripts/vulture/find-dead-code.sh b/scripts/vulture/find-dead-code.sh
@@ -31,19 +31,19 @@ set -e
 ############################################################################
 
 OUTPUT_DIR="reports/vulture"
-mkdir -p ${OUTPUT_DIR}
-OUTPUT_FILE=${OUTPUT_DIR}/vulture-report.txt
-echo '' > $OUTPUT_FILE
+mkdir -p "$OUTPUT_DIR"
+OUTPUT_FILE="${OUTPUT_DIR}/vulture-report.txt"
+echo '' > "$OUTPUT_FILE"
 # exclude test code from analysis, as it isn't explicitly called by other
 # code. Additionally, application code that is only called by tests 
 # should be considered dead
 EXCLUSIONS='/test,/acceptance,cms/envs,lms/envs,/terrain,migrations/,signals.py'
 MIN_CONFIDENCE=90
 # paths to the code on which to run the analysis
-CODE_PATHS='cms common lms openedx'
+CODE_PATHS=('cms' 'common' 'lms' 'openedx')
 WHITELIST_PATH="$(dirname "${BASH_SOURCE[0]}")/whitelist.py"
-echo "Checking for dead code in the following paths: $CODE_PATHS"
+echo "Checking for dead code in the following paths: ${CODE_PATHS[*]}"
 echo "Results can be found in $OUTPUT_FILE"
-vulture $CODE_PATHS $WHITELIST_PATH --exclude $EXCLUSIONS \
---min-confidence $MIN_CONFIDENCE \
---sort-by-size |tac > $OUTPUT_FILE
+vulture "${CODE_PATHS[@]}" "$WHITELIST_PATH" --exclude "$EXCLUSIONS" \
+--min-confidence "$MIN_CONFIDENCE" \
+--sort-by-size |tac > "$OUTPUT_FILE"