Skip to content

Commit 7b69412

Browse files
authored
Fix maintainer-approval not blocking PRs without approval (#4935)
## Why PR #4931 switched `maintainer-approval` from commit statuses (`createCommitStatus`) to check runs (`checks.create`) so the check is clickable in the GitHub UI. The pending state used `status: "in_progress"`, which GitHub treats as "still running" rather than "blocking". This meant all PRs could merge without maintainer approval. ## Changes Removes the three `checks.create` calls for pending states (wildcard files, uncovered groups, no approval). When no check run or status exists for `maintainer-approval` on a SHA, GitHub shows the required check as "Expected" (yellow dot) and blocks the merge. Approved PRs still get a success check run (green, clickable). The result: - **No approval**: yellow dot, merge blocked, reviewer info in PR comment - **Approved**: green checkmark, clickable, shows who approved - **Merge queue**: green checkmark, auto-approved (unchanged) ## Test plan - [x] All 20 tests in `maintainer-approval.test.js` pass - [ ] Verify on a subsequent PR (after merge) that `maintainer-approval` shows yellow "Expected" without approval, then turns green after approval Note: the workflow uses `pull_request_target`, so it runs from main. This PR cannot test itself.
1 parent 2b2cf62 commit 7b69412

2 files changed

Lines changed: 19 additions & 39 deletions

File tree

.github/workflows/maintainer-approval.js

Lines changed: 10 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,10 @@ module.exports = async ({ github, context, core }) => {
516516
core
517517
);
518518

519-
// Set commit status. Approved PRs return early (commit status is sufficient).
519+
// Approved PRs get a success check run and return early.
520+
// Pending PRs intentionally create NO check run or status. The required
521+
// status check "maintainer-approval" stays as "Expected" (yellow dot) in
522+
// the GitHub UI, which blocks the merge until approval is granted.
520523
if (result.allCovered && approverLogins.length > 0) {
521524
core.info("All ownership groups have per-path approval.");
522525
await github.rest.checks.create({
@@ -531,36 +534,20 @@ module.exports = async ({ github, context, core }) => {
531534

532535
if (result.hasWildcardFiles) {
533536
const fileList = result.wildcardFiles.join(", ");
534-
const msg =
537+
core.info(
535538
`Files need maintainer review: ${fileList}. ` +
536-
`Maintainers: ${maintainers.join(", ")}`;
537-
core.info(msg);
538-
await github.rest.checks.create({
539-
...checkParams,
540-
status: "in_progress",
541-
output: { title: STATUS_CONTEXT, summary: msg },
542-
});
539+
`Maintainers: ${maintainers.join(", ")}`
540+
);
543541
} else if (result.uncovered && result.uncovered.length > 0) {
544542
const groupList = result.uncovered
545543
.map(({ pattern, owners }) => `${pattern} (needs: ${owners.join(", ")})`)
546544
.join("; ");
547-
const msg = `Needs approval: ${groupList}`;
548545
core.info(
549-
`${msg}. Alternatively, any maintainer can approve: ${maintainers.join(", ")}.`
546+
`Needs approval: ${groupList}. ` +
547+
`Alternatively, any maintainer can approve: ${maintainers.join(", ")}.`
550548
);
551-
await github.rest.checks.create({
552-
...checkParams,
553-
status: "in_progress",
554-
output: { title: STATUS_CONTEXT, summary: msg },
555-
});
556549
} else {
557-
const msg = `Waiting for maintainer approval: ${maintainers.join(", ")}`;
558-
core.info(msg);
559-
await github.rest.checks.create({
560-
...checkParams,
561-
status: "in_progress",
562-
output: { title: STATUS_CONTEXT, summary: msg },
563-
});
550+
core.info(`Waiting for maintainer approval: ${maintainers.join(", ")}`);
564551
}
565552

566553
// Score contributors via git history

.github/workflows/maintainer-approval.test.js

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -251,12 +251,11 @@ describe("maintainer-approval", () => {
251251

252252
await runModule({ github, context, core });
253253

254-
assert.equal(github._checkRuns.length, 1);
255-
assert.equal(github._checkRuns[0].status, "in_progress");
256-
assert.ok(github._checkRuns[0].output.summary.includes("/bundle/"));
254+
// No check run created; the required check stays as "Expected" (yellow dot).
255+
assert.equal(github._checkRuns.length, 0);
257256
});
258257

259-
it("wildcard files present -> pending, mentions maintainer", async () => {
258+
it("wildcard files present -> pending, no check run", async () => {
260259
const github = makeGithub({
261260
reviews: [
262261
{ state: "APPROVED", user: { login: "randomreviewer" } },
@@ -268,12 +267,10 @@ describe("maintainer-approval", () => {
268267

269268
await runModule({ github, context, core });
270269

271-
assert.equal(github._checkRuns.length, 1);
272-
assert.equal(github._checkRuns[0].status, "in_progress");
273-
assert.ok(github._checkRuns[0].output.summary.includes("maintainer"));
270+
assert.equal(github._checkRuns.length, 0);
274271
});
275272

276-
it("no approvals at all -> pending", async () => {
273+
it("no approvals at all -> pending, no check run", async () => {
277274
const github = makeGithub({
278275
reviews: [],
279276
files: [{ filename: "cmd/pipelines/foo.go" }],
@@ -283,8 +280,7 @@ describe("maintainer-approval", () => {
283280

284281
await runModule({ github, context, core });
285282

286-
assert.equal(github._checkRuns.length, 1);
287-
assert.equal(github._checkRuns[0].status, "in_progress");
283+
assert.equal(github._checkRuns.length, 0);
288284
});
289285

290286
it("team member approved -> success for team-owned path", async () => {
@@ -317,8 +313,7 @@ describe("maintainer-approval", () => {
317313

318314
await runModule({ github, context, core });
319315

320-
assert.equal(github._checkRuns.length, 1);
321-
assert.equal(github._checkRuns[0].status, "in_progress");
316+
assert.equal(github._checkRuns.length, 0);
322317
});
323318

324319
it("CHANGES_REQUESTED does not count as approval", async () => {
@@ -333,8 +328,7 @@ describe("maintainer-approval", () => {
333328

334329
await runModule({ github, context, core });
335330

336-
assert.equal(github._checkRuns.length, 1);
337-
assert.equal(github._checkRuns[0].status, "in_progress");
331+
assert.equal(github._checkRuns.length, 0);
338332
});
339333

340334
it("self-approval by PR author is excluded", async () => {
@@ -349,8 +343,7 @@ describe("maintainer-approval", () => {
349343

350344
await runModule({ github, context, core });
351345

352-
assert.equal(github._checkRuns.length, 1);
353-
assert.equal(github._checkRuns[0].status, "in_progress");
346+
assert.equal(github._checkRuns.length, 0);
354347
});
355348

356349
it("no * rule in OWNERS -> setFailed", async () => {

0 commit comments

Comments
 (0)