diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index a525db91..8c71d0f6 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -70,14 +70,22 @@ jobs: git diff --name-only --diff-filter=AMR -z "$BASE_SHA"..."$HEAD_SHA" \ -- bios/ > changed_files.zlist + # The report has to reach the pull request whatever the outcome, so the + # exit code is recorded here and acted on after the comment is posted. + # Swallowing it outright left the job green on a file that failed its + # hash check. - name: Validate BIOS files id: validate run: | if [ -s changed_files.zlist ]; then + set +e xargs -0 python scripts/validate_pr.py --markdown -- \ - < changed_files.zlist > report.md 2>&1 || true + < changed_files.zlist > report.md 2>&1 + echo "rc=$?" >> "$GITHUB_OUTPUT" + set -e else echo "No BIOS files changed" > report.md + echo "rc=0" >> "$GITHUB_OUTPUT" fi cat report.md @@ -88,6 +96,12 @@ jobs: env: GH_TOKEN: ${{ github.token }} + - name: Fail when a file did not validate + if: steps.validate.outputs.rc != '0' + run: | + echo "validate_pr.py exited ${{ steps.validate.outputs.rc }}" >&2 + exit 1 + validate-configs: runs-on: ubuntu-latest steps: diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 960511f8..446331f5 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -131,6 +131,41 @@ class WorkflowRegressions(unittest.TestCase): document["on"] = document.pop(True, document.get("on")) return document + def test_no_validation_step_discards_its_exit_code(self): + """A check whose result is thrown away is not a check. + + The BIOS validation step ended in `|| true`, so validate_pr.py could + exit 1 on a file that failed its hash and the job stayed green. The + report still has to reach the pull request, so the code is recorded + and acted on afterwards rather than swallowed. A best-effort side + action such as adding a label is not a check and keeps its `|| true`. + """ + checks = ("python scripts/", "unittest", "mkdocs build") + for name in ("validate.yml", "deploy-site.yml"): + workflow = self._workflow(name) + for job_name, job in workflow["jobs"].items(): + for step in job.get("steps", []): + # A shell continuation puts the command and its `|| true` + # on different lines, so they are rejoined before scanning. + body = str(step.get("run", "")).replace("\\\n", " ") + for line in body.splitlines(): + if not any(marker in line for marker in checks): + continue + self.assertNotIn( + "|| true", + line, + f"{name}:{job_name}:{step.get('name', '?')} runs a " + f"check and discards its exit code: {line.strip()}", + ) + + workflow = self._workflow("validate.yml") + steps = workflow["jobs"]["validate-bios"]["steps"] + gate = [s for s in steps if "rc != " in str(s.get("if", ""))] + self.assertTrue( + gate, + "nothing in validate-bios acts on the validation exit code", + ) + def test_the_suite_runs_on_a_direct_push_to_main(self): workflow = self._workflow("validate.yml") triggers = workflow["on"]