ci: test migration ci check bug (#2192)

- Fixed a bug with not treating new files as ignored.
- Cleaned up and added unit tests.

.github/workflows/ci.yml

@@ -36,6 +36,18 @@ jobs:
         run: docker compose up -d --wait
       - name: Test README
         run: just test-readme
+  test-scripts:
+    name: Test Scripts
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - name: Init Hermit
+        uses: cashapp/activate-hermit@v1
+      - name: Test Scripts
+        run: just test-scripts
   sql:
     name: SQL
     runs-on: ubuntu-latest
@@ -62,8 +74,8 @@ jobs:
           fetch-depth: 0
       - name: Init Hermit
         uses: cashapp/activate-hermit@v1
-#      - name: Freeze Migrations
-#        run: just ensure-frozen-migrations
+      - name: Freeze Migrations
+        run: just ensure-frozen-migrations
   lint:
     name: Lint
     runs-on: ubuntu-latest
@@ -87,9 +99,8 @@ jobs:
       # Too annoying to disable individual warnings
       # - name: staticcheck
       #   run: staticcheck ./...
-      - name: shellcheck
-        shell: bash
-        run: shellcheck -f gcc -e SC2016 scripts/* | to-annotation
+      - name: lint-scripts
+        run: just lint-scripts
   proto-breaking:
     name: Proto Breaking Change Check
     runs-on: ubuntu-latest

Justfile

@@ -138,12 +138,19 @@ tidy:
 
 # Check for changes in existing SQL migrations compared to main
 ensure-frozen-migrations:
-  scripts/ensure-frozen-migrations
+  @scripts/ensure-frozen-migrations
 
 # Run backend tests
 test-backend:
   @gotestsum --hide-summary skipped --format-hide-empty-pkg -- -short -fullpath ./...
 
+test-scripts:
+  GIT_COMMITTER_NAME="CI" \
+    GIT_COMMITTER_EMAIL="[email protected]" \
+    GIT_AUTHOR_NAME="CI" \
+    GIT_AUTHOR_EMAIL="[email protected]" \
+    scripts/tests/test-ensure-frozen-migrations.sh
+
 # Lint the frontend
 lint-frontend: build-frontend
   @cd frontend && npm run lint && tsc
@@ -152,6 +159,9 @@ lint-frontend: build-frontend
 lint-backend:
   @golangci-lint run --new-from-rev=$(git merge-base origin/main HEAD) ./...
 
+lint-scripts:
+	@shellcheck -f gcc -e SC2016 $(find scripts -type f -not -path scripts/tests) | to-annotation
+
 # Run live docs server
 docs:
   git submodule update --init --recursive

lefthook.yml

@@ -15,4 +15,4 @@ pre-push:
       root: frontend/
       run: just lint-frontend
     ensure-frozen-migrations:
-      run: just ensure-frozen-migrations
+      run: just ensure-frozen-migrations

scripts/ensure-frozen-migrations

@@ -1,9 +1,6 @@
 #!/bin/bash
 set -euo pipefail
 
-# echo
-set -x
-
 # This script checks if the SQL migration have been modified compared to the common ancestor with the main branch.
 # It ignores comments when comparing the files.
 
@@ -24,39 +21,49 @@ show_diff() {
     diff -u <(remove_comments "$file1") <(remove_comments "$file2")
 }
 
+fail() {
+    echo "❌ Existing migrations changed. Please make sure only comments are changed in existing migration files."
+    exit 1
+}
+
 main() {
     local sql_dir="backend/controller/sql/schema"
     local changed_files
-    local merge_base
 
-    # Find the merge base commit
-    merge_base=$(git merge-base HEAD origin/main)
+    merge_base_sha=$(git merge-base HEAD origin/main)
+    changed_files=$(git diff --name-only "$merge_base_sha" -- "$sql_dir")
 
-    # Get list of changed SQL files compared to the merge base
-    changed_files=$(git diff --name-only "$merge_base" -- "$sql_dir"/*.sql)
+    # Show the diff of the SQL files compared to HEAD to the user
+    git diff --color=always "$merge_base_sha" -- "$sql_dir"/*.sql | cat
 
+    # Track new files added or changed
     if [ -z "$changed_files" ]; then
-        echo "👍 No SQL files changed in $sql_dir compared to the common ancestor with main branch"
+        echo "✅ No files changed in schema directory"
         exit 0
     fi
 
     while IFS= read -r file; do
         if [ ! -f "$file" ]; then
-            echo "$file no longer exists!"
+            echo "❌ $file existed in merge-base of HEAD and main ($merge_base_sha), but has been deleted in head"
             exit 1
         fi
-        if compare_sql_files "$file" <(git show "$merge_base:$file"); then
+
+        # Does the file exist in HEAD?
+        if ! git show "$merge_base_sha:$file" &> /dev/null; then
+            echo "✅ New: $file"
+            continue
+        fi
+
+        if compare_sql_files "$file" <(git show "$merge_base_sha:$file"); then
             : # Do nothing if files are the same
         else
             echo "❌ Migration files changes detected"
-            show_diff "$file" <(git show "$merge_base:$file")
-            exit 1
+            show_diff "$file" <(git show "$merge_base_sha:$file")
+            fail
         fi
     done <<< "$changed_files"
 
-    git diff --color=always "$merge_base" -- "$sql_dir"/*.sql | cat
-    echo "👍 Only comments/new lines changed in SQL files"
+    echo "✅ No schema changes detected for existing migration files"
 }
 
-# Run the main function
-main
+main

scripts/tests/test-ensure-frozen-migrations.sh

@@ -0,0 +1,92 @@
+#!/bin/bash
+set -euo pipefail
+set -x
+
+script_path='scripts/ensure-frozen-migrations'
+schema_dir='backend/controller/sql/schema'
+
+function create() {
+  echo "CREATE TABLE testing (id SERIAL PRIMARY KEY);" > "$schema_dir/testing.sql"
+}
+
+function remove_existing() {
+  rm "$schema_dir/20240704103403_create_module_secrets.sql"
+}
+
+function modify_comment_in_existing() {
+  sed -i.bak 's/Function for deployment notifications./🧐/' "$schema_dir/20231103205514_init.sql"
+  rm "$schema_dir/20231103205514_init.sql.bak"
+}
+
+function modify_content_in_existing() {
+  sed -i.bak 's/CREATE OR REPLACE FUNCTION notify_event()/CREATE 🥲 notify_event()/' "$schema_dir/20231103205514_init.sql"
+  rm "$schema_dir/20231103205514_init.sql.bak"
+}
+
+function no_changes() {
+  echo "no-changes" >> .gitignore
+  git add .gitignore
+}
+
+function commit_migrations_dir() {
+  git add "$schema_dir"
+  git commit -m "ci: automated test commit, this should not be pushed!"
+}
+
+
+# higher order test function, accepting a function as an argument, and an expected fail or pass.
+# omitted means it should pass.
+# fail should be used when the test is expected to fail.
+function test() {
+  local test_function=$1
+  local expected_result=${2:-pass}
+  local saved_commit
+  saved_commit=$(git rev-parse HEAD)
+
+  echo "---- $test_function -------------------------"
+
+  $test_function
+
+  git status
+  commit_migrations_dir
+
+  local did_fail=false
+  if ! $script_path; then
+    if [ "$expected_result" == "pass" ]; then
+      echo "❌ $test_function expected to pass but failed"
+      did_fail=true
+    fi
+  else
+    if [ "$expected_result" == "fail" ]; then
+      echo "❌ $test_function expected to fail but passed"
+      did_fail=true
+    fi
+  fi
+
+  # only clean up the schema dir
+  rm "$schema_dir"/*
+  git reset "$saved_commit"
+  git checkout .gitignore "$schema_dir"
+
+  if $did_fail; then
+    echo "❌ FAIL $test_function"
+    exit 1
+  else
+    echo "✅ PASS $test_function"
+  fi
+}
+
+function main() {
+  # cd into the project root
+  cd "$(git rev-parse --show-toplevel)" || exit 1
+
+  test no_changes
+  test create
+  test remove_existing fail
+  test modify_comment_in_existing
+  test modify_content_in_existing fail
+
+  echo "✅ All tests passed 🎉"
+}
+
+main