From aa782d0c7cf6027c376b18c87f8100b7085a81f2 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 22 Feb 2023 11:25:39 -0500 Subject: [PATCH] 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: https://github.com/openedx/.github/pull/60 --- .github/workflows/shellcheck.yml | 65 +++++++++++++++++++++++++++++ common/test/data/static/contains.sh | 2 + scripts/all-tests.sh | 8 ++-- scripts/generic-ci-tests.sh | 19 ++++----- scripts/paver_autocomplete.sh | 8 +++- scripts/reset-test-db.sh | 20 ++++----- scripts/vulture/find-dead-code.sh | 16 +++---- 7 files changed, 105 insertions(+), 33 deletions(-) create mode 100644 .github/workflows/shellcheck.yml diff --git a/.github/workflows/shellcheck.yml b/.github/workflows/shellcheck.yml new file mode 100644 index 000000000000..7a4d77c29bc6 --- /dev/null +++ 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 index 6aab7a14fe33..318b9ace8946 100644 --- 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 index f1b6e8d7dcd9..2f343ccc8219 100755 --- 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 index 151c9fa7b4c9..901fef4baa88 100755 --- 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 < "reports/$1.xml" < @@ -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 index 6480d12a0f5a..be1b5fb0455f 100644 --- 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 index 75fa576c4513..b0573a8e1369 100755 --- 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 index f93d869e5c05..e24882595ebb 100755 --- 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"