Merge pull request #21891 from edsantiago/no-new-tests-label

CI: must-add-tests check: use GH label, not text

CONTRIBUTING.md

@@ -164,9 +164,9 @@ Regardless of the type of PR, all PRs should include:
 * well documented code changes.
 * additional testcases. Ideally, they should fail w/o your code change applied.
   (With a few exceptions, CI hooks will block your PR unless your change
-  includes files named `*_test.go` or under the `test/` subdirectory. To
-  bypass this block, include the string `[NO NEW TESTS NEEDED]` in your
-  commit message).
+  includes files named `*_test.go` or under the `test/` subdirectory. Repo
+  admins may bypass this restriction by setting the 'No New Tests' GitHub
+  label on the PR).
 * documentation changes.
 
 Squash your commits into logical pieces of work that might want to be reviewed

contrib/cirrus/pr-should-include-tests

@@ -2,17 +2,16 @@
 #
 # Intended for use in CI: check git commits, barf if no tests added.
 #
+ME=$(basename $0)
+
+# Github label which allows overriding this check
+OVERRIDE_LABEL="No New Tests"
 
 # Docs-only changes are excused
 if [[ "${CIRRUS_CHANGE_TITLE}" =~ CI:DOCS ]]; then
     exit 0
 fi
 
-# So are PRs where 'NO NEW TESTS NEEDED' appears in the Github message
-if [[ "${CIRRUS_CHANGE_MESSAGE}" =~ NO.NEW.TESTS.NEEDED ]]; then
-    exit 0
-fi
-
 # HEAD should be good enough, but the CIRRUS envariable allows us to test
 head=${CIRRUS_CHANGE_IN_REPO:-HEAD}
 # Base of this PR. Here we absolutely rely on cirrus.
@@ -51,14 +50,41 @@ if [[ -z "$filtered_changes" ]]; then
     exit 0
 fi
 
-# One last chance: perhaps the developer included the magic '[NO NEW TESTS NEEDED]'
-# string in an amended commit.
-if git log --format=%B ${base}..${head} | grep -F '[NO NEW TESTS NEEDED]'; then
-   exit 0
+# Nope. Only allow if the github 'no-tests-needed' label is set
+if [[ -z "$CIRRUS_PR" ]]; then
+    echo "$ME: cannot query github: \$CIRRUS_PR is undefined" >&2
+    exit 1
+fi
+if [[ -z "$CIRRUS_REPO_CLONE_TOKEN" ]]; then
+    echo "$ME: cannot query github: \$CIRRUS_REPO_CLONE_TOKEN is undefined" >&2
+    exit 1
+fi
+
+query="{
+  \"query\": \"query {
+  repository(owner: \\\"containers\\\", name: \\\"podman\\\") {
+    pullRequest(number: $CIRRUS_PR) {
+      labels(first: 100) {
+        nodes {
+          name
+        }
+      }
+    }
+  }
+}\"
+}"
+
+result=$(curl -s -H "Authorization: bearer $CIRRUS_REPO_CLONE_TOKEN" -H "Accept: application/vnd.github.antiope-preview+json" -H "Content-Type: application/json" -X POST --data @- https://api.github.com/graphql <<<"$query")
+
+labels=$(jq -r '.data.repository.pullRequest.labels.nodes[].name' <<<"$result")
+
+if grep -F -x -q "$OVERRIDE_LABEL" <<<"$labels"; then
+    # PR has the label set
+    exit 0
 fi
 
 cat <<EOF
-$(basename $0): PR does not include changes in the 'tests' directory
+$ME: PR does not include changes in the 'tests' directory
 
 Please write a regression test for what you're fixing. Even if it
 seems trivial or obvious, try to add a test that will prevent
@@ -69,8 +95,8 @@ tests, possibly just adding a small step to a similar existing test.
 Every second counts in CI.
 
 If your commit really, truly does not need tests, you can proceed
-by adding '[NO NEW TESTS NEEDED]' to the body of your commit message.
-Please think carefully before doing so.
+by asking a repo maintainer to set the '$OVERRIDE_LABEL' github label.
+This will only be done when there's no reasonable alternative.
 EOF
 
 exit 1

contrib/cirrus/pr-should-include-tests.t

@@ -7,6 +7,13 @@
 #
 ME=$(basename $0)
 
+# As of 2024-02 our test script queries github, for which we need token
+if [[ -z "$GITHUB_TOKEN" ]]; then
+    echo "$ME: Please set \$GITHUB_TOKEN" >&2
+    exit 1
+fi
+export CIRRUS_REPO_CLONE_TOKEN="$GITHUB_TOKEN"
+
 ###############################################################################
 # BEGIN test cases
 #
@@ -26,19 +33,19 @@ ME=$(basename $0)
 #        commit history, but once we do, please add a new '0' test here.
 #
 tests="
-0  68c9e02df  db71759b1   PR 8821: multiple commits, includes tests
-0  bb82c37b7  eeb4c129b   PR 8832: single commit, w/tests, merge-base test
-1  1f5927699  864592c74   PR 8685, multiple commits, no tests
-0  7592f8fbb  6bbe54f2b   PR 8766, no tests, but CI:DOCS in commit message
-0  355e38769  bfbd915d6   PR 8884, a vendor bump
-0  ffe2b1e95  e467400eb   PR 8899, only .cirrus.yml
-0  06a6fd9f2  3cc080151   PR 8695, docs-only, without CI:DOCS
-0  a47515008  ecedda63a   PR 8816, unit tests only
-0  caa84cd35  e55320efd   PR 8565, hack/podman-socat only
-0  c342583da  12f835d12   PR 8523, version.go + podman.spec.in
-0  8f75ed958  7b3ad6d89   PR 8835, only a README.md change
-0  b6db60e58  f06dd45e0   PR 9420, a test rename
-0  c6a896b0c  4ea5d6971   PR 11833, includes magic string
+0  68c9e02df  db71759b1   8821  multiple commits, includes tests
+0  bb82c37b7  eeb4c129b   8832  single commit, w/tests, merge-base test
+1  1f5927699  864592c74   8685  multiple commits, no tests
+0  7592f8fbb  6bbe54f2b   8766  no tests, but CI:DOCS in commit message
+0  355e38769  bfbd915d6   8884  a vendor bump
+0  ffe2b1e95  e467400eb   8899  only .cirrus.yml
+0  06a6fd9f2  3cc080151   8695  docs-only, without CI:DOCS
+0  a47515008  ecedda63a   8816  unit tests only
+0  caa84cd35  e55320efd   8565  hack/podman-socat only
+0  c342583da  12f835d12   8523  version.go + podman.spec.in
+0  8f75ed958  7b3ad6d89   8835  only a README.md change
+0  b6db60e58  f06dd45e0   9420  a test rename
+0  c6a896b0c  4ea5d6971  11833  includes magic string
 "
 
 # The script we're testing
@@ -76,10 +83,10 @@ function run_test_script() {
                 echo "# Actual:   $output"
                 rc=1
             else
-                echo "ok $testnum $testname"
+                echo "ok $testnum $testname - rc=$expected_rc"
             fi
         else
-            echo "ok $testnum $testname"
+            echo "ok $testnum $testname - rc=$expected_rc"
         fi
     fi
 
@@ -97,15 +104,6 @@ function run_test_script() {
                 echo "ok $testnum $rest (override with CI:DOCS)"
             fi
 
-            testnum=$(( testnum + 1 ))
-            CIRRUS_CHANGE_MESSAGE="hi there [NO TESTS NEEDED] bye" $test_script &>/dev/null
-            if [[ $? -ne 0 ]]; then
-                echo "not ok $testnum $rest (override with '[NO TESTS NEEDED]')"
-                rc=1
-            else
-                echo "ok $testnum $rest (override with '[NO TESTS NEEDED]')"
-            fi
-
             tested_override=1
         fi
     fi
@@ -119,16 +117,17 @@ rc=0
 testnum=0
 tested_override=
 
-while read expected_rc parent_sha  commit_sha rest; do
+while read expected_rc parent_sha  commit_sha pr rest; do
     # Skip blank lines
     test -z "$expected_rc" && continue
 
     export DEST_BRANCH=$parent_sha
     export CIRRUS_CHANGE_IN_REPO=$commit_sha
     export CIRRUS_CHANGE_TITLE=$(git log -1 --format=%s $commit_sha)
     export CIRRUS_CHANGE_MESSAGE=
+    export CIRRUS_PR=$pr
 
-    run_test_script $expected_rc "$rest"
+    run_test_script $expected_rc "PR $pr - $rest"
 done <<<"$tests"
 
 echo "1..$testnum"