-
Notifications
You must be signed in to change notification settings - Fork 0
Add CI gate for easy branch protection setup #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ requires-python = ">=3.12" | |
| dev = [ | ||
| "pytest", | ||
| "copier", | ||
| "pyyaml", | ||
| ] | ||
|
|
||
| [tool.pytest.ini_options] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| Goal: allow easy activation of branch protection in github via rule set. | ||
|
|
||
| Normally, all targets that must be green must be hand-configured. This is flaky. | ||
|
|
||
| Instead, implement via CI gate so that rules don't need to list all possible targets. | ||
|
|
||
| See https://github.com/rodekruis/qualitative-feedback-analysis/blob/main/.github/workflows/ci.yaml for reference. | ||
|
|
||
| --- | ||
|
|
||
| ## Spec | ||
|
|
||
| **What:** Add one aggregate `ci-gate` job to **both** CI workflows — the shipped | ||
| `template/.github/workflows/cicd.yaml` (so every generated project has it) and this repo's own | ||
| `.github/workflows/ci.yaml` — restrict `push:` to `main` in both, add a drift test that the gate's | ||
| `needs` covers every other job, and note the required check name in the template README. | ||
|
|
||
| **Why:** A branch ruleset must name required status checks literally, so today it has to list every | ||
| matrix leg (`test (3.12)`, `test (3.13)`, …). That list silently rots the moment a matrix dimension | ||
| or job is added or renamed — the ruleset keeps passing while the new leg goes unchecked, which is | ||
| the flakiness this ticket is about. One unmatrixed aggregate job whose name never changes is the | ||
| only thing the ruleset has to require, and `needs:` moves the "everything must be green" list into | ||
| the workflow, next to the jobs it guards. | ||
|
|
||
| ### The gate job | ||
|
|
||
| Identical in both files except its `needs:` list. Append to `jobs:` in each: | ||
|
|
||
| ```yaml | ||
| # Single aggregate check to require in the branch ruleset. It depends on | ||
| # every other CI job and fails if any of them failed or was cancelled. | ||
| # Requiring this one check (instead of each job or matrix leg) keeps the | ||
| # ruleset stable when matrix dimensions change, and means a conditionally | ||
| # skipped dependency never leaves a required check stuck "waiting" — | ||
| # `skipped` is treated as acceptable here. | ||
| ci-gate: | ||
| if: always() | ||
| needs: [lint, test] # own ci.yaml has only `test`, so: needs: [test] | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Verify no required job failed | ||
| run: | | ||
| results="${{ join(needs.*.result, ' ') }}" | ||
| echo "Upstream job results: ${results}" | ||
| for r in ${results}; do | ||
| if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then | ||
| echo "::error::A required CI job did not pass (result: ${r})." | ||
| exit 1 | ||
| fi | ||
| done | ||
| echo "All required CI jobs passed (or were acceptably skipped)." | ||
| ``` | ||
|
|
||
| `if: always()` is load-bearing: without it the job is skipped when a dependency fails, and a | ||
| skipped required check leaves the PR waiting forever instead of failing. | ||
|
|
||
| The job has **no `uses:` step** — not even `actions/checkout` — so it adds nothing to the | ||
| action-pinning surface of #4. | ||
|
|
||
| ### Trigger change (both files) | ||
|
|
||
| Both workflows currently fire on `push:` (every branch) *and* `pull_request:`, which produces two | ||
| identical `ci-gate` check runs on any same-repo PR branch. Restrict push to `main`: | ||
|
|
||
| ```yaml | ||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| workflow_call: # template/cicd.yaml only; the repo's own ci.yaml has none | ||
| ``` | ||
|
|
||
| Keep `pull_request:` — dropping it (as the qfa reference does) means fork PRs never run CI, so the | ||
| required check never appears and those PRs become unmergeable. Accepted cost: pushing a branch with | ||
| no PR open no longer runs CI. | ||
|
|
||
| ### Drift test | ||
|
|
||
| The gate is only as good as its `needs:` list — add a job, forget the list, and the gate goes green | ||
| while the new job burns. Add to `tests/test_template.py`, parameterized over both workflow files on | ||
| disk (no rendering needed, see below), asserting: a `ci-gate` job exists, it carries `if: always()`, | ||
| and `set(needs) == set(jobs) - {"ci-gate"}`. | ||
|
|
||
| Two traps for the implementer: | ||
|
|
||
| - **PyYAML parses a bare `on:` key as boolean `True`** (YAML 1.1). Any assertion that inspects | ||
| triggers must read `workflow[True] if True in workflow else workflow["on"]` — the same dance | ||
| `tests/test_docker_packaging.py` uses in the aeloop repo. | ||
| - **`pyyaml` is currently only a transitive dependency** (via `copier`). Add it explicitly to the | ||
| `dev` group in the root `pyproject.toml` rather than relying on the transitive. | ||
|
|
||
| The template workflow can be parsed straight from disk because `_templates_suffix: .jinja` means | ||
| only `.jinja` files are rendered — `cicd.yaml` is copied verbatim, which is why its existing | ||
| `${{ matrix.python-version }}` survives generation today. Do **not** rename it to `.jinja`; `${{ }}` | ||
| would then collide with Jinja. | ||
|
|
||
| ### README note | ||
|
|
||
| Under *Development* in `template/README.md.jinja`, at most four lines naming `ci-gate` as the single | ||
| check to require in a branch ruleset. Per `AGENTS.md`, add the fact and nothing more — no | ||
| walkthrough of the GitHub settings UI. | ||
|
|
||
| ## Acceptance criteria | ||
|
|
||
| - [ ] `template/.github/workflows/cicd.yaml` has a `ci-gate` job exactly as specified above, with | ||
| `needs: [lint, test]`, `if: always()`, `runs-on: ubuntu-latest`, and no `uses:` step. | ||
| - [ ] `.github/workflows/ci.yaml` has the same job with `needs: [test]`. | ||
| - [ ] In both files `push:` is restricted to `branches: [main]`, `pull_request:` is retained, and the | ||
| template's `workflow_call:` is retained. | ||
| - [ ] `tests/test_template.py` gains a test, parameterized over both workflow files, asserting the | ||
| gate exists, carries `if: always()`, and that its `needs` equals every other job in that file. | ||
| - [ ] That test fails if a job is added to either workflow without being added to `needs` (verify by | ||
| temporarily adding a dummy job locally, or by reasoning stated in the PR body). | ||
| - [ ] `pyyaml` is an explicit entry in the `dev` dependency group of the root `pyproject.toml`. | ||
| - [ ] `template/README.md.jinja` names `ci-gate` as the required check in ≤4 added lines under | ||
| *Development*. | ||
| - [ ] `template/.github/workflows/cicd.yaml` still has no `.jinja` suffix and renders verbatim — the | ||
| existing rendered-project tests still pass. | ||
| - [ ] `uv run pytest tests` is green. | ||
| - [ ] Commit uses the `ci:` prefix (as in 9b55106) and carries no AI attribution trailer. | ||
|
|
||
| ## Activating the protection (repo admin, after merge) | ||
|
|
||
| The one check to require is `ci-gate`. Applies identically to this repo and to any generated project. | ||
|
|
||
| UI: *Settings → Rules → Rulesets → New branch ruleset* → target **Default branch** → tick **Require | ||
| status checks to pass** → add `ci-gate` → enforcement **Active**. | ||
|
|
||
| Or: | ||
|
|
||
| ```bash | ||
| gh api -X POST repos/<owner>/<repo>/rulesets --input - <<'JSON' | ||
| { | ||
| "name": "main", | ||
| "target": "branch", | ||
| "enforcement": "active", | ||
| "conditions": { "ref_name": { "include": ["~DEFAULT_BRANCH"], "exclude": [] } }, | ||
| "rules": [ | ||
| { "type": "required_status_checks", | ||
| "parameters": { | ||
| "strict_required_status_checks_policy": false, | ||
| "required_status_checks": [ { "context": "ci-gate" } ] | ||
| } } | ||
| ] | ||
| } | ||
| JSON | ||
| ``` | ||
|
|
||
| - Leave **Require branches to be up to date** (`strict_required_status_checks_policy`) off unless you | ||
| want every merge to force a rebase and a full CI re-run on the next PR in the queue. | ||
| - Add the parameterless `deletion` and `non_fast_forward` rule types to also block branch deletion | ||
| and force-pushes. | ||
| - Adding a CI job later needs no ruleset edit — that is the point of the gate. | ||
|
|
||
| ## Out of scope | ||
|
|
||
| - Creating or enabling the branch ruleset itself — that is a repo-admin action in GitHub settings, | ||
| not code. The section above is the recipe; this ticket only makes one stable check name available | ||
| to require. | ||
| - Automating ruleset creation via `gh api` (GitHub does not import rulesets from a repo file). | ||
| - Renaming `cicd.yaml` → `ci.yaml` in the template (#8) and pinning actions to hashes (#4). Both are | ||
| independent of this change; whichever lands first, the other rebases. | ||
|
|
||
| ## Verification | ||
|
|
||
| ```bash | ||
| uv run pytest tests # needs network; renders a project and runs its lint/test/pre-commit | ||
| ``` | ||
|
|
||
| The gate's runtime behaviour can only be fully proven on GitHub: after merge, confirm a `ci-gate` | ||
| check run appears on a PR and that a ruleset can require it by that name. | ||
|
|
||
| No `Depends-on:` — nothing this spec builds on is unmerged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| from types import SimpleNamespace | ||
|
|
||
| import pytest | ||
| import yaml | ||
|
|
||
| import copier | ||
|
|
||
|
|
@@ -15,6 +16,28 @@ | |
|
|
||
| TEMPLATE_ROOT = Path(__file__).resolve().parent.parent | ||
|
|
||
| # Both CI workflows carry the aggregate `ci-gate` job. They can be parsed | ||
| # straight from disk: `_templates_suffix: .jinja` means only `.jinja` files are | ||
| # rendered, so the template's cicd.yaml is copied verbatim (which is also why | ||
| # its `${{ matrix.python-version }}` survives generation). | ||
| CI_WORKFLOWS = [ | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. A glob is not the fix — ~Written by Claude, run via the agentic engineering loop |
||
| TEMPLATE_ROOT / ".github" / "workflows" / "ci.yaml", | ||
| TEMPLATE_ROOT / "template" / ".github" / "workflows" / "cicd.yaml", | ||
| ] | ||
|
|
||
|
|
||
| def _load_workflow(path: Path) -> dict: | ||
| return yaml.safe_load(path.read_text(encoding="utf-8")) | ||
|
|
||
|
|
||
| def _triggers(workflow: dict) -> dict: | ||
| """Return a workflow's `on:` block. | ||
|
|
||
| PyYAML follows YAML 1.1, where a bare ``on`` key is the boolean ``True``, | ||
| so the block cannot simply be looked up by its name. | ||
| """ | ||
| return workflow[True] if True in workflow else workflow["on"] | ||
|
|
||
| COPIER_DATA = { | ||
| "author_name": "Test Author", | ||
| "author_email": "test@example.com", | ||
|
|
@@ -230,6 +253,42 @@ def test_default_slug_is_valid_package_name(tmp_path): | |
| assert slug.isidentifier(), f"slug is not a valid identifier: {slug!r}" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("workflow_path", CI_WORKFLOWS, ids=lambda p: p.name) | ||
| def test_ci_gate_covers_every_job(workflow_path): | ||
| """The `ci-gate` job must depend on every other job in its workflow. | ||
|
|
||
| A branch ruleset names required checks literally, so it requires only | ||
| `ci-gate` and the workflow itself carries the "everything must be green" | ||
| list. That indirection is only safe while `needs:` stays complete — add a | ||
| job (or a whole matrix) and forget the list, and the gate goes green while | ||
| the new job burns. This test is the tripwire for exactly that drift. | ||
| """ | ||
| workflow = _load_workflow(workflow_path) | ||
| jobs = workflow["jobs"] | ||
|
|
||
| assert "ci-gate" in jobs, f"{workflow_path.name} has no ci-gate job" | ||
| gate = jobs["ci-gate"] | ||
|
|
||
| # Without `if: always()` the gate is skipped when a dependency fails, and a | ||
| # skipped required check leaves the PR waiting forever instead of failing. | ||
| assert gate.get("if") == "always()" | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. Exact-string match.
~Written by Claude, run via the agentic engineering loop |
||
| assert set(gate["needs"]) == set(jobs) - {"ci-gate"} | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("workflow_path", CI_WORKFLOWS, ids=lambda p: p.name) | ||
| def test_ci_workflow_triggers(workflow_path): | ||
| """Push runs only on main; pull_request stays so fork PRs still get a gate. | ||
|
|
||
| Firing on every push *and* on pull_request produces two identical `ci-gate` | ||
| check runs for a same-repo PR branch. Dropping `pull_request:` instead | ||
| would mean fork PRs never run CI, so the required check never appears and | ||
| those PRs cannot be merged. | ||
| """ | ||
| triggers = _triggers(_load_workflow(workflow_path)) | ||
| assert triggers["push"] == {"branches": ["main"]} | ||
| assert "pull_request" in triggers | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor (test coverage gap for a spec'd invariant). This test guards Dropping if workflow_path.name == "cicd.yaml":
assert "workflow_call" in triggersNot gating — the file is correct today. ~Written by Claude, run via the agentic engineering loop |
||
|
|
||
|
|
||
| def test_invalid_project_slug_is_rejected(tmp_path): | ||
| """An explicitly supplied invalid project_slug must fail validation. | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit. This is a single ~185-character line, while the surrounding prose in this file wraps around 90 (see the Documentation section just below). Content and length are exactly right per the spec — only the wrapping is inconsistent.
~Written by Claude, run via the agentic engineering loop