diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml new file mode 100644 index 000000000..90b61a49e --- /dev/null +++ b/.github/actionlint.yaml @@ -0,0 +1,11 @@ +# actionlint reads this file from .github/ on every run, in CI and locally. +# Format: https://github.com/rhysd/actionlint/blob/v1.7.12/docs/config.md +paths: + .github/workflows/push.yaml: + ignore: + # Both come from the Test job's "Deploy documentation" step, which + # PolicyEngine/policyengine-core#594 moves into its own Docs job and + # fixes. Delete these two entries in whichever PR fixes or removes that + # step. + - 'property "os" is not defined in object type \{\}' + - 'input "GITHUB_TOKEN" is not defined in action "JamesIves/github-pages-deploy-action@v4"' diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index a52d09218..1eed1b705 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -11,6 +11,21 @@ jobs: uses: astral-sh/setup-uv@v8.1.0 - name: Check formatting run: uvx ruff format --check . + lint-workflows: + name: Lint workflows + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v6 + with: + persist-credentials: false + - name: Run actionlint + # rhysd/actionlint 1.7.12. The image bundles shellcheck and pyflakes, + # which actionlint runs on `run:` scripts. + uses: docker://rhysd/actionlint@sha256:b1934ee5f1c509618f2508e6eb47ee0d3520686341fec936f3b79331f9315667 + with: + args: -color check-changelog: name: Check changelog fragment runs-on: ubuntu-latest diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index 8605abf3c..0bc97cf7b 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -42,7 +42,7 @@ jobs: run: | uv pip install --system towncrier python .github/bump_version.py - towncrier build --yes --version $(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))") + towncrier build --yes --version "$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))")" - name: Preview changelog update run: ".github/get-changelog-diff.sh" - name: Update changelog @@ -109,6 +109,6 @@ jobs: with: user: __token__ password: ${{ secrets.PYPI }} - skip_existing: true + skip-existing: true - name: Publish a git tag run: bash .github/publish-git-tag.sh diff --git a/Makefile b/Makefile index e774214b3..abf910541 100644 --- a/Makefile +++ b/Makefile @@ -27,4 +27,4 @@ build: changelog: python .github/bump_version.py - towncrier build --yes --version $$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))") \ No newline at end of file + towncrier build --yes --version "$$(python -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))")" \ No newline at end of file diff --git a/README.md b/README.md index 8c6b88f9a..b92c86625 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,11 @@ uv run pytest tests/core/test_file.py::test_name -v # Format before committing (CI enforces) make format + +# Lint GitHub Actions workflows after editing .github/, from the repo root +# (CI enforces; install shellcheck and pyflakes so actionlint also checks +# run: scripts) +uvx --from actionlint-py==1.7.12.25 actionlint ``` **Always use `uv run`** for Python commands; bare `python` / `pytest` / `pip` may pick up the wrong environment. diff --git a/changelog.d/ci-actionlint.changed.md b/changelog.d/ci-actionlint.changed.md new file mode 100644 index 000000000..fab3f8947 --- /dev/null +++ b/changelog.d/ci-actionlint.changed.md @@ -0,0 +1 @@ +Pull requests now run actionlint on the GitHub Actions workflows, and the release workflow quotes the version it passes to towncrier and uses the `skip-existing` input of the PyPI publish action. diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index 64b88f3f3..12762cbfd 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -4,6 +4,7 @@ from pathlib import Path import pytest +import yaml ROOT = Path(__file__).resolve().parents[2] @@ -144,13 +145,167 @@ def test_release_tag_script_propagates_push_failure(tmp_path: Path): assert "origin" in result.stderr +def only_step(steps: list[dict], matches, description: str) -> int: + indices = [index for index, step in enumerate(steps) if matches(step)] + assert len(indices) == 1, f"expected one {description} step, found {indices}" + return indices[0] + + +def assert_publish_tags_only_after_pypi_succeeds(publish: dict) -> None: + steps = publish["steps"] + checkout = steps[ + only_step( + steps, + lambda step: step.get("uses", "").startswith("actions/checkout@"), + "checkout", + ) + ] + pypi_index = only_step( + steps, + lambda step: step.get("uses", "").startswith("pypa/gh-action-pypi-publish@"), + "PyPI publish", + ) + tag_index = only_step( + steps, + lambda step: ".github/publish-git-tag.sh" in step.get("run", ""), + "git tag", + ) + pypi_step = steps[pypi_index] + tag_step = steps[tag_index] + + # The tag script pushes a tag, so Publish itself needs write access. + # Permissions granted to any other job do not apply to it. + permissions = publish.get("permissions", {}) + assert permissions == "write-all" or ( + isinstance(permissions, dict) and permissions.get("contents") == "write" + ), "Publish lacks contents: write" + # The script looks for an existing tag locally, and a checkout with + # fetch-depth 0 fetches every tag. Action inputs are strings, so 0 and "0" + # are the same input. + assert str(checkout.get("with", {}).get("fetch-depth")) == "0", ( + "checkout is not fetch-depth 0" + ) + # Steps run in order and a failure skips the rest, so the tag step runs + # only after a successful upload. A condition on either step, or + # continue-on-error on the upload, could let it run anyway: a skipped + # step does not fail the job. + assert pypi_index < tag_index, "tag step precedes the PyPI step" + assert "if" not in pypi_step, "PyPI step has a condition" + assert "if" not in tag_step, "tag step has a condition" + assert pypi_step.get("continue-on-error", False) is False, ( + "PyPI step continues on error" + ) + # A failed tag push must fail the job, not leave an untagged release, so + # the step runs the script alone: nothing such as `|| true` may follow it. + assert tag_step.get("continue-on-error", False) is False, ( + "tag step continues on error" + ) + assert tag_step["run"].strip() == "bash .github/publish-git-tag.sh", ( + "tag step runs more than the tag script" + ) + + def test_publish_workflow_tags_only_after_pypi_succeeds(): - workflow = PUSH_WORKFLOW.read_text() - publish_job = workflow.split(" Publish:\n", maxsplit=1)[1] - publish_step = "uses: pypa/gh-action-pypi-publish@release/v1" - tag_step = "run: bash .github/publish-git-tag.sh" - - assert "permissions:\n contents: write" in publish_job - assert "fetch-depth: 0" in publish_job - assert publish_job.index(tag_step) > publish_job.index(publish_step) - assert "publish-git-tag.sh || true" not in publish_job + publish = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"]["Publish"] + + assert_publish_tags_only_after_pypi_succeeds(publish) + + +def drop_publish_permissions_and_add_later_writer(jobs: dict) -> None: + # A text slice from " Publish:" to the end of the file read the later + # job's permissions as Publish's and passed this workflow. + del jobs["Publish"]["permissions"] + jobs["Docs"] = { + "runs-on": "ubuntu-latest", + "permissions": {"contents": "write"}, + "steps": [{"run": "make documentation"}], + } + + +def find_step(jobs: dict, fragment: str) -> dict: + return next( + step + for step in jobs["Publish"]["steps"] + if fragment in step.get("uses", "") + step.get("run", "") + ) + + +def shallow_checkout(jobs: dict) -> None: + del find_step(jobs, "actions/checkout@")["with"]["fetch-depth"] + + +def tag_before_pypi(jobs: dict) -> None: + steps = jobs["Publish"]["steps"] + tag_step = find_step(jobs, "publish-git-tag.sh") + steps.remove(tag_step) + steps.insert(steps.index(find_step(jobs, "gh-action-pypi-publish")), tag_step) + + +def ignore_tag_failure(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["run"] += " || true" + + +def continue_after_tag_failure(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["continue-on-error"] = True + + +def continue_after_pypi_failure(jobs: dict) -> None: + find_step(jobs, "gh-action-pypi-publish")["continue-on-error"] = True + + +def tag_even_if_pypi_fails(jobs: dict) -> None: + find_step(jobs, "publish-git-tag.sh")["if"] = "always()" + + +def skip_pypi_on_this_branch(jobs: dict) -> None: + # The default branch is master, so this skips the upload on every release. + find_step(jobs, "gh-action-pypi-publish")["if"] = "github.ref == 'refs/heads/main'" + + +@pytest.mark.parametrize( + ("mutate", "violation"), + [ + pytest.param(mutate, violation, id=mutate.__name__) + for mutate, violation in [ + ( + drop_publish_permissions_and_add_later_writer, + "Publish lacks contents: write", + ), + (shallow_checkout, "checkout is not fetch-depth 0"), + (tag_before_pypi, "tag step precedes the PyPI step"), + (skip_pypi_on_this_branch, "PyPI step has a condition"), + (tag_even_if_pypi_fails, "tag step has a condition"), + (continue_after_pypi_failure, "PyPI step continues on error"), + (continue_after_tag_failure, "tag step continues on error"), + (ignore_tag_failure, "tag step runs more than the tag script"), + ] + ], +) +def test_publish_workflow_check_rejects_unsafe_release_order(mutate, violation): + jobs = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"] + mutate(jobs) + + with pytest.raises(AssertionError, match=violation): + assert_publish_tags_only_after_pypi_succeeds(jobs["Publish"]) + + +def grant_write_all(jobs: dict) -> None: + jobs["Publish"]["permissions"] = "write-all" + + +def quote_fetch_depth(jobs: dict) -> None: + find_step(jobs, "actions/checkout@")["with"]["fetch-depth"] = "0" + + +@pytest.mark.parametrize( + "mutate", + [ + pytest.param(mutate, id=mutate.__name__) + for mutate in [grant_write_all, quote_fetch_depth] + ], +) +def test_publish_workflow_check_accepts_equivalent_settings(mutate): + jobs = yaml.safe_load(PUSH_WORKFLOW.read_text())["jobs"] + mutate(jobs) + + assert_publish_tags_only_after_pypi_succeeds(jobs["Publish"])