From 99f83f690eb3e4e691b93766ecab4f7d518acd59 Mon Sep 17 00:00:00 2001 From: Jasper Mayone Date: Mon, 21 Sep 2026 15:58:44 -0400 Subject: [PATCH] refactor: drop the reviewer rotation and rely on CODEOWNERS --- .github/dns-reviewers.txt | 13 ---- .github/workflows/assign-reviewer.yml | 96 --------------------------- CONTRIBUTING.md | 16 ++--- README.md | 10 ++- docs/runbook.md | 11 ++- 5 files changed, 19 insertions(+), 127 deletions(-) delete mode 100644 .github/dns-reviewers.txt delete mode 100644 .github/workflows/assign-reviewer.yml diff --git a/.github/dns-reviewers.txt b/.github/dns-reviewers.txt deleted file mode 100644 index a63baa0..0000000 --- a/.github/dns-reviewers.txt +++ /dev/null @@ -1,13 +0,0 @@ -# Round robin review rotation for DNS pull requests. -# -# One GitHub username per line. Lines that start with # are ignored. -# -# Everybody here must be a member of the @WITCodingClub/dns-managers team, -# because CODEOWNERS requires an approval from that team. This file only -# decides whose turn it is to be asked. Anybody on the team can still approve. -# -# To hand the rotation to somebody new, add their username and open a pull -# request. To pause somebody, comment their line out. - -jaspermayone -Cattn diff --git a/.github/workflows/assign-reviewer.yml b/.github/workflows/assign-reviewer.yml deleted file mode 100644 index 5403121..0000000 --- a/.github/workflows/assign-reviewer.yml +++ /dev/null @@ -1,96 +0,0 @@ -name: assign-reviewer - -# Picks the next person in the review rotation and asks them to review. -# -# GitHub can do this by itself with team review assignment, but that feature -# needs a paid organisation plan. This is the same idea in a workflow: the -# rotation is .github/dns-reviewers.txt and the turn is decided by the pull -# request number, so it moves on by one with every pull request. -# -# This workflow never checks out or runs code from the pull request, which is -# why `pull_request_target` is safe here. - -on: - pull_request_target: - types: [opened, ready_for_review] - -permissions: - contents: read - -jobs: - assign: - name: pick the next reviewer - runs-on: ubuntu-latest - if: github.event.pull_request.draft == false - permissions: - contents: read - pull-requests: write - env: - # CODEOWNERS asks the whole team for a review. Set this to false if you - # would rather leave that request in place and only add the individual. - REMOVE_TEAM_REQUEST: 'true' - TEAM_SLUG: dns-managers - steps: - - name: Check out main (for the rotation file) - uses: actions/checkout@v7 - with: - ref: ${{ github.event.pull_request.base.ref }} - sparse-checkout: .github/dns-reviewers.txt - sparse-checkout-cone-mode: false - - - name: Request a review - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number }} - AUTHOR: ${{ github.event.pull_request.user.login }} - run: | - set -eu - - rotation=$(grep -vE '^\s*(#|$)' .github/dns-reviewers.txt | tr -d ' \r') - count=$(printf '%s\n' "$rotation" | grep -c . || true) - if [ "$count" -eq 0 ]; then - echo "::warning title=No reviewers::.github/dns-reviewers.txt is empty." - exit 0 - fi - - # Already has somebody on it, so leave it alone. This keeps a reopen - # or a "ready for review" from piling on more reviewers. - existing=$(gh pr view "$PR" --repo "$REPO" \ - --json reviewRequests --jq '.reviewRequests | map(.login // empty) | length') - if [ "$existing" -gt 0 ]; then - echo "A reviewer is already requested. Nothing to do." - exit 0 - fi - - # Start at the pull request number and walk forward, so consecutive - # pull requests go to consecutive people, and skip the author. - reviewer='' - i=0 - while [ "$i" -lt "$count" ]; do - pick=$(( (PR + i) % count + 1 )) - candidate=$(printf '%s\n' "$rotation" | sed -n "${pick}p") - if [ "$candidate" != "$AUTHOR" ]; then - reviewer="$candidate" - break - fi - i=$(( i + 1 )) - done - - if [ -z "$reviewer" ]; then - echo "::notice title=No reviewer::$AUTHOR is the only person in the rotation." - exit 0 - fi - - echo "Asking $reviewer to review #$PR." - gh pr edit "$PR" --repo "$REPO" \ - --add-reviewer "$reviewer" \ - --add-assignee "$reviewer" - - if [ "$REMOVE_TEAM_REQUEST" = 'true' ]; then - # CODEOWNERS still requires an approval from the team. Dropping the - # team's review request only stops everybody being notified. - gh api -X DELETE "repos/$REPO/pulls/$PR/requested_reviewers" \ - -f "team_reviewers[]=$TEAM_SLUG" >/dev/null 2>&1 \ - || echo "The team was not requested, or could not be removed." - fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ef4297b..0a826c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,18 +36,16 @@ You are on the [DNS Managers team](https://github.com/orgs/WITCodingClub/teams/dns-managers). A pull request cannot merge without an approving review from one of you. -### Reviews go round the team +### Reviews go to the whole team -A workflow asks one person per pull request, in rotation. The rotation is -[`.github/dns-reviewers.txt`](./.github/dns-reviewers.txt). It skips the author -of the pull request. +`CODEOWNERS` makes the team a required reviewer, so GitHub asks all of you on +every pull request. Whoever gets to it first reviews it. -Being asked does not make it only your job. Anybody on the team can approve. -If you cannot get to a review, say so on the pull request so that somebody else -picks it up. +Nobody is assigned. That means a pull request can sit while each of you assumes +the other has it. If you start a review, say so on the pull request, and if you +cannot get to one, say that too. -To change the rotation, edit the file and open a pull request. Comment out a -line to pause somebody. Add a line to bring somebody in. +To change who reviews, change who is on the team. ### What to check in a review diff --git a/README.md b/README.md index eaa3033..d939e08 100644 --- a/README.md +++ b/README.md @@ -56,13 +56,11 @@ in this order by itself. ### 2. Open a pull request -A bot adds two things to your pull request: +A bot posts a **plan** on your pull request. It lists every record the merge +would create, change, or delete. Read it. If it shows something you did not +intend, fix your branch. -- **A plan.** It lists every record the merge would create, change, or delete. - Read it. If it shows something you did not intend, fix your branch. -- **A reviewer.** The rotation in - [`.github/dns-reviewers.txt`](./.github/dns-reviewers.txt) decides whose turn - it is. +The DNS Managers team is asked to review automatically. Push more commits to the same branch if the reviewer asks for changes. Do not close the pull request and open a new one. diff --git a/docs/runbook.md b/docs/runbook.md index 252c158..6183845 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -179,10 +179,15 @@ The zone files and Cloudflare now disagree. Fix it, do not leave it. 3. Run `gh workflow run deploy.yml` and confirm it passes. 4. Delete the old token in Cloudflare. -### Add somebody to the rotation +### Add somebody as a reviewer -1. Add them to the `dns-managers` team. -2. Add their GitHub username to `.github/dns-reviewers.txt` in a pull request. +Add them to the `dns-managers` team. `CODEOWNERS` points at the team, so that +is the only step. Check that the team still has write access: + +```console +$ gh api orgs/WITCodingClub/teams/dns-managers/repos/WITCodingClub/dns \ + --jq .permissions +``` ### Remove a subdomain