diff --git a/.github/rulesets/main-build-and-test.json b/.github/rulesets/main-every-check-green.json similarity index 73% rename from .github/rulesets/main-build-and-test.json rename to .github/rulesets/main-every-check-green.json index 80aff1a..7f37823 100644 --- a/.github/rulesets/main-build-and-test.json +++ b/.github/rulesets/main-every-check-green.json @@ -1,5 +1,5 @@ { - "name": "main: build and test must pass", + "name": "main: every check must pass", "target": "branch", "enforcement": "active", "conditions": { @@ -23,9 +23,12 @@ { "type": "required_status_checks", "parameters": { - "strict_required_status_checks_policy": false, + "strict_required_status_checks_policy": true, "required_status_checks": [ - { "context": "Build and test" } + { "context": "Build and test" }, + { "context": "Coverage" }, + { "context": "Playwright" }, + { "context": "Visual check" } ] } }, diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 10bf9b9..18aa43a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,22 +8,20 @@ name: CI # against, so the app the browsers see is the app the unit tests passed on, not a rebuild of it. on: - # The merge ref, which is the only thing worth building. `actions/checkout` resolves a - # `pull_request` event to `refs/pull/N/merge` — this branch already merged into main — where a - # push event checks out the branch tip on its own. `strict_required_status_checks_policy` is - # deliberately off (see docs/deployment.md), so a branch can merge without being rebuilt against - # a moved main, and this event is what covers that. It also covers a fork's pull request. + # The event that reports these four checks in the ordinary course of things, and the merge ref is + # the thing worth building: `actions/checkout` resolves a `pull_request` event to + # `refs/pull/N/merge`, this branch already merged into main, where a push event would check out + # the branch tip on its own. Nothing runs on main afterwards — merging deploys — so what goes + # green here is the last word on the commit that reaches the volume. It covers a fork's pull + # request too. pull_request: - # Called by fly-deploy.yml, so the gate in front of a deploy is literally the same build and test - # a pull request ran, rather than a copy of it that can drift. The browser jobs below opt out of - # this event: they are advisory, and a flaky browser should not be able to hold up a release. - workflow_call: - # The escape hatch. Runs everything, browsers included — useful for checking a branch that has no - # pull request yet, and for the case where a pull request somehow shows no checks at all. + # The escape hatch, for a branch with no pull request yet and for the case where a pull request + # somehow shows no checks at all. It reports the same four contexts, so it can unlock a merge on + # its own — off the branch tip rather than the merge ref. See docs/deployment.md. workflow_dispatch: -# Keyed by ref, so a pull request's checks start straight away instead of queueing behind a -# deploy. A run on main is left to finish — its build is what a deploy is waiting on. +# Keyed by ref, so pushing again to a pull request cancels that pull request's run and nothing +# else. A dispatched run is left to finish: it was asked for by hand. concurrency: group: ci-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} @@ -32,8 +30,10 @@ permissions: contents: read jobs: - # The required check. `main`'s ruleset names "Build and test" exactly, so renaming this job means - # editing .github/rulesets/main-build-and-test.json to match — see docs/deployment.md. + # All four jobs here are required checks, and `main`'s ruleset names each of them by the `name:` + # it declares — so renaming one means editing .github/rulesets/main-every-check-green.json in the + # same change, or the merge button waits forever on a check nothing reports. See + # docs/deployment.md. build: name: Build and test runs-on: ubuntu-latest @@ -112,13 +112,14 @@ jobs: # docs/testing.md has the reasoning. The result is written to this run's summary page, so the # number is read without opening a log or downloading the report. # - # Advisory, like the two browser jobs: it goes red under the floor but `main`'s ruleset requires - # "Build and test" alone, so it reports rather than blocks. It needs no compiler — the report was - # produced by the test run above — so it is node and a git history, and it is over in seconds. + # Blocking, like the two browser jobs below: under the floor, the merge button stays disabled. A + # change that touches no coverable line — docs, CSS, a workflow — measures nothing and passes, so + # requiring this cannot wedge a pull request that had no code in it. It needs no compiler, the + # report having come out of the test run above, so it is node and a git history and it is over in + # seconds. coverage: name: Coverage needs: build - if: github.event_name != 'workflow_call' runs-on: ubuntu-latest timeout-minutes: 10 steps: @@ -148,15 +149,12 @@ jobs: # What the app does: the public/admin split, the dialogs, the match-day journey, both languages, # the phone layout. See docs/testing.md. # - # Advisory. `main`'s ruleset requires "Build and test" and nothing else, so this reports without - # blocking a merge — promoting it once it has a track record on real runners is one line in - # .github/rulesets/main-build-and-test.json and nothing here. It also skips the workflow_call - # event, which is fly-deploy.yml asking for its gate: a browser should not stand between a merged - # pull request and a release. + # Blocking. Merging is now the release, and nothing re-tests on main afterwards, so a browser + # failure is dealt with here or not at all. The cost is that a flake holds the merge: re-run the + # job from the Actions page, because the ruleset grants no bypass to anyone. playwright: name: Playwright needs: build - if: github.event_name != 'workflow_call' runs-on: ubuntu-latest timeout-minutes: 20 steps: @@ -229,12 +227,11 @@ jobs: # Whether it renders at all, and whether a thumb can hit it. This is the only thing holding the # Touch / PWA fixes in docs/known_issues.md in place — every one of them is CSS, and a MudBlazor - # upgrade or one more global .mud-* rule undoes any of them silently. Advisory, and skipped for - # workflow_call, for the same reasons as the job above. + # upgrade or one more global .mud-* rule undoes any of them silently. Blocking, for the same + # reason as the job above. visual: name: Visual check needs: build - if: github.event_name != 'workflow_call' runs-on: ubuntu-latest timeout-minutes: 20 steps: diff --git a/.github/workflows/fly-deploy.yml b/.github/workflows/fly-deploy.yml index 8f23a29..f3a1cb4 100644 --- a/.github/workflows/fly-deploy.yml +++ b/.github/workflows/fly-deploy.yml @@ -5,7 +5,15 @@ name: Deploy to Fly.io on: push: branches: [main] + # The confirmation is what the gate job and the environment reviewer used to be for this route: + # both went when merging became the release, and a dispatch has neither a pull request behind it + # nor a pause in front of it. A stray "Run workflow" click would otherwise replace the container + # that is serving, which on a match day drops the live screen's circuits. workflow_dispatch: + inputs: + confirm: + description: Type "deploy" to replace what is serving right now + required: true # One deploy at a time: SQLite lives on a single volume and the app migrates on boot, so two # overlapping deploys race over one machine. @@ -17,27 +25,30 @@ permissions: contents: read jobs: - # Gate: nothing reaches the Fly volume that doesn't compile and pass its tests. The app - # auto-migrates on startup, so a broken build is a broken production database. - build: - uses: ./.github/workflows/ci.yml - + # The merge is the decision. Every check on the pull request had to be green before it could land, + # and the branch had to be up to date with main to get there, so this starts the moment the merge + # commit exists — no gate job, no approval, nobody to wait for. The compile still happens: flyctl + # builds the image from the Dockerfile, so a merge that does not build never reaches the volume, + # and the smoke check below is what proves the one that did is serving. deploy: - needs: build - # `workflow_dispatch` runs against any branch, and without the ref check would put that branch - # on the production volume. The actor check is about *who*: a dispatch can be started by anyone - # with write access, and while the environment's reviewer below would stop it anyway, refusing - # it here keeps a run that was never going to deploy out of the approval queue. + # Three conditions, and only the first applies to a merge. `workflow_dispatch` runs against any + # branch, and without the ref check would put that branch on the production volume; the actor + # check is about *who*, since a dispatch can be started by anyone with write access; the input + # is about *meant to*. A merge is none of these — it arrives already reviewed by four green + # checks — so the `push` arm passes on the ref alone and deploys with nothing to fill in. + # + # On a push `inputs.confirm` is null, which is why the dispatch conditions sit behind the + # event_name test rather than beside it. if: >- github.ref == 'refs/heads/main' - && (github.event_name != 'workflow_dispatch' || github.actor == github.repository_owner) + && (github.event_name != 'workflow_dispatch' + || (github.actor == github.repository_owner && inputs.confirm == 'deploy')) runs-on: ubuntu-latest - # The gate that decides *whether* a release goes out, and the only place the Fly token exists. - # `production` carries a required reviewer, so this job holds at "Waiting" until a human - # approves it — merging to main proposes a deploy rather than performing one. It also carries a - # deployment branch policy of `main` only, which is what stops a workflow added on a feature - # branch from reading FLY_API_TOKEN at all: an environment secret is unreachable from a ref the - # environment does not admit. See docs/deployment.md — none of it lives in this file, and the + # Not a gate any more — the only place the Fly token exists. `production` carries a deployment + # branch policy of `main` only, which is what stops a workflow added on a feature branch from + # reading FLY_API_TOKEN at all: an environment secret is unreachable from a ref the environment + # does not admit. Its *Required reviewers* box is deliberately empty, which is what makes a + # merge deploy on its own. See docs/deployment.md — none of it lives in this file, and the # environment has to exist before this workflow runs. environment: name: production diff --git a/CLAUDE.md b/CLAUDE.md index 4a598d5..2de7002 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,7 +42,7 @@ dotnet ef database update --project src/FootballFormation.Core src/FootballFormation.Core/ Models, Data (EF Core), Reporting, Services, Result — no UI references src/FootballFormation.UI/ Razor Class Library: pages, components, navigation, state, theming src/FootballFormation.Web/ Host: Program.cs, App.razor, Routes.razor, wwwroot (CSS, JS, PWA) -tests/FootballFormation.Core.Tests/ xUnit v3 — the CI gate the deploy depends on +tests/FootballFormation.Core.Tests/ xUnit v3 — one of the four checks a merge waits for docs/ The detailed documentation (see the index below) scripts/ visual-check.sh + its Playwright driver ``` @@ -167,14 +167,14 @@ There are **no component tests** (no bUnit). The UI is checked by driving the re browser, in two places, both against a throwaway database: - **`cd tests/ui && npm test`** — Playwright, ~39 tests, about a minute. Runs on every pull request - as a job in `ci.yml`, advisory rather than the merge gate. Behaviour: the public/admin + as a job in `ci.yml`, and a red run holds the merge. Behaviour: the public/admin split, the squad and match dialogs, the whole match-day journey from dragging a lineup to blowing the final whistle, both languages, and the phone layout. Read [docs/testing.md](docs/testing.md#ui-tests-testsui) **before adding one** — a Blazor Server page renders twice and the prerender is fully clickable and completely inert, so `goto()` waits for Blazor's `_bl_*` attributes and `clickFor()` clicks for an outcome. There are no fixed sleeps in that directory; do not introduce one. -- **`scripts/visual-check.sh`** — the `Visual check` job in `ci.yml`, also advisory. Screenshots every +- **`scripts/visual-check.sh`** — the `Visual check` job in `ci.yml`, also blocking. Screenshots every page, then measures every touch target on three phone viewports against the 44px floor. The only check that a page renders at all, and the only thing holding the Touch / PWA fixes in `known_issues.md` in place. @@ -184,16 +184,17 @@ admin matrix; it is not part of this repository.) ## Workflow - Work on a feature branch. `main` takes pull requests only, and the merge button stays disabled - until **Build and test** is green and every review thread is resolved - (`.github/rulesets/main-build-and-test.json`). + until **all four** checks are green — **Build and test**, **Coverage**, **Playwright**, **Visual + check** — the branch is up to date with `main`, and every review thread is resolved + (`.github/rulesets/main-every-check-green.json`, which grants no bypass to anyone). - `ci.yml` runs `dotnet build -c Release` + `dotnet test` on every pull request. That test run - carries the coverage collector, and an advisory **Coverage** job judges the lines the pull - request changed against the 80% floor and writes the numbers to the run's summary page. - `fly-deploy.yml` *calls that same workflow* as the gate its deploy job depends on, then - smoke-checks `/health` until it reports the commit that was just built. -- Merging to `main` *proposes* a deploy; it does not perform one. The deploy job runs in the - `production` environment, which has a required reviewer, so the run waits at *Waiting* until the - maintainer approves it. There is no staging environment — that approval is the last look. + carries the coverage collector, and the **Coverage** job judges the lines the pull request changed + against the 80% floor and writes the numbers to the run's summary page. A change with no coverable + line in it passes rather than wedging. +- **Merging to `main` releases.** `fly-deploy.yml` starts on the merge commit with no gate job and + no approval, then smoke-checks `/health` until it reports the commit that was just built. There is + no staging environment and nothing re-runs on `main`, so the four checks on the pull request are + the last look — which is why a flaky browser job is re-run rather than merged past. - Commit messages are plain imperative sentences describing the intent, not conventional-commit prefixes: *"Let a deploy recognise its own release, not just a live one"*, *"Split the games list on the scoreline, not the calendar"*. diff --git a/README.md b/README.md index 7c965e5..33d3a31 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,7 @@ FootballFormation/ │ ├── FootballFormation.UI/ # Blazor components, pages, helpers, layout │ └── FootballFormation.Web/ # Host project, Program.cs, wwwroot ├── tests/ -│ └── FootballFormation.Core.Tests/ # Service and domain tests (gate the deploy) +│ └── FootballFormation.Core.Tests/ # Service and domain tests (gate the merge) ├── docs/ # Architecture & project documentation │ ├── project_overview.md │ ├── architecture.md @@ -117,18 +117,18 @@ outside Production *and* only for loopback callers. dotnet test ``` -`.github/workflows/ci.yml` runs `dotnet build -c Release` and `dotnet test`. It runs on every pull -request, and `fly-deploy.yml` calls that same workflow as the gate its deploy job depends on — so -the checks in front of production are literally the ones a pull request ran, not a copy that can -drift. Note that `Directory.Build.props` sets `TreatWarningsAsErrors` in **Release only** — a -warning that builds fine locally will fail CI. Build Release before pushing. The exception is -`MSB3568` (a duplicate resource name, which quietly changes what the app says), an error in every -configuration. +`.github/workflows/ci.yml` runs `dotnet build -c Release` and `dotnet test` on every pull request, +alongside the coverage and browser jobs below. Note that `Directory.Build.props` sets +`TreatWarningsAsErrors` in **Release only** — a warning that builds fine locally will fail CI. Build +Release before pushing. The exception is `MSB3568` (a duplicate resource name, which quietly changes +what the app says), an error in every configuration. -A ruleset makes that check binding: `main` takes pull requests only, and the merge button stays -disabled until `Build and test` is green. It lives in `.github/rulesets/main-build-and-test.json` -and has to be imported into the repository's settings once — see -[docs/deployment.md](docs/deployment.md#only-a-green-build-can-be-merged). +A ruleset makes those checks binding: `main` takes pull requests only, the branch has to be up to +date with `main`, and the merge button stays disabled until all four — `Build and test`, `Coverage`, +`Playwright` and `Visual check` — are green. Merging is what releases: nothing runs on `main` +afterwards, so those four are the last word on what reaches production. The ruleset lives in +`.github/rulesets/main-every-check-green.json` and has to be imported into the repository's settings +once — see [docs/deployment.md](docs/deployment.md#only-a-green-build-can-be-merged). ### UI tests @@ -140,7 +140,7 @@ Playwright, driving the real app in a browser against a database that exists onl public/admin split, the squad and match dialogs, the full match-day journey from dragging a lineup to blowing the final whistle, both languages, and the phone layout. About a minute, 34 tests. Runs on every pull request as the `Playwright` job in `.github/workflows/ci.yml`, against the app that -workflow published — an advisory check rather than the merge gate. See [docs/testing.md](docs/testing.md#ui-tests-testsui). +workflow published — one of the four checks the merge waits for. See [docs/testing.md](docs/testing.md#ui-tests-testsui). ### Visual checks @@ -157,7 +157,7 @@ It then measures rather than looks: the match dialog and its date picker are reo and landscape phone sizes and every touch target is checked for the 44px minimum and for dead space between it and its neighbours. That is the only thing holding the touch fixes in [docs/known_issues.md](docs/known_issues.md) in place, so it runs on every pull request too — the -`Visual check` job in `ci.yml`, advisory like the one beside it, and it uploads its screenshots +`Visual check` job in `ci.yml`, blocking like the one beside it, and it uploads its screenshots either way. See [docs/testing.md](docs/testing.md). diff --git a/docs/deployment.md b/docs/deployment.md index 55aca95..496c882 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -58,55 +58,83 @@ certificate are done (usually minutes after DNS propagates). ## Redeploying after changes -Merging to `main` *proposes* a deploy via GitHub Actions (`.github/workflows/fly-deploy.yml`, -authenticated by the `FLY_API_TOKEN` secret — a scoped deploy token from -`flyctl tokens create deploy --app gjs-meiden`). It does not perform one: the deploy job names the -**`production` environment**, which holds a required reviewer, so the run stops at *Waiting* until -somebody approves it. See [Only one person can deploy](#only-one-person-can-deploy) below — that -environment, not this workflow, is where the token lives and where the decision is made. +Merging to `main` **is** the release. GitHub Actions picks up the merge commit +(`.github/workflows/fly-deploy.yml`, authenticated by the `FLY_API_TOKEN` secret — a scoped deploy +token from `flyctl tokens create deploy --app gjs-meiden`) and the deploy job starts straight away: +no gate job in front of it, no approval to click, nothing at *Waiting*. + +That is affordable because the weight sits at the other end. Every check on the pull request has to +be green and the branch has to be up to date with `main` before the merge button unlocks at all, so +the decision to merge and the decision to release are the same decision, taken once. See +[Only a green build can be merged](#only-a-green-build-can-be-merged) below. Pull requests are gated by a **separate** workflow, `.github/workflows/ci.yml` ("CI"), which runs -`Build and test` — restore, a Release build (where warnings are errors) and the test suite. It -holds no deploy job and no Fly token, so a pull request cannot reach the volume even in principle; -the deploy workflow no longer triggers on `pull_request` at all. `fly-deploy.yml` calls the same -CI workflow as its gate (`uses: ./.github/workflows/ci.yml`), so what runs before a deploy is -literally what ran on the PR rather than a copy that can drift. Deploying additionally requires -`main`, which stops a `workflow_dispatch` run against a feature branch from putting that branch -into production. +four jobs: `Build and test` (restore, a Release build where warnings are errors, and the test +suite), `Coverage`, `Playwright` and `Visual check`. It holds no deploy job and no Fly token, so a +pull request cannot reach the volume even in principle; the deploy workflow does not trigger on +`pull_request` at all. Nothing re-runs on `main` afterwards — those four checks are the last word on +the commit that reaches the volume. What compiles it again is the Docker build inside +`flyctl deploy`, so a merge that does not build fails the deploy rather than shipping; what proves +the result is live is the smoke check. Deploying also requires `main`, which stops a +`workflow_dispatch` run against a feature branch from putting that branch into production. ## Only a green build can be merged A workflow can report a failure but it cannot refuse a merge — that is a repository setting, and -until it is switched on a red PR merges as easily as a green one. `.github/rulesets/main-build-and-test.json` +until it is switched on a red PR merges as easily as a green one. `.github/rulesets/main-every-check-green.json` is that setting, written down: a GitHub **ruleset** covering the default branch which - requires a pull request, so nothing lands on `main` by direct push; -- requires the **`Build and test`** check to pass, so the merge button stays disabled while it is - queued, running, or failing; +- requires **all four** checks — `Build and test`, `Coverage`, `Playwright` and `Visual check` — so + the merge button stays disabled while any of them is queued, running, or failing; +- requires the branch to be **up to date with `main`** before it can land; - blocks deletion and force-pushes on `main`, since the deploy history is what a rollback reads; - grants **no bypass to anyone**, including the repo owner. +Four rather than one because merging now deploys. Nothing runs between the merge button and the Fly +volume, so a check that only reports is a check nobody is obliged to read, and the browser jobs are +the only thing in this repository that knows whether a page renders at all. + **GitHub does not read this file from the repository.** Nothing in a repo can grant itself branch protection — that would rather defeat the point. The file is an importable export, and it has to be applied once by hand: -> *Settings → Rules → Rulesets → New ruleset → **Import a ruleset*** → upload -> `.github/rulesets/main-build-and-test.json` → **Create**. - -Check it took by opening any pull request: the merge button should be greyed with *Required -statuses must pass before merging*. After that the file is the record of what is configured — change -the rule here and re-import, so the setting is reviewable in a diff like everything else. - -**`Build and test` has to run on every pull request, or the guard inverts.** A required check that -never reports leaves a PR pending forever rather than mergeable, so `ci.yml` deliberately carries no -`paths:` filter — adding one later to "skip CI for docs" would silently wedge every docs-only PR. -If the job is ever renamed, the ruleset's `context` must be renamed with it. - -`ci.yml` triggers on `pull_request` alone, so that event is now the *only* thing that reports this -check — it also carried a `push` trigger until every pull request was found to be building twice -(see `testing.md`, "What triggers these"). A pull request showing no checks at all, rather than a -red one, is what a regression here looks like: re-run it from *Actions → CI → Run workflow*, or push -one more commit. +> *Settings → Rules → Rulesets* → delete the ruleset already there → **New ruleset → Import a +> ruleset** → upload `.github/rulesets/main-every-check-green.json` → **Create**. + +Import always *creates*, so deleting the old one first is what stops two rulesets applying at once. +Check it took by opening any pull request: the merge button should be greyed with *Required statuses +must pass before merging*, and name all four. After that the file is the record of what is +configured — change the rule here and re-import, so the setting is reviewable in a diff like +everything else. + +**All four have to run on every pull request, or the guard inverts.** A required check that never +reports leaves a PR pending forever rather than mergeable, so `ci.yml` deliberately carries no +`paths:` filter — adding one later to "skip CI for docs" would silently wedge every docs-only PR, +and there are now four checks to wedge it rather than one. If a job is ever renamed, the ruleset's +`context` must be renamed with it in the same change. + +`Coverage` is safe to require for the same reason the filter is not: a change with no coverable line +in it — docs, CSS, a workflow — measures nothing and passes, rather than dividing by zero and going +red. It is the 80% floor on *changed* lines, so a pull request that touches no code has nothing to +fall short of. + +`ci.yml` triggers on `pull_request` and nothing else automatic — it also carried a `push` trigger +until every pull request was found to be building twice (see `testing.md`, "What triggers it"). A +pull request showing no checks at all, rather than a red one, is what a regression here looks like: +re-run it from *Actions → CI → Run workflow*, or push one more commit. + +**That escape hatch is `workflow_dispatch`, and it reports the same four contexts.** Check runs +attach to a commit, so a dispatched run against a branch posts its results onto the pull request +whose head that commit is, and can unlock the merge button on its own. What it built is not quite +the same thing: a dispatch checks out the **branch tip**, where the `pull_request` event resolves to +`refs/pull/N/merge` — this branch already merged into `main`. Being up to date with `main` is +required, so the two are usually the same tree; on a wedged pull request, prefer one more commit and +keep the dispatch for when there is nothing to push. + +**A flake now blocks a merge.** That is the cost of requiring the browser jobs, and it is the +intended one: re-run the failed job from the run's page. There is no bypass, so a red check that +will not go green has to be fixed or the ruleset has to be disabled, which is visible. **A restricted actions policy stops the check before it can report — and it looks like nothing ran.** Under *Settings → Actions → General → Actions permissions*, the owner-only option ("Allow @@ -130,25 +158,39 @@ If the policy must stay restricted, the allow-list needs **both** of: | `superfly/*` | `superfly/flyctl-actions/setup-flyctl` — `fly-deploy.yml` | **The second is the one that bites quietly.** `ci.yml` uses no third-party action, so ticking only -the GitHub box turns every check green while `fly-deploy.yml` still fails at startup — a refused run -creates no job, so there is no approval waiting for you either, and the symptom is not a red build -but a site that silently stops updating. +the GitHub box turns every check green while `fly-deploy.yml` still fails at startup — and since a +refused run creates no job, the merge that was meant to release produces nothing to look at. The +symptom is not a red build but a site that silently stops updating. Whenever this policy is touched, check the deploy workflow too, not just the pull request. -**Deliberately not enabled: *require branches to be up to date*** (`strict_required_status_checks_policy`). -It would force every PR to re-run against a moved `main` before landing, which for a single-maintainer -repo is mostly friction. The case it protects against — a PR that passed against a stale `main` and -breaks once merged — is already caught before it can reach production, because `fly-deploy.yml` runs -the same CI workflow again on `main` and the deploy job depends on it. It is also partly covered on -the pull request itself: `actions/checkout` resolves a `pull_request` event to `refs/pull/N/merge`, -so the second of the two `Build and test` runs a pull request produces is building this branch -*already merged into* `main`. Turn the policy on if that changes. +**Deliberately enabled: *require branches to be up to date*** (`strict_required_status_checks_policy`). +It forces a pull request whose base has moved to take `main` on board and go green again before it +can land — the *Update branch* button, then four checks over from the top. + +It was off for a long time, on the grounds that the case it protects against — a PR that passed +against a stale `main` and breaks once merged — was caught before it could reach production, because +`fly-deploy.yml` re-ran CI on `main` and the deploy depended on it. That re-run is gone: merging +deploys, and nothing tests the merged result any more. This policy is what replaced it, and the two +are a pair — turning it off means putting the gate job back. + +`actions/checkout` resolving a `pull_request` event to `refs/pull/N/merge` covers part of the same +ground already, since the checks are built on this branch *already merged into* `main`. What it +cannot cover is `main` moving after those checks went green, which is exactly the window this +closes. + +The cost is real and worth naming: every *Update branch* click re-runs all four checks, browsers +included, so a merge behind a moved `main` is about twenty minutes rather than four. **The escape hatch is the ruleset, not a bypass.** With `bypass_actors` empty there is no way to merge past a red build quietly; an emergency means setting the ruleset to *Disabled*, which is a visible, logged act that shows up in the repo's rule insights. That is the intended trade — the guard is worth little if the person most likely to be in a hurry can step around it silently. +It costs more than it used to. While the gate job existed, disabling the ruleset only skipped the +*merge* check and the commit was still built and tested on `main` before Fly saw it. There is no +such second look now: disabling the ruleset means an unbuilt, untested commit merges, deploys, and +migrates the live volume, in that order and within a couple of minutes. Turn it back on afterwards. + **`required_approving_review_count` is 0, and one account is the reason.** GitHub refuses to let anyone approve their own pull request — the *Approve* radio is not rendered on your own PR, and the API answers `422 Can not approve your own pull request`. There is no setting that relaxes it; every @@ -165,32 +207,38 @@ What is enabled instead is **`required_review_thread_resolution`**, which does b account: it needs no approval, only that every review comment thread is resolved before the merge button unlocks. Leave a note on a diff and the merge waits for you to deal with it. -The requirement that actually stops unreviewed code reaching people is one layer further on — it -guards the deploy rather than the merge, and it is described next. **Adding a second account as a -collaborator is the only way to make the merge itself require another pair of eyes**; if that -happens, set the count to 1 and leave `bypass_actors` empty. +**Adding a second account as a collaborator is the only way to make the merge itself require another +pair of eyes**; if that happens, set the count to 1 and leave `bypass_actors` empty. Until then the +four checks are what a merge has to satisfy, and the person clicking merge is the review. ## Only one person can deploy -Three things could put a release on the volume, and each is closed separately. +Three things could put a release on the volume, and each is closed separately. A required reviewer on +the `production` environment used to stand across the first two of them; removing it is what made +merging the release. That decision did not disappear, it moved to the merge button, which needs four +green checks and a branch up to date with `main`, and which only somebody with write access can +press. **A push to `main`.** Impossible directly — the ruleset above requires a pull request and grants no -bypass — and merging no longer deploys on its own. `fly-deploy.yml`'s deploy job names the -`production` environment, which carries a **required reviewer**; the job holds at *Waiting* and -Fly is never contacted until that person clicks *Approve*. Unlike a pull request approval, GitHub -does permit approving your own deployment: the *Prevent self-review* checkbox on the environment -is opt-in and stays off here, which is exactly what makes this gate work for a single maintainer -where the review gate cannot. +bypass. What lands on `main` lands by merge, and a merge now deploys within a minute or two of the +click, with nothing to approve on the way. The `production` environment still exists and the job +still names it, but only for its secret and its branch policy; its *Required reviewers* box is +empty, and re-ticking it is how to put the pause back for a risky release. **A `workflow_dispatch` run.** Anyone with write access can start one. The job's `if` refuses a -dispatch from anybody but `github.repository_owner`, and refuses any ref that is not `main`. The -environment reviewer would catch it regardless; the condition keeps a run that was never going to -deploy from queueing for approval. - -**Reading the token out of a workflow on a branch.** This is the one a branch rule does not cover. -A repository secret is readable by any workflow in the repository, so a `.github/workflows/*.yml` -added on a feature branch could have printed `FLY_API_TOKEN` and deployed from a laptop afterwards -— no merge and no approval involved. `FLY_API_TOKEN` is therefore an **environment** secret on +dispatch from anybody but `github.repository_owner`, refuses any ref that is not `main`, and refuses +one whose `confirm` input is not the word `deploy`. With both the gate job and the reviewer gone, +those three are the whole of what stands in front of a manual release rather than a tidy-up in front +of one — the input in particular is there because a stray *Run workflow* click would otherwise +replace the container that is serving, which mid-match drops the live screen's circuits. The `push` +arm of the same condition tests the ref and nothing else, so a merge still deploys with nothing to +fill in. + +**Reading the token out of a workflow on a branch.** This is the one a branch rule does not cover, +and the one the removed approval never covered either. A repository secret is readable by any +workflow in the repository, so a `.github/workflows/*.yml` added on a feature branch could have +printed `FLY_API_TOKEN` and deployed from a laptop afterwards — no merge involved. +`FLY_API_TOKEN` is therefore an **environment** secret on `production`, not a repository secret, and `production` sets its deployment branch policy to `main` only. A job that does not name the environment cannot see the secret; a job that does name it is refused on any other ref, before it starts. Keeping a copy at repository level would reopen the @@ -199,20 +247,26 @@ hole silently, so there must not be one. Setting that up is manual — nothing in a repository can grant itself these, for the same reason a repository cannot grant itself branch protection: -> *Settings → Environments → New environment → `production`* → tick **Required reviewers** and add -> yourself → under **Deployment branches and tags** choose *Selected branches* and add `main` → +> *Settings → Environments → New environment → `production`* → leave **Required reviewers** +> unticked → under **Deployment branches and tags** choose *Selected branches* and add `main` → > **Save**. Then *Environment secrets → Add secret* → `FLY_API_TOKEN`, and **delete the > repository-level secret of the same name** under *Settings → Secrets and variables → Actions*. +The reviewer box is the only part of that which is a preference rather than a rule. Ticking it and +adding yourself is how a deploy goes back to holding at *Waiting* until somebody clicks *Approve* — +worth doing for a release you want to sit on, and worth unticking again afterwards, because a run +left waiting parks every later one behind it (see the concurrency note below). + **Create the environment before this workflow reaches `main`.** A job naming an environment that -does not exist does not fail — GitHub creates it silently, with no reviewer and no branch policy — -and repository secrets are still visible to it. The first merge would deploy straight through, and -nothing about the run would look wrong. +does not exist does not fail — GitHub creates it silently, with no branch policy — and repository +secrets are still visible to it. The branch policy is the whole point of it now, so the difference +between the environment being there and being conjured is not something the run would show you. -**An unapproved run holds the concurrency group.** `concurrency: fly-deploy` has -`cancel-in-progress: false`, so a release left waiting for approval parks every later one behind -it. That is the right trade for a single volume that migrates on boot, but it means "approve or -cancel" rather than "ignore". +**Deploys queue, they do not overlap.** `concurrency: fly-deploy` has `cancel-in-progress: false`, +so two merges close together release in order rather than racing — the right trade for a single +volume that migrates on boot. Nothing waits for a person any more, so the queue drains on its own; +it only stops draining if the reviewer box has been ticked and a run is sitting at *Waiting*, and +then it is "approve or cancel" rather than "ignore". **Outside GitHub, the token and the Fly org are the boundary.** The workflow's token is scoped with `flyctl tokens create deploy --app gjs-meiden`, so it can deploy this app and nothing else. Anyone @@ -224,8 +278,8 @@ fly orgs show # who else can deploy this app at all fly tokens list # revoke anything unrecognised with: fly tokens revoke ``` -Manual deploys from the repo root still work, and deliberately skip every gate above — the approval, -the CI gate, and the smoke check: +Manual deploys from the repo root still work, and deliberately skip everything above — the checks, +the merge, and the smoke check: ```powershell fly deploy @@ -243,7 +297,41 @@ because the honest answer is not "the ruleset stops them". **Merging needs write access, and nobody else has any.** A stranger's pull request is a request; the merge button is only rendered for a collaborator. The ruleset on `main` is the second line, not the first — it means even an account *with* write access cannot push to `main` directly or merge past a -red `Build and test`, because `bypass_actors` is empty. +red check, because `bypass_actors` is empty. + +**Nobody joins a repository; they are let in.** Since a merge is now a release, it is worth being +exact about the ways another account could ever come to press that button. There are five, and every +one of them starts with an action taken here: + +| Route | What it takes | Could they merge? | +|---|---|---| +| A collaborator invite | *Settings → Collaborators → Add people*, and they accept | Yes, at Write and above | +| A GitHub App | Installing one that asks for *Contents: write* / *Pull requests: write* | Yes, its token merges as the repository | +| A deploy key | Adding one with **Allow write access** ticked | It can push branches; `main` still needs a pull request | +| An Actions token | A workflow declaring `contents: write`, added by somebody who can push a branch | Only a pull request that already satisfies every rule — the ruleset binds `GITHUB_TOKEN` too | +| The owner's account | A stolen token, or no second factor | Yes, as the owner | + +Nothing a stranger can do unilaterally appears in that table, which is the point. What it also shows +is where the guard stops: **the ruleset covers `main` only**, so an invited collaborator could push +feature branches freely and open as many pull requests as they liked — they simply could not land +one past four red checks. + +The Actions row is the subtle one, and worth reading twice. *Workflow permissions: read-only* in the +repository settings is a **default, not a ceiling** — a workflow that declares +`permissions: contents: write` gets it regardless, and a workflow added on a feature branch runs +from that branch without being merged first (which is the same property that makes an environment +secret, rather than a repository secret, the thing protecting `FLY_API_TOKEN` above). What stops +that workflow merging anything it likes is not the setting, it is the ruleset: `GITHUB_TOKEN` is +bound by required checks and by `bypass_actors` being empty, exactly like a person. The setting +still matters for the workflow that forgets to say anything, which is most of them. + +**The lever not pulled, on purpose.** A *second* ruleset restricting **updates** to `main`, with +bypass granted only to the *Repository admin* role, would mean a Write collaborator, an installed +app and an Actions token could none of them merge — only an admin. It is not here because with one +admin and no other collaborator it guards nothing that is not already shut, and a bypass list is a +thing that rots quietly. Add it on the day somebody is invited, as its own ruleset: bypass is granted +per ruleset, so putting the admin role on the existing one would exempt admins from the four checks +as well. **Deploying needs the token, and a fork cannot see it.** GitHub withholds every secret from a workflow triggered by a fork's pull request and issues it a read-only `GITHUB_TOKEN`. Since @@ -275,9 +363,8 @@ writable token, and it should not. repositories in an organisation. Making this repository private is the only thing that removes fork pull requests and drive-by issues outright, and it is a product decision rather than a security one. -**The realistic route to a stranger merging is none of the above.** It is the owner's account, an -installed GitHub App carrying write access, or a deploy key with write enabled — none of which a -branch rule touches. Worth confirming periodically: +**The realistic route to a stranger merging is the table above, not the fork.** None of those five +is something a branch rule touches. Worth confirming periodically: | Where | What to check | |---|---| @@ -287,9 +374,9 @@ branch rule touches. Worth confirming periodically: | *Settings → Actions → General* | Workflow permissions **read-only**, and *Allow GitHub Actions to create and approve pull requests* **off** | | Account settings | Two-factor authentication or a passkey on the owner account | -The three workflows here each declare `permissions: contents: read` at the top, so the token they -get is read-only regardless of what the repository default is set to. The repository default still -matters for anything added later that forgets to. +Both workflows here declare `permissions: contents: read` at the top, so the token they get is +read-only regardless of what the repository default is set to. The repository default still matters +for anything added later that forgets to. ## A deploy has to prove it serves diff --git a/docs/project_overview.md b/docs/project_overview.md index 6181bde..f2d7256 100644 --- a/docs/project_overview.md +++ b/docs/project_overview.md @@ -17,7 +17,7 @@ src/ ├── FootballFormation.UI — Razor Class Library: pages, components, theming, layout └── FootballFormation.Web — Blazor host: Program.cs, App.razor, Routes.razor tests/ -└── FootballFormation.Core.Tests — xUnit; runs as a CI gate before deploy (see testing.md) +└── FootballFormation.Core.Tests — xUnit; one of the four checks a merge waits for (see testing.md) ``` UI is a separate RCL for future **MAUI Blazor Hybrid** reuse. Statistics and playing-time diff --git a/docs/testing.md b/docs/testing.md index 760a6c8..06fb576 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -2,10 +2,16 @@ `tests/FootballFormation.Core.Tests` — xUnit v3. Run with `dotnet test` from the repo root. -CI runs `dotnet build -c Release` and `dotnet test` as a **gate**: the Fly.io deploy job depends on -it, so nothing reaches the production volume that doesn't compile and pass. Before that gate -existed a push to `main` deployed straight to production, and the app auto-migrates on boot — a bad -migration reached a live database on startup. +CI runs `dotnet build -c Release` and `dotnet test` as a **gate on the merge**: it is one of the four +checks that have to be green before a pull request can land, and since landing is what deploys, a +commit that does not compile or does not pass never becomes a release. The gate used to sit one step +later — the deploy job depended on a re-run of this workflow — and it moved forward when merging +became the release. What stands between a merge and the volume now is the Docker build, which +compiles the app again and fails the deploy if it cannot, and the `/health` smoke check, which +refuses to call the release good until the new commit is the one answering. + +Getting that ordering right matters because the app auto-migrates on boot: a bad migration reaches a +live database on startup, and the only cheap place to catch it is here. ## What is covered @@ -207,9 +213,10 @@ Everything lives in `.github/workflows/ci.yml`, in four jobs on one chain: ``` Build and test ──┬── Coverage - (required) ├── Playwright + ├── Playwright └── Visual check - (all three advisory) + + (all four are required checks) ``` **`Build and test`** restores, builds Release, runs `dotnet test`, then publishes — and the publish @@ -228,9 +235,10 @@ seconds. The verdict, the whole-project line and branch numbers, and a per-file uncovered line numbers are written to `$GITHUB_STEP_SUMMARY` — the run's own front page, so the result is read without opening a log or downloading the Cobertura report. -Advisory, like the browser jobs below: it goes red under the floor without blocking a merge, since -`main`'s ruleset names **Build and test** and nothing else. Promoting it is one line in -`.github/rulesets/main-build-and-test.json` and nothing in the workflow. +Blocking, like the browser jobs below: `main`'s ruleset names all four checks, so going under the +floor keeps the merge button disabled. That is safe to require because the floor is on the lines a +change *touched* — a pull request with no coverable line in it measures nothing and passes, rather +than wedging on a division by zero. **`Playwright` and `Visual check`** download that artifact and start it. Neither calls a compiler; they install the .NET SDK only for the runtime to run `dotnet FootballFormation.Web.dll` with. @@ -262,10 +270,10 @@ Playwright moves the browser revision without moving `package.json`. **`pull_request`, and that is the whole of it.** The merge ref is the thing worth building: `actions/checkout` resolves a `pull_request` event to `refs/pull/N/merge`, the branch already merged -into `main`, where a push event checks out the branch tip on its own. With -`strict_required_status_checks_policy` off (see `deployment.md`) a branch can merge without being -rebuilt against a moved `main`, so this is the only event that covers that combination. It covers a -fork's pull request too. +into `main`, where a push event checks out the branch tip on its own. Merging is what deploys and +nothing re-runs on `main` afterwards, so this event is the last word on the commit that reaches the +volume — `strict_required_status_checks_policy` is on (see `deployment.md`) so the branch cannot +have gone stale underneath it. It covers a fork's pull request too. `ci.yml` used to carry a `push` trigger as well, and `ui-checks.yml` ran on push alone, so a pull request built four times over two files. The push trigger had a real reason once — GitHub starts no @@ -279,9 +287,10 @@ app-token behaviour ever regresses, the symptom is a pull request whose checks n than a red one — `workflow_dispatch` is the escape hatch and runs the browsers too, and one more commit on the branch also does it. -**`workflow_call`** is `fly-deploy.yml` asking for its gate, and the two browser jobs skip that -event by `if`. A deploy waits on the build and the unit tests; an advisory browser job should not -stand between a merged pull request and a release. +**`workflow_dispatch`** is the escape hatch, and the only other trigger. `ci.yml` used to expose a +**`workflow_call`** as well, which is how `fly-deploy.yml` re-ran the build as its gate — the browser +jobs opted out of that event so a flake could not hold up a release. Both are gone: the deploy has no +gate job any more, the merge is the release, and every job here blocks the merge instead. ### Is it stable enough for CI? @@ -291,10 +300,11 @@ busy loops competing for them, which stretched a run to 2.2–2.5 minutes and ch That is the retry-on-outcome design doing its job — `clickFor` absorbs a slow circuit instead of failing on it. -It runs on every pull request as the `playwright` job in **`.github/workflows/ci.yml`**. `main`'s -ruleset requires only **Build and test**, so this check is advisory: it reports, it does not block. -Promoting it once it has a track record on real runners is one line in -`.github/rulesets/main-build-and-test.json` and nothing in the workflow. +It runs on every pull request as the `playwright` job in **`.github/workflows/ci.yml`**, and `main`'s +ruleset names it, so a red run holds the merge. It was advisory until merging became the release; +once nothing re-tests on `main`, a browser failure has to be dealt with on the pull request or not at +all. The other side of that trade is that a flake blocks too — re-run the job from the run's page, +because `.github/rulesets/main-every-check-green.json` grants no bypass to anyone. The job compiles nothing — it starts the app the `Build and test` job published, so a cold compile is never competing with the `webServer` start-up timeout. It installs only Chromium. On a failure it @@ -357,7 +367,7 @@ also a Dutch test. Specs share one app and one database and run in a single worker, so they stay out of each other's way by naming what they create after themselves rather than by counting rows. -Runs on every pull request as an advisory check — see "Is it stable enough for CI?" above. +Runs on every pull request as a required check — see "Is it stable enough for CI?" above. ## Visual checks @@ -370,8 +380,9 @@ touch target is under its floor. Setting `VISUAL_APP_DLL` to a published `FootballFormation.Web.dll` skips the build and runs that copy instead — which is how the `Visual check` job does it, against what `Build and test` published. -It runs on every pull request as the `visual` job in `ci.yml` — advisory, like the Playwright job -beside it. That job uploads `artifacts/visual/` whether it passed or not: the +It runs on every pull request as the `visual` job in `ci.yml` — required, like the Playwright job +beside it, so a page that stops rendering stops the merge. That job uploads `artifacts/visual/` +whether it passed or not: the measurements are the part that can fail, but the screenshots are worth a look on a pull request that changed a page, and nothing else in CI produces one. Locally the harness drives the Chromium in a Claude Code web container; everywhere else `visual-check.mjs` lets Playwright resolve its own, which