From cd2b4fa1193c8e521a1d5ce5f7e930291a7816a5 Mon Sep 17 00:00:00 2001 From: gak Date: Wed, 31 Jul 2024 11:57:33 +1000 Subject: [PATCH] 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 | 21 ++++- Justfile | 12 ++- lefthook.yml | 2 +- scripts/ensure-frozen-migrations | 41 +++++---- .../tests/test-ensure-frozen-migrations.sh | 92 +++++++++++++++++++ 5 files changed, 144 insertions(+), 24 deletions(-) create mode 100755 scripts/tests/test-ensure-frozen-migrations.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 771d08e930..62f7e9a943 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/Justfile b/Justfile index 5e37f922a0..e3f85dd38a 100644 --- a/Justfile +++ b/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="no-reply@tbd.email" \ + GIT_AUTHOR_NAME="CI" \ + GIT_AUTHOR_EMAIL="no-reply@tbd.email" \ + 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 diff --git a/lefthook.yml b/lefthook.yml index d8ecda5087..70194ed311 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -15,4 +15,4 @@ pre-push: root: frontend/ run: just lint-frontend ensure-frozen-migrations: - run: just ensure-frozen-migrations \ No newline at end of file + run: just ensure-frozen-migrations diff --git a/scripts/ensure-frozen-migrations b/scripts/ensure-frozen-migrations index 6c921f1e88..53ce069073 100755 --- a/scripts/ensure-frozen-migrations +++ b/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 \ No newline at end of file +main diff --git a/scripts/tests/test-ensure-frozen-migrations.sh b/scripts/tests/test-ensure-frozen-migrations.sh new file mode 100755 index 0000000000..bab62af730 --- /dev/null +++ b/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 \ No newline at end of file