Fix shellcheck CI job, fix/handle all shellcheck findings (#620)

* Fix shellcheck linting Signed-off-by: Daniel Bast <[email protected]> * Fix/handle all shellcheck findings Signed-off-by: Daniel Bast <[email protected]> * Incorporate review findings Signed-off-by: Daniel Bast <[email protected]> * Switch to pre-commit Signed-off-by: Daniel Bast <[email protected]> --------- Signed-off-by: Daniel Bast <[email protected]>
tilt-dev · Dec 8, 2024 · 20444a6 · 20444a6
1 parent a0787e5
commit 20444a6
Show file tree

Hide file tree

Showing 20 changed files with 91 additions and 39 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -9,13 +9,23 @@ jobs:
       - run: GOBIN=$HOME/.bin go install 'github.com/tilt-dev/[email protected]'
       - 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
@@ -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
@@ -1,11 +1,12 @@
-.PHONY: test publish-ci-image
+SHELL := /bin/bash -o pipefail -o errexit
 
 test:
 	./test.sh
 
 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
@@ -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
@@ -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
@@ -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
diff --git a/kubectl_build/Tiltfile b/kubectl_build/Tiltfile
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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" "$@"
+# 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
@@ -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
@@ -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
@@ -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