From 5cd4eb6b79f252e077f86e0df6976481b10139da Mon Sep 17 00:00:00 2001
From: Daniel Goldstein <danielgold95@gmail.com>
Date: Tue, 10 Oct 2023 21:47:15 -0400
Subject: [PATCH] [linting] Catch unintended errors in check-sql.sh (#13745)

I really borked our SQL linting. This PR is short but it catches a few
critical problems.


1. The point of `check-sql.sh` is to detect modifications or deletions
of SQL files in PRs and fail if such a change occurs. Currently on
`main` it does not detect modifications. In #13456, I removed the
`delete-<service>-tables.sql` files (intentionally), so added the `^D`
to the `grep` regex to indicate that it is OK to have a deletion. What I
inadvertently did though is change the rule from "It's ok to have
Additions of any file OR Modifications of estimated-current.sql /
delete-<service>-tables.sql" to "It's ok to have Additions OR
Modifications OR Deletions of estimated-current.sql /
delete-<service>-tables.sql". Really this should have been "It's ok to
have Additions OR Modifications of estimated-current.sql OR Deletions of
delete-<service>-tables.sql". I've changed it to reflect that rule.

2. Rules currently silently *pass* in CI with an error message that git
is not installed. In #13437 I changed the image used to run the linters
and inadvertently didn't include `git` which `check-sql.sh` needs to
run. Here's how it failed in a sneaky way:
- Since `git` is not installed, all calls to `git` fail, but the script
is not run with `set -e` so every line of the script is executed
- Since `git` lines fail, `modified_sql_file_list` remains empty
- Since `modified_sql_file_list` remains empty, it appears to the check
at the end that everything checked out
- The if statement runs successfully and the script returns with error
code 0

To fix this I do a few things:
- installed `git` in the linting image
- `set -e` by default and only enable `set +e` later on when necessary
(because we don't want a failed `git diff` to immediately exit)
- Do away with the file checking and instead check the error code of the
grep. If nothing survives the grep filter, which means there were no
illegal changes made, grep will return with exit code 1. So we treat
that exit code as a success.
---
 build.yaml                   |  3 ++-
 check-sql.sh                 | 23 ++++++++++++++++-------
 ci/test/resources/build.yaml | 22 ++++++++++++++++++++++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/build.yaml b/build.yaml
index d50b07c42c4..1173cae2f8b 100644
--- a/build.yaml
+++ b/build.yaml
@@ -1462,7 +1462,7 @@ steps:
       inline: |
         ARG BASE_IMAGE={{ hail_ubuntu_image.image }}
         FROM $BASE_IMAGE
-        RUN hail-apt-get-install make
+        RUN hail-apt-get-install git make
         COPY hail/python/pinned-requirements.txt hail-requirements.txt
         COPY hail/python/dev/pinned-requirements.txt dev-requirements.txt
         COPY gear/pinned-requirements.txt gear-requirements.txt
@@ -2791,6 +2791,7 @@ steps:
       cp /io/repo/tls/create_certs.py ./ci/test/resources/
       cp /io/repo/tls/create_test_db_config.sh ./ci/test/resources/
       cp /io/repo/pylintrc ./
+      cp /io/repo/check-sql.sh ./
       cp /io/repo/pyproject.toml ./
       cp -R /io/repo/docker ./
       cp -R /io/repo/gear ./
diff --git a/check-sql.sh b/check-sql.sh
index 8779bce56a9..3c564ba114a 100755
--- a/check-sql.sh
+++ b/check-sql.sh
@@ -1,25 +1,34 @@
 #!/bin/bash
 
-# Verify the the working tree modifices no sql files relative to the main
+set -e
+set -o pipefail
+
+# Verify the the working tree modifies no sql files relative to the main
 # branch. This will always pass on a deploy because the working tree is an
 # unmodified copy of the main branch.
 
 target_treeish=${HAIL_TARGET_SHA:-$(git merge-base main HEAD)}
 
-modified_sql_file_list=$(mktemp)
-
 if [ ! -d sql ]; then
     echo 'No migrations to check, exiting.'
     exit 0
 fi
 
+set +e
 git diff --name-status $target_treeish sql \
-    | grep -Ev $'^A|^M|^D\t[^/]+/sql/(estimated-current.sql|delete-[^ ]+-tables.sql)' \
-           > $modified_sql_file_list
+    | grep -Ev $'^A|^M\t[^/]+/sql/estimated-current.sql|^D\t[^/]+/sql/delete-[^ ]+-tables.sql'
+grep_exit_code=$?
+
 
-if [ "$(cat $modified_sql_file_list | wc -l)" -ne 0 ]
+if [[ $grep_exit_code -eq 0 ]]
 then
-    cat $modified_sql_file_list
     echo 'At least one migration file was modified. See above.'
     exit 1
+elif [[ $grep_exit_code -eq 1 ]]
+then
+    # Exit code 1 means nothing survived grep's filter, so no illegal changes were made
+    # https://www.gnu.org/software/grep/manual/html_node/Exit-Status.html#Exit-Status-1
+    exit 0
 fi
+
+exit $grep_exit_code
diff --git a/ci/test/resources/build.yaml b/ci/test/resources/build.yaml
index 982ccf74c21..6b2d1a60a34 100644
--- a/ci/test/resources/build.yaml
+++ b/ci/test/resources/build.yaml
@@ -219,6 +219,28 @@ steps:
         to: /io/pyproject.toml
     dependsOn:
       - hello_image
+  - kind: runImage
+    name: check_sql_linting
+    image:
+      valueFrom: git_make_bash_image.image
+    script: |
+      set -ex
+      cd /io/repo/ci/test/resources
+      echo "foo" >> sql/create-hello-tables.sql
+
+      set +e
+      bash /io/repo/check-sql.sh
+
+      if [[ $? -eq 0 ]]
+      then
+          echo "check-sql.sh passed when it should have failed"
+      fi
+    inputs:
+      - from: /repo
+        to: /io/repo
+    dependsOn:
+      - git_make_bash_image
+      - merge_code
   - kind: createDatabase2
     name: hello_database
     databaseName: hello