From 20444a60d8f45f07235af9a688006466c4c571eb Mon Sep 17 00:00:00 2001
From: Daniel Bast <2790401+dbast@users.noreply.github.com>
Date: Sun, 8 Dec 2024 14:48:16 +0100
Subject: [PATCH] Fix shellcheck CI job, fix/handle all shellcheck findings
 (#620)

* Fix shellcheck linting

Signed-off-by: Daniel Bast <2790401+dbast@users.noreply.github.com>

* Fix/handle all shellcheck findings

Signed-off-by: Daniel Bast <2790401+dbast@users.noreply.github.com>

* Incorporate review findings


Signed-off-by: Daniel Bast <2790401+dbast@users.noreply.github.com>

* Switch to pre-commit

Signed-off-by: Daniel Bast <2790401+dbast@users.noreply.github.com>

---------

Signed-off-by: Daniel Bast <2790401+dbast@users.noreply.github.com>
---
 .circleci/config.yml                          | 18 +++++++++++----
 .pre-commit-config.yaml                       | 23 +++++++++++++++++++
 Makefile                                      |  9 ++++----
 cancel/test/test.sh                           |  1 -
 cancel/test/trigger.sh                        |  1 +
 get_obj/test/test.sh                          | 16 ++++++-------
 helm_remote/test/verify.exp                   |  0
 kubectl_build/Tiltfile                        |  0
 kubefwd/run-kubefwd-internal.sh               |  2 +-
 kubefwd/sudo-kubefwd.sh                       |  1 +
 ngrok/ngrok-reconcile-uiresource.sh           |  1 +
 pypiserver/src/build.sh                       |  3 +++
 restart_process/test/Dockerfile.test          |  3 ++-
 restart_process/test/start.sh                 |  4 +++-
 .../test/trigger_custom_deploy_live_update.sh |  5 ++--
 syncback/krsync-callback.sh                   |  1 +
 syncback/krsync.sh                            | 16 +++++++++----
 syncback/test/test.sh                         | 18 +++++++--------
 test.py                                       |  6 ++++-
 test.sh                                       |  2 +-
 20 files changed, 91 insertions(+), 39 deletions(-)
 create mode 100644 .pre-commit-config.yaml
 mode change 100644 => 100755 helm_remote/test/verify.exp
 mode change 100644 => 100755 kubectl_build/Tiltfile
 mode change 100644 => 100755 pypiserver/src/build.sh

diff --git a/.circleci/config.yml b/.circleci/config.yml
index 2137f5669..feb0f9d9f 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -9,13 +9,23 @@ jobs:
       - run: GOBIN=$HOME/.bin go install 'github.com/tilt-dev/tilt-extensions-ci@v1.1.0'
       - run: $HOME/.bin/tilt-extensions-ci .
 
-  shellcheck:
+  pre-commit:
     docker:
-      - image: docker/tilt-extensions-ci@sha256:70127b822dc42d45480dc6cddd97297d7b454503aa75b053728b565533233989
+      - image: python:3.11.10-bookworm@sha256:5e5ef24d131e78708b5947823f2830ab42328132b6caff002bf6481d64b99593
 
     steps:
       - checkout
-      - run: make shellcheck
+      - run: |
+          python -m pip install pre-commit
+          cp .pre-commit-config.yaml pre-commit-cache-key.txt
+          python --version >> pre-commit-cache-key.txt
+      - restore_cache:
+          key: pre-commit-3-{{ checksum "pre-commit-cache-key.txt" }}
+      - run: make pre-commit
+      - save_cache:
+          key: pre-commit-3-{{ checksum "pre-commit-cache-key.txt" }}
+          paths:
+            - ~/.cache/pre-commit
 
   test:
     # A common error is to build an extension that doesn't properly handle
@@ -39,5 +49,5 @@ workflows:
   build:
     jobs:
       - validate
-      - shellcheck
+      - pre-commit
       - test
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
new file mode 100644
index 000000000..b2b1dbc82
--- /dev/null
+++ b/.pre-commit-config.yaml
@@ -0,0 +1,23 @@
+# Update this file via:
+#   pre-commit autoupdate
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v5.0.0
+    hooks:
+      - id: check-yaml
+        args: [--allow-multiple-documents]
+        exclude: ^.*templates.*$
+      - id: check-executables-have-shebangs
+      - id: check-shebang-scripts-are-executable
+  - repo: https://github.com/shellcheck-py/shellcheck-py
+    rev: v0.10.0.1
+    hooks:
+      - id: shellcheck
+  - repo: https://github.com/python-jsonschema/check-jsonschema
+    rev: 0.30.0
+    hooks:
+      - id: check-circle-ci
+  - repo: meta
+    hooks:
+      - id: check-hooks-apply
+      - id: check-useless-excludes
diff --git a/Makefile b/Makefile
index 3239592ca..19ae36e93 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,4 @@
-.PHONY: test publish-ci-image
+SHELL := /bin/bash -o pipefail -o errexit
 
 test:
 	./test.sh
@@ -6,6 +6,7 @@ test:
 publish-ci-image:
 	docker buildx build --push --pull --platform linux/amd64 -t docker/tilt-extensions-ci -f .circleci/Dockerfile .circleci
 
-shellcheck:
-	find . -type f -name '*.sh' -not -path "*/node_modules/*" -not -path "*/.git-sources/*" -not -path "*/.git/*" -exec \
-	    docker run --rm -it -e SHELLCHECK_OPTS="-e SC2001" -v $$(pwd):/mnt nlknguyen/alpine-shellcheck {} \;
+pre-commit:
+	pre-commit run --verbose --show-diff-on-failure --color=always --all-files
+
+.PHONY: $(MAKECMDGOALS)
diff --git a/cancel/test/test.sh b/cancel/test/test.sh
index 73eb5cfa5..1e36f2892 100755
--- a/cancel/test/test.sh
+++ b/cancel/test/test.sh
@@ -4,7 +4,6 @@ cd "$(dirname "$0")" || exit
 
 set -uxo pipefail
 tilt ci > tilt.log &
-TILT_PID=$!
 
 sleep 1
 timeout 30 tail -f tilt.log | grep -q "uibutton.tilt.dev/sleep:update:cancel created"
diff --git a/cancel/test/trigger.sh b/cancel/test/trigger.sh
index f82913e6e..43013807d 100755
--- a/cancel/test/trigger.sh
+++ b/cancel/test/trigger.sh
@@ -3,6 +3,7 @@
 
 YAML=$(tilt get uibutton "sleep:update:cancel" -o yaml)
 TIME=$(date '+%FT%T.000000Z')
+# shellcheck disable=SC2001
 NEW_YAML=$(echo "$YAML" | sed "s/lastClickedAt.*/lastClickedAt: $TIME/g")
 
 # Currently, kubectl doesn't support subresource APIs.
diff --git a/get_obj/test/test.sh b/get_obj/test/test.sh
index 8a0bdea9f..3eedffcdb 100755
--- a/get_obj/test/test.sh
+++ b/get_obj/test/test.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit 1
 
 # Spinning up initial test deployment
 tilt ci -f setup.Tiltfile
@@ -17,7 +17,7 @@ RESULT=$?
 if [[ $RESULT -ne 0 ]];then
   echo "ERROR: App failed to start"
   cat tilt.log
-  exit $RESULT
+  exit "$RESULT"
 fi
 
 echo "Verifying remote development is using the correct image built from Dockerfile.dev"
@@ -36,7 +36,7 @@ RESULT=$?
 if [[ $RESULT -ne 0 ]];then
   echo "ERROR: File sync doesn't work"
   cat tilt.log
-  exit $RESULT
+  exit "$RESULT"
 fi
 
 echo "Verifying that port forwarding was enabled"
@@ -45,23 +45,23 @@ RESULT=$?
 if [[ $RESULT -ne 0 ]];then
   echo "ERROR: Port forwarding wasn't enabled"
   cat tilt.log
-  exit $RESULT
+  exit "$RESULT"
 fi
 
 echo "Verifying that livenessProbe was removed"
-RESULT=$(kubectl get deployment/example-nodejs -o yaml | grep "livenessProbe" | wc -l)
+RESULT=$(kubectl get deployment/example-nodejs -o yaml | grep -c "livenessProbe")
 if [[ $RESULT -ne 0 ]];then
   echo "ERROR: livenessProbe wasn't removed"
   kubectl get deployment/example-nodejs -o yaml
-  exit $RESULT
+  exit "$RESULT"
 fi
 
 echo "Verifying that readinessProbe was removed"
-RESULT=$(kubectl get deployment/example-nodejs -o yaml | grep "readinessProbe" | wc -l)
+RESULT=$(kubectl get deployment/example-nodejs -o yaml | grep -c "readinessProbe")
 if [[ $RESULT -ne 0 ]];then
   echo "ERROR: readinessProbe wasn't removed"
   kubectl get deployment/example-nodejs -o yaml
-  exit $RESULT
+  exit "$RESULT"
 fi
 
 cat tilt.log
diff --git a/helm_remote/test/verify.exp b/helm_remote/test/verify.exp
old mode 100644
new mode 100755
diff --git a/kubectl_build/Tiltfile b/kubectl_build/Tiltfile
old mode 100644
new mode 100755
diff --git a/kubefwd/run-kubefwd-internal.sh b/kubefwd/run-kubefwd-internal.sh
index 7b9582c2b..4f56e97a8 100755
--- a/kubefwd/run-kubefwd-internal.sh
+++ b/kubefwd/run-kubefwd-internal.sh
@@ -6,7 +6,7 @@
 set -euo pipefail
 
 flags=()
-while read namespace; do
+while read -r namespace; do
     if [[ "$namespace" == "" ]]; then
       continue
     fi
diff --git a/kubefwd/sudo-kubefwd.sh b/kubefwd/sudo-kubefwd.sh
index 8f8f5d24b..0d7684457 100755
--- a/kubefwd/sudo-kubefwd.sh
+++ b/kubefwd/sudo-kubefwd.sh
@@ -48,6 +48,7 @@ chmod a+rw "$TRIGGER"
 "$DIR/watch-namespaces.sh" "$TRIGGER" &
 WATCH_PID="$!"
 
+# shellcheck disable=SC2317
 function cleanup {
     set -x
     set +e
diff --git a/ngrok/ngrok-reconcile-uiresource.sh b/ngrok/ngrok-reconcile-uiresource.sh
index 0943d0df9..7e461b611 100755
--- a/ngrok/ngrok-reconcile-uiresource.sh
+++ b/ngrok/ngrok-reconcile-uiresource.sh
@@ -17,6 +17,7 @@ links="$(tilt get uiresource -o jsonpath='{range .status.endpointLinks[*]}{.url}
 for link in $links;
 do
     host="localhost"
+    # shellcheck disable=SC2001
     port="$(echo "$link" | sed -e "s|^http://$host:\([0-9]*\).*|\1|")"
     if [[ "$port" != "$link" ]]; then
         ports+=("$port")
diff --git a/pypiserver/src/build.sh b/pypiserver/src/build.sh
old mode 100644
new mode 100755
index 2b44243ed..5b026604f
--- a/pypiserver/src/build.sh
+++ b/pypiserver/src/build.sh
@@ -1,3 +1,5 @@
+#!/bin/bash
+
 if [[ $# -eq 1 ]]; then
   printf "Specify path to the package"
   exit 1
@@ -12,4 +14,5 @@ if [[ $# -eq 2 ]]; then
   cmd="${cmd} upload -r ${2}"
 fi
 
+# shellcheck disable=SC2086
 python3 ${cmd}
diff --git a/restart_process/test/Dockerfile.test b/restart_process/test/Dockerfile.test
index cfbeb9faf..87397f41a 100644
--- a/restart_process/test/Dockerfile.test
+++ b/restart_process/test/Dockerfile.test
@@ -1,4 +1,5 @@
-FROM alpine
+# distro with bash required as start.sh contains bash specific code
+FROM debian:bookworm-slim
 
 RUN echo 0 > restart_count.txt
 
diff --git a/restart_process/test/start.sh b/restart_process/test/start.sh
index c9bc98143..c22484567 100755
--- a/restart_process/test/start.sh
+++ b/restart_process/test/start.sh
@@ -1,4 +1,6 @@
-#!/bin/sh
+#!/bin/bash
+# bash shebang required as we are using bash specific features: pipefail + trap
+
 set -euo pipefail
 RESTART_COUNT="$(cat restart_count.txt)"
 echo "RESTART_COUNT: $RESTART_COUNT"
diff --git a/restart_process/test/trigger_custom_deploy_live_update.sh b/restart_process/test/trigger_custom_deploy_live_update.sh
index a7547a095..68807bfaa 100755
--- a/restart_process/test/trigger_custom_deploy_live_update.sh
+++ b/restart_process/test/trigger_custom_deploy_live_update.sh
@@ -8,9 +8,8 @@ set -u
 touch start.sh
 # seconds
 TIMEOUT=10
-for ((i=0;i<=$TIMEOUT;++i)) do
-  tilt logs | grep -v test_update | grep -q "RESTART_COUNT: 1"
-  if [[ $? -eq 0 ]]; then
+for ((i=0;i<=TIMEOUT;++i)) do
+  if tilt logs | grep -v test_update | grep -q "RESTART_COUNT: 1"; then
     echo restart detected
     exit 0
   fi
diff --git a/syncback/krsync-callback.sh b/syncback/krsync-callback.sh
index 0cb20a446..68dbbaa6e 100755
--- a/syncback/krsync-callback.sh
+++ b/syncback/krsync-callback.sh
@@ -16,5 +16,6 @@ fi
 
 # $K8S_OBJECT is intentionally not quoted here so -n/-c flags can be
 # parsed by kubectl
+# shellcheck disable=SC2086
 exec kubectl exec -i $K8S_OBJECT -- "$@"
 
diff --git a/syncback/krsync.sh b/syncback/krsync.sh
index f4a2ae339..6237d5aea 100755
--- a/syncback/krsync.sh
+++ b/syncback/krsync.sh
@@ -28,27 +28,31 @@ install_rsync_tilt() {
     set +e # disable exit on error
     # $K8S_OBJECT is intentionally not quoted here so -n/-c flags can be
     # parsed by kubectl
+    # shellcheck disable=SC2086
     kubectl exec $K8S_OBJECT -- ls "$RSYNC_TILT_PATH" > /dev/null 2>&1
     exit_code="$?"
     set -e
     if [[ "$exit_code" != "0" ]]; then
-        # shellcheck disable=SC2002
         echo "installing tilt's rsync"
-        cat "$KRSYNC_TAR_PATH" | kubectl exec -i $K8S_OBJECT -- tar -xf - -C $(dirname $RSYNC_TILT_PATH)
+        # shellcheck disable=SC2086,SC2002
+        cat "$KRSYNC_TAR_PATH" | kubectl exec -i $K8S_OBJECT -- tar -xf - -C "$(dirname $RSYNC_TILT_PATH)"
     fi
 }
 
 find_rsync_path() {
     local rsync_path=--rsync-path=$RSYNC_TILT_PATH
     # see if we already have a compatible rsync on the container
+    # shellcheck disable=SC2086,SC2181
     rsync_output=$(kubectl exec $K8S_OBJECT -- rsync --version 2> /dev/null)
+    # shellcheck disable=SC2181
     if [ $? -eq 0 ]; then
         detected_rsync_version=$(echo "$rsync_output" | head -1 | awk '{ print $3 }')
+        # shellcheck disable=SC2071
         if [[ "$detected_rsync_version" > 3 ]]; then
             rsync_path=
         fi
     fi
-    echo $rsync_path
+    echo "$rsync_path"
 }
 
 # Quote/set to the entire first argument, which could contain
@@ -58,6 +62,8 @@ export RSYNC_TILT_PATH="$2"
 export KRSYNC_TAR_PATH="${0%/*}/rsync.tilt.tar"
 shift
 shift
-export RSYNC_PATH=$(find_rsync_path)
+RSYNC_PATH=$(find_rsync_path)
+export RSYNC_PATH
 install_rsync_tilt
-exec rsync $RSYNC_PATH --blocking-io --rsh "$krsync_callback_path" "$@"
\ No newline at end of file
+# shellcheck disable=SC2086
+exec rsync $RSYNC_PATH --blocking-io --rsh "$krsync_callback_path" "$@"
diff --git a/syncback/test/test.sh b/syncback/test/test.sh
index 2cf45204d..24a3e5399 100755
--- a/syncback/test/test.sh
+++ b/syncback/test/test.sh
@@ -1,16 +1,16 @@
 #!/bin/bash
 
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit
 
 set -ex
-tilt ci $@
+tilt ci "$@"
 syncback_dir=$(cat syncback-dir.txt)
-test -f $syncback_dir/python/main.py
-test ! -f $syncback_dir/python/example.txt
-test -f $syncback_dir/ruby/main.rb
-test ! -f $syncback_dir/ruby/example.txt
-test -f $syncback_dir/rsync/rsync.txt
-test ! -f $syncback_dir/rsync/example.txt
+test -f "$syncback_dir"/python/main.py
+test ! -f "$syncback_dir"/python/example.txt
+test -f "$syncback_dir"/ruby/main.rb
+test ! -f "$syncback_dir"/ruby/example.txt
+test -f "$syncback_dir"/rsync/rsync.txt
+test ! -f "$syncback_dir"/rsync/example.txt
 # Test that rsync was copied for containers that needed it
 kubectl exec -n syncback-test -c python deploy/syncback-containers -- sh -c "[ -f /bin/rsync.tilt ]"
 kubectl exec -n syncback-test -c ruby deploy/syncback-containers -- sh -c "[ -f /bin/rsync.tilt ]"
@@ -20,4 +20,4 @@ kubectl exec -n syncback-test -c rsync deploy/syncback-containers -- sh -c "[ !
 test -f syncback-button.txt -a "$(cat syncback-button.txt)" = syncback-containers
 
 tilt down --delete-namespaces
-rm -rf $syncback_dir syncback-dir.txt syncback-button.txt
+rm -rf "$syncback_dir" syncback-dir.txt syncback-button.txt
diff --git a/test.py b/test.py
index 479600c83..a1e658de6 100755
--- a/test.py
+++ b/test.py
@@ -63,6 +63,7 @@
 
 # `git ls-files` instead of `find` to ensure we skip stuff like .git and ./git_resource/test/.git-sources
 files = subprocess.check_output(["git", "ls-files", "**/test.sh"]).decode('utf-8').splitlines()
+tests = []
 for f in files:
 
   # don't re-execute itself
@@ -82,5 +83,8 @@
     print("dry run: %s" % f)
     continue
 
-  print("running %s" % f)
+  tests.append(f)
+
+for count, f in enumerate(tests):
+  print("running %d/%d: %s" % (count + 1, len(tests), f))
   subprocess.check_call([f])
diff --git a/test.sh b/test.sh
index 827805073..61492a694 100755
--- a/test.sh
+++ b/test.sh
@@ -3,5 +3,5 @@
 # find all scripts named 'test.sh' and run them
 # fail immediately if any fail
 
-cd "$(dirname "$0")"
+cd "$(dirname "$0")" || exit 1
 exec python3 ./test.py