ci: make Lint's actionlint step blocking (#1967)

.github/workflows/lint.yml

@@ -5,12 +5,13 @@ name: "Lint"
 # them: it appears only in cron-vendor.yml, as a formatter that rewrites the
 # generated constants, not as a check.
 #
-# Every step is advisory (`continue-on-error: true`): none is clean, and a job
-# that is red the day it lands teaches everyone to scroll past red. Promote a
-# step by deleting its `continue-on-error` once its count reaches zero. Each
-# step writes its result to the run summary; read the numbers there. This
-# header records no counts on purpose: most of those #753 recorded had drifted
-# by the time it was filed.
+# Every step but actionlint is advisory (`continue-on-error: true`): none is
+# clean, and a job that is red the day it lands teaches everyone to scroll past
+# red. Promote a step by deleting its `continue-on-error` once its count reaches
+# zero, and move it below the advisory steps, as a failed step skips every step
+# after it. Each step writes its result to the run summary; read the numbers
+# there. This header records no counts on purpose: most of those #753 recorded
+# had drifted by the time it was filed.
 #
 # pylint's R class moves without the code moving: `R0401` cyclic-import
 # depends on module-processing order, so runs on one tree disagree on it. A
@@ -36,9 +37,9 @@ name: "Lint"
 #     `no-member` false positives off the same metaprogramming.
 
 on:
-  # No `push: [main]`: these steps gate nothing, so a second advisory run per
-  # merge, on a commit its pull request already linted, buys no information
-  # while competing for oversubscribed runners. The schedule covers main's drift.
+  # No `push: [main]`: a second run per merge, on a commit its pull request
+  # already linted, buys no information while competing for oversubscribed
+  # runners. The schedule covers main's drift.
   pull_request:
     branches: [main]
   schedule:
@@ -64,52 +65,6 @@ jobs:
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
 
-      # actionlint checks every workflow, running the image's shellcheck over
-      # each `run:` block. First, as it needs no Python. It is advisory; whether
-      # it should block now that it reports nothing is open on #1967.
-      #
-      # The release binary is pinned by version and sha256, from the release's
-      # checksums file. Dependabot does not see this pin; bump both together.
-      # The workflows use YAML anchors, which actionlint reads from 1.7.10 on.
-      #
-      # The `-ignore` hides a false positive: `concurrency.queue` is a documented
-      # GitHub key (project-status.yml uses it) that no actionlint release knows
-      # yet (rhysd/actionlint#657, fixed by rhysd/actionlint#654). Drop the
-      # pattern once a release with that fix is pinned.
-      - name: actionlint
-        continue-on-error: true
-        timeout-minutes: 5
-        env:
-          ACTIONLINT_VERSION: 1.7.12
-          ACTIONLINT_SHA256: 8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8
-        run: |
-          archive="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz"
-          curl -fsSL --retry 3 -o "$RUNNER_TEMP/$archive" \
-            "https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}/$archive"
-          echo "$ACTIONLINT_SHA256  $RUNNER_TEMP/$archive" | sha256sum -c -
-          tar -xzf "$RUNNER_TEMP/$archive" -C "$RUNNER_TEMP" actionlint
-          "$RUNNER_TEMP/actionlint" -version
-
-          set +e
-          "$RUNNER_TEMP/actionlint" -oneline -no-color \
-            -ignore '^unexpected key "queue" for "concurrency" section\.' 2>&1 | tee actionlint.log
-          status=${PIPESTATUS[0]}
-          set -e
-
-          # Without pipefail a pipeline's status is uniq's, so an empty count
-          # is caught by its output, not its status.
-          rules=$(grep -oE '\[[a-z-]+\]$' actionlint.log | sort | uniq -c)
-          shellcheck=$(grep -oE 'SC[0-9]+:[a-z]+' actionlint.log | sort | uniq -c)
-          {
-            echo '### actionlint'
-            echo '```'
-            printf 'findings by rule:\n%s\n' "${rules:-none}"
-            printf 'shellcheck findings by code:\n%s\n' "${shellcheck:-none}"
-            echo '```'
-          } >> "$GITHUB_STEP_SUMMARY"
-
-          exit "$status"
-
       # One interpreter, not a matrix: these tools read the source, so more
       # interpreters would only repeat the same findings. `latest` from
       # .github/python-versions.toml, like every single-interpreter job (#1661).
@@ -203,8 +158,8 @@ jobs:
 
           exit "$status"
 
-      # Last because it is by far the slowest, so the quick verdicts reach the
-      # summary first.
+      # Last of the advisory steps because it is by far the slowest, so the
+      # quick verdicts reach the summary first.
       - name: pylint
         continue-on-error: true
         run: |
@@ -222,3 +177,57 @@ jobs:
           } >> "$GITHUB_STEP_SUMMARY"
 
           exit "$status"
+
+      # actionlint checks every workflow, running the image's shellcheck over
+      # each `run:` block. It blocks, as #1967 ruled: a finding fails Lint. That
+      # is safe because #2023, #2037 and #2038 fixed every finding it reported.
+      # It runs last, though it needs no Python, so that a finding skips none of
+      # the advisory steps above, and on `!cancelled()`, so that a failed Python
+      # setup does not skip it. Lint is not among ruleset 23497679's required
+      # checks, so a red Lint does not hold a merge.
+      #
+      # The image's shellcheck is not pinned: a runner image that adds a check
+      # can flag unchanged code and turn Lint red on an unrelated pull request.
+      # Fix the script it flags; that is what the check is for.
+      #
+      # The release binary is pinned by version and sha256, from the release's
+      # checksums file. Dependabot does not see this pin; bump both together.
+      # The workflows use YAML anchors, which actionlint reads from 1.7.10 on.
+      #
+      # The `-ignore` hides a false positive: `concurrency.queue` is a documented
+      # GitHub key (project-status.yml uses it) that no actionlint release knows
+      # yet (rhysd/actionlint#657, fixed by rhysd/actionlint#654). Drop the
+      # pattern once a release with that fix is pinned.
+      - name: actionlint
+        if: ${{ !cancelled() }}
+        timeout-minutes: 5
+        env:
+          ACTIONLINT_VERSION: 1.7.12
+          ACTIONLINT_SHA256: 8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8
+        run: |
+          archive="actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz"
+          curl -fsSL --retry 3 -o "$RUNNER_TEMP/$archive" \
+            "https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}/$archive"
+          echo "$ACTIONLINT_SHA256  $RUNNER_TEMP/$archive" | sha256sum -c -
+          tar -xzf "$RUNNER_TEMP/$archive" -C "$RUNNER_TEMP" actionlint
+          "$RUNNER_TEMP/actionlint" -version
+
+          set +e
+          "$RUNNER_TEMP/actionlint" -oneline -no-color \
+            -ignore '^unexpected key "queue" for "concurrency" section\.' 2>&1 | tee actionlint.log
+          status=${PIPESTATUS[0]}
+          set -e
+
+          # Without pipefail a pipeline's status is uniq's, so an empty count
+          # is caught by its output, not its status.
+          rules=$(grep -oE '\[[a-z-]+\]$' actionlint.log | sort | uniq -c)
+          shellcheck=$(grep -oE 'SC[0-9]+:[a-z]+' actionlint.log | sort | uniq -c)
+          {
+            echo '### actionlint'
+            echo '```'
+            printf 'findings by rule:\n%s\n' "${rules:-none}"
+            printf 'shellcheck findings by code:\n%s\n' "${shellcheck:-none}"
+            echo '```'
+          } >> "$GITHUB_STEP_SUMMARY"
+
+          exit "$status"

docs/source/contributing/workflows.rst

@@ -1346,16 +1346,21 @@ that names a particular action.
 Workflow Lint (`#1967 <https://github.com/JarryShaw/PyPCAPKit/issues/1967>`__)
 ------------------------------------------------------------------------------
 
-Lint's ``lint`` job runs actionlint over every workflow in its ``actionlint`` step,
-first, as it needs no Python. The step downloads the release that
-``ACTIONLINT_VERSION`` names and checks it against ``ACTIONLINT_SHA256``, from that
-release's checksums file, before unpacking it. Dependabot does not read this pin,
-so a bump changes both. The workflows use YAML anchors, which actionlint reads from
+Lint's ``lint`` job runs actionlint over every workflow in its ``actionlint`` step.
+The step downloads the release that ``ACTIONLINT_VERSION`` names and checks it
+against ``ACTIONLINT_SHA256``, from that release's checksums file, before unpacking
+it. Dependabot does not read this pin, so a bump changes both. The workflows use YAML anchors, which actionlint reads from
 1.7.10 on, and the pin is 1.7.12 or newer.
 
-actionlint runs the image's shellcheck over each ``run:`` block, and shellcheck
-still reports style and info findings, so the step is advisory like the others in
-Lint. Its run summary counts the findings by actionlint rule and by shellcheck code.
+actionlint runs the image's shellcheck over each ``run:`` block. Unlike Lint's
+other steps, this one blocks: a finding fails the job, as the owner ruled on #1967
+once #2023, #2037 and #2038 had fixed every finding it reported. It runs last and
+on ``!cancelled()``, so a finding skips none of the advisory steps and a failed
+Python setup does not skip it. Lint is not a required check of ruleset 23497679, so
+a red Lint does not hold a merge. The image's shellcheck is not pinned, so a runner
+image that adds a check can turn Lint red on unchanged code; the fix is to the
+script it flags. The step's run summary counts the findings by actionlint rule and
+by shellcheck code.
 
 The step's ``-ignore`` pattern, which matches a message, not a file, hides a false
 positive: ``concurrency.queue``, which ``project-status.yml`` sets and GitHub
@@ -1364,9 +1369,11 @@ documents, is a key no actionlint release knows yet (`rhysd/actionlint#657
 release that knows it is pinned.
 
 :file:`tests/project/test_actionlint_in_lint_1967.py` fails when the step is gone,
-pins an older release or no checksum, unpacks before it verifies, or passes an
-``-ignore`` pattern other than that, and when no workflow sets
-``concurrency.queue`` any more.
+pins an older release or no checksum, unpacks before it verifies, passes an
+``-ignore`` pattern other than that, or stops blocking: when it carries
+``continue-on-error``, exits with a status other than actionlint's, or sits above
+an advisory step or a failed setup would skip it. It fails too when no workflow
+sets ``concurrency.queue`` any more.
 
 Dependency Updates (`#1968 <https://github.com/JarryShaw/PyPCAPKit/issues/1968>`__)
 -----------------------------------------------------------------------------------

tests/project/test_actionlint_in_lint_1967.py

@@ -9,6 +9,11 @@
 
 * :class:`TestTheStep` pins the step: one of it, the version, the checksum, and
   the order download, verify, unpack, run.
+* :class:`TestItBlocks` pins that a finding fails Lint, as the owner ruled on
+  #1967 once #2023, #2037 and #2038 had fixed every finding: no
+  ``continue-on-error``, actionlint's own exit status, and a place below every
+  advisory step, run on ``!cancelled()``, so a finding skips none of them and a
+  failed setup does not skip it.
 * :class:`TestTheIgnores` pins each ``-ignore`` pattern to the false positive it
   exists for, so a blanket pattern fails here, and checks that a workflow still
   uses the key, so the pattern goes when the key does.
@@ -50,6 +55,8 @@
         ),
     ),
 }
+#: The ``if:`` values under which a step still runs after an earlier step failed.
+RUNS_AFTER_A_FAILURE = frozenset({'${{ !cancelled() }}', '${{ always() }}', '!cancelled()', 'always()'})
 #: An ``-ignore`` argument in the step's script, quoted either way.
 IGNORE = re.compile(r'''-ignore\s+(?P<quote>['"])(?P<pattern>.*?)(?P=quote)''')
 
@@ -101,6 +108,33 @@ def test_it_bounds_its_time(self) -> None:
         self.assertLess(minutes, _workflows.job(WORKFLOW, JOB)['timeout-minutes'])
 
 
+class TestItBlocks(unittest.TestCase):
+    """An actionlint finding fails Lint, and skips none of Lint's advisory steps."""
+
+    def test_it_is_not_continue_on_error(self) -> None:
+        self.assertNotIn('continue-on-error', step())
+
+    def test_it_exits_with_actionlints_status(self) -> None:
+        script = step()['run']
+        # The command with its continuation lines, up to the end of its last line.
+        run = re.search(r'"\$RUNNER_TEMP/actionlint" -oneline\b(?:[^\n]*\\\n)*[^\n]*\n', script)
+        self.assertIsNotNone(run, 'the step does not run actionlint')
+        self.assertNotRegex(run[0], r'\|\|')  # type: ignore[index]
+        after = script[run.end():].splitlines()  # type: ignore[union-attr]
+        self.assertEqual(after[0].strip(), 'status=${PIPESTATUS[0]}')
+        self.assertEqual([line for line in after if line.strip()][-1].strip(), 'exit "$status"')
+
+    def test_it_runs_after_a_failed_setup(self) -> None:
+        self.assertIn(step().get('if'), RUNS_AFTER_A_FAILURE)
+
+    def test_no_step_after_it_is_skipped_by_a_finding(self) -> None:
+        steps = _workflows.steps(WORKFLOW, JOB)
+        index = next(i for i, entry in enumerate(steps) if entry.get('name') == STEP)
+        skipped = [entry.get('name') or entry.get('uses') for entry in steps[index + 1:]
+                   if entry.get('if') not in RUNS_AFTER_A_FAILURE]
+        self.assertEqual(skipped, [], 'a finding would skip these steps; move actionlint below them')
+
+
 class TestTheIgnores(unittest.TestCase):
     """Each ``-ignore`` hides one known false positive, and only while it can occur."""
 

tests/project/test_unit_tests_shellcheck_1967.py

@@ -2,8 +2,8 @@
 """#1967: every ``run:`` block of unit-tests.yml passes shellcheck.
 
 Lint's ``actionlint`` step runs shellcheck over every workflow's ``run:`` blocks,
-but is advisory. Step 5 of #1967 fixed unit-tests.yml's findings at the root, so
-this test keeps it clean: it checks each
+and a finding fails Lint. Step 5 of #1967 fixed unit-tests.yml's findings at the
+root, and this test keeps it clean without waiting for a Lint run: it checks each
 ``run:`` block of a ``bash`` or ``sh`` step as actionlint does: each
 ``${{ ... }}`` expression replaced by underscores of the same length, the
 shell's ``set -e`` line above it, and the checks actionlint disables left out.