From e54eeaad2c4f1d562457db767c3418dc662b1455 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 22 Oct 2026 11:13:07 -0400 Subject: [PATCH] ci: make Lint's actionlint step blocking (#1967) - lint.yml: drop the actionlint step's `continue-on-error`, so a finding fails Lint; move it below the advisory steps and run it on `!cancelled()` so a finding skips none of them and a failed setup does not skip it. Version and sha256 pin unchanged. - lint.yml: the step's comment says it blocks, why that is safe now (#2023, #2037, #2038), and that the image's shellcheck can drift. - workflows.rst: Workflow Lint no longer calls the step advisory. - test_actionlint_in_lint_1967: TestItBlocks pins the blocking shape; test_unit_tests_shellcheck_1967's docstring no longer says advisory. --- .github/workflows/lint.yml | 123 ++++++++++-------- docs/source/contributing/workflows.rst | 29 +++-- tests/project/test_actionlint_in_lint_1967.py | 34 +++++ .../test_unit_tests_shellcheck_1967.py | 4 +- 4 files changed, 120 insertions(+), 70 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index dc875f110e..b96c6dbbc1 100644 --- a/.github/workflows/lint.yml +++ b/.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" diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 44d5984327..d9f4ae4368 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -1346,16 +1346,21 @@ that names a particular action. Workflow Lint (`#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 `__) ----------------------------------------------------------------------------------- diff --git a/tests/project/test_actionlint_in_lint_1967.py b/tests/project/test_actionlint_in_lint_1967.py index 3d41457b57..59753e5532 100644 --- a/tests/project/test_actionlint_in_lint_1967.py +++ b/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['"])(?P.*?)(?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.""" diff --git a/tests/project/test_unit_tests_shellcheck_1967.py b/tests/project/test_unit_tests_shellcheck_1967.py index a5314babbb..39fd508f6b 100644 --- a/tests/project/test_unit_tests_shellcheck_1967.py +++ b/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.