Branch protection: required check names may contain commas (#182)
* Branch protection: required check names may contain commas
- CI / Typecheck and bundlesuccess
- CI / Unit tests (1/2)success
- CI / Unit tests (2/2)success
- Deploy / Deploy to Cloudflaresuccess
- End-to-end / End-to-end (agent reviews, tasks, Today)success
- End-to-end / End-to-end (git, PRs, Actions)success
- End-to-end / End-to-end (large pushes)success
- End-to-end / End-to-end (passkeys, registry, email)success
- End-to-end / End-to-end (real histories)success
- End-to-end / End-to-end (security)success
- Live test / Containers, registry, sandboxes, webhooks, schedules, LFS and pull requestssuccess
6 files changed+53−11
Files (6)
modifiedapps/diffinery/docs/collaboration/checks-and-statuses.md+1−1View file
| @@ -282,7 +282,7 @@ | ||
| 282 | 282 | |
| 283 | 283 | A branch protection rule's **Required status checks** (`requiredChecks` in the API) lists check names that must pass on a pull request's head commit before it can merge, and on a merge group's commit before the merge queue merges it. Both check runs and commit statuses count: a required check named `ci/external` is satisfied by a check run named `ci/external` or by a commit status with context `ci/external`. |
| 284 | 284 | |
| 285 | -To set them in the web UI, open **Settings**, then **Branch protection**, and enter the names, comma-separated, in **Required status checks**, exactly as they appear on pull requests. See [Branch protection](/docs/repositories/branch-protection) for the whole rule. | |
| 285 | +To set them in the web UI, open **Settings**, then **Branch protection**, and enter the names in **Required status checks**, one per line, exactly as they appear on pull requests. A name may contain commas, for example `CI / test (ubuntu, 22)`. See [Branch protection](/docs/repositories/branch-protection) for the whole rule. | |
| 286 | 286 | |
| 287 | 287 | ### How a required check is decided |
| 288 | 288 |
modifiedapps/diffinery/docs/collaboration/merge-queue.md+1−1View file
| @@ -24,7 +24,7 @@ | ||
| 24 | 24 | |
| 25 | 25 | 1. Open the repository's **Settings** and choose **Branch protection** (`https://diffinery.com/acme/website/settings/branches`). |
| 26 | 26 | 2. Open the rule for the branch (for example `main`), or fill in **Add a rule** with `main` as the **Branch name pattern**. |
| 27 | -3. In **Required status checks**, list the checks that must pass, comma-separated and exactly as they appear on pull requests, for example `CI / test, CI / lint`. | |
| 27 | +3. In **Required status checks**, list the checks that must pass, one per line and exactly as they appear on pull requests, for example `CI / test` and `CI / lint`. | |
| 28 | 28 | 4. Tick **Require a merge queue**, then set: |
| 29 | 29 | - **Merge method**: **Create a merge commit**, **Squash and merge** or **Rebase and merge**. |
| 30 | 30 | - **Build concurrency**: how many merge groups are built and checked at once. |
modifiedapps/diffinery/docs/repositories/branch-protection.md+1−1View file
| @@ -209,7 +209,7 @@ | ||
| 209 | 209 | 1. Open the repository, for example `https://diffinery.com/acme/website`. |
| 210 | 210 | 2. Select **Settings**, then **Branch protection** in the sidebar (`https://diffinery.com/acme/website/settings/branches`). |
| 211 | 211 | 3. Under **Add a rule**, enter the **Branch name pattern**, for example `main`. The form starts with **Require a pull request before merging** checked and **Required approvals** set to 1. |
| 212 | -4. Set **Required approvals**, enter **Required status checks** as a comma-separated list of check names, and check the options you want. | |
| 212 | +4. Set **Required approvals**, enter **Required status checks** one check name per line (names may contain commas, as matrix jobs' do), and check the options you want. | |
| 213 | 213 | 5. To use a merge queue, check **Require a merge queue** and choose the **Merge method**, **Build concurrency** and **Check timeout (minutes)**. |
| 214 | 214 | 6. Select **Create rule**. |
| 215 | 215 |
modifiedapps/diffinery/src/repo/required.ts+28−0View file
| @@ -12,6 +12,34 @@ | ||
| 12 | 12 | return (c.creatorLogin ?? c.app).toLowerCase(); |
| 13 | 13 | } |
| 14 | 14 | |
| 15 | +/** | |
| 16 | + * The required checks typed into the branch protection form: one per line. On a single line, commas | |
| 17 | + * separate names, except inside parentheses or brackets, so a matrix job such as | |
| 18 | + * "End-to-end (git, PRs, Actions)" stays one name. | |
| 19 | + */ | |
| 20 | +export function parseCheckList(text: string): string[] { | |
| 21 | + const names: string[] = []; | |
| 22 | + for (const line of text.split(/\r?\n/)) { | |
| 23 | + if (text.includes("\n")) { | |
| 24 | + names.push(line); | |
| 25 | + continue; | |
| 26 | + } | |
| 27 | + let depth = 0; | |
| 28 | + let start = 0; | |
| 29 | + for (let i = 0; i < line.length; i++) { | |
| 30 | + const ch = line[i]; | |
| 31 | + if (ch === "(" || ch === "[") depth++; | |
| 32 | + else if ((ch === ")" || ch === "]") && depth > 0) depth--; | |
| 33 | + else if (ch === "," && depth === 0) { | |
| 34 | + names.push(line.slice(start, i)); | |
| 35 | + start = i + 1; | |
| 36 | + } | |
| 37 | + } | |
| 38 | + names.push(line.slice(start)); | |
| 39 | + } | |
| 40 | + return [...new Set(names.map((n) => n.trim()).filter(Boolean))]; | |
| 41 | +} | |
| 42 | + | |
| 15 | 43 | export type RequiredState = { state: "success" | "failure" | "pending" | "missing"; conclusion: string | null; status: string | null }; |
| 16 | 44 | |
| 17 | 45 | /** |
modifiedapps/diffinery/src/web/repo-settings.tsx+6−7View file
| @@ -7,6 +7,7 @@ | ||
| 7 | 7 | import { HttpError } from "../lib/util.js"; |
| 8 | 8 | import { MAX_REVIEWERS, MAX_WAIT_TIMER, type Environment } from "../repo/deployments.js"; |
| 9 | 9 | import { resolveReviewers } from "../http/environment-reviews.js"; |
| 10 | +import { parseCheckList } from "../repo/required.js"; | |
| 10 | 11 | import type { ProtectionRules, Role } from "../repo/types.js"; |
| 11 | 12 | import { ICONS } from "./assets.js"; |
| 12 | 13 | import { KCheckbox, KRadio, KSelect, type KOption } from "./kumo.js"; |
| @@ -123,8 +124,10 @@ | ||
| 123 | 124 | </div> |
| 124 | 125 | <div class="field"> |
| 125 | 126 | <label>Required status checks</label> |
| 126 | - <input name="requiredChecks" type="text" value={(rules.requiredChecks ?? []).join(", ")} placeholder="ci / test, lint" /> | |
| 127 | - <div class="hint">Comma-separated check names, exactly as they appear on pull requests.</div> | |
| 127 | + <textarea name="requiredChecks" class="mono" style="min-height:80px" autocomplete="off" spellcheck={false} placeholder={"CI / test\nlint"}> | |
| 128 | + {(rules.requiredChecks ?? []).join("\n")} | |
| 129 | + </textarea> | |
| 130 | + <div class="hint">One check name per line, exactly as it appears on pull requests. Names may contain commas.</div> | |
| 128 | 131 | </div> |
| 129 | 132 | <div class="field"> |
| 130 | 133 | <div class="kstack" style="gap:8px"> |
| @@ -186,11 +189,7 @@ | ||
| 186 | 189 | for (const [key] of RULE_FLAGS) if (f.b(key)) (rules as Record<string, unknown>)[key] = true; |
| 187 | 190 | const approvals = Math.max(0, Math.min(10, parseInt(f.s("requiredApprovals"), 10) || 0)); |
| 188 | 191 | if (approvals) rules.requiredApprovals = approvals; |
| 189 | - const checks = f | |
| 190 | - .s("requiredChecks") | |
| 191 | - .split(/[,\n]/) | |
| 192 | - .map((s) => s.trim()) | |
| 193 | - .filter(Boolean); | |
| 192 | + const checks = parseCheckList(f.s("requiredChecks")); | |
| 194 | 193 | if (checks.length) rules.requiredChecks = checks; |
| 195 | 194 | if (f.b("mergeQueue")) { |
| 196 | 195 | const method = f.s("mqMethod"); |
modifiedapps/diffinery/test/unit/required.test.ts+16−1View file
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import { describe, expect, it } from "vitest"; |
| 2 | -import { ACTIONS_APP, normalizeCheckSources, requiredCheck } from "../../src/repo/required.js"; | |
| 2 | +import { ACTIONS_APP, normalizeCheckSources, parseCheckList, requiredCheck } from "../../src/repo/required.js"; | |
| 3 | 3 | import type { Check } from "../../src/repo/collab.js"; |
| 4 | 4 | |
| 5 | 5 | type Entry = Partial<Check> & Pick<Check, "name" | "app" | "status">; |
| @@ -48,3 +48,18 @@ | ||
| 48 | 48 | for (const v of [null, [], "x", { ci: 1 }, { ci: "" }, { ci: "a b" }, { "": "x" }]) expect(() => normalizeCheckSources(v)).toThrow(); |
| 49 | 49 | }); |
| 50 | 50 | }); |
| 51 | + | |
| 52 | +describe("parseCheckList", () => { | |
| 53 | + it("takes one name per line, commas and all", () => { | |
| 54 | + expect(parseCheckList("CI / Typecheck and bundle\r\nEnd-to-end / End-to-end (git, PRs, Actions)\n\n lint \n")).toEqual(["CI / Typecheck and bundle", "End-to-end / End-to-end (git, PRs, Actions)", "lint"]); | |
| 55 | + }); | |
| 56 | + | |
| 57 | + it("splits a single line on commas outside parentheses and brackets", () => { | |
| 58 | + expect(parseCheckList("CI / Unit tests (1/2), End-to-end (git, PRs, Actions), build [linux, arm64], lint")).toEqual(["CI / Unit tests (1/2)", "End-to-end (git, PRs, Actions)", "build [linux, arm64]", "lint"]); | |
| 59 | + }); | |
| 60 | + | |
| 61 | + it("drops blanks and duplicates", () => { | |
| 62 | + expect(parseCheckList(" , lint,,lint ")).toEqual(["lint"]); | |
| 63 | + expect(parseCheckList("")).toEqual([]); | |
| 64 | + }); | |
| 65 | +}); |