diff --git a/.changes/pr-pipeline/perf-wake-skip-armed-held-e1011cc7.md b/.changes/pr-pipeline/perf-wake-skip-armed-held-e1011cc7.md new file mode 100644 index 00000000000..f412aa2fda6 --- /dev/null +++ b/.changes/pr-pipeline/perf-wake-skip-armed-held-e1011cc7.md @@ -0,0 +1,5 @@ +--- +bump: none +--- + +No release: pr-verify's `wake-pr-merge` job no longer dispatches pr-merge for a lanes App pull request that is already armed or carries `do-not-merge`; the scheduled sweep makes the same decision for those, so only the wake's run count changes. diff --git a/.github/scripts/pr-verify/workflow.test.mjs b/.github/scripts/pr-verify/workflow.test.mjs index 0bcb08397db..1baad8f699d 100644 --- a/.github/scripts/pr-verify/workflow.test.mjs +++ b/.github/scripts/pr-verify/workflow.test.mjs @@ -1187,10 +1187,11 @@ const wake = (env) => { mkdirSync(bin); const args = path.join(dir, "args"); writeFileSync(args, ""); - // The stand-in answers an author read with GH_AUTHOR, or fails it when that is unset. + // The stand-in answers an author read with GH_AUTHOR, fails it when that is + // unset, or prints GH_ERROR_BODY and fails, as gh does on an HTTP error. writeFileSync( path.join(bin, "gh"), - `#!/bin/sh\necho "$@" >>'${args}'\nif [ "$1" = api ]; then [ -n "$GH_AUTHOR" ] || exit 1; echo "$GH_AUTHOR"; fi\n`, + `#!/bin/sh\necho "$@" >>'${args}'\nif [ "$1" = api ]; then [ -z "$GH_ERROR_BODY" ] || { printf '%s' "$GH_ERROR_BODY"; exit 1; }; [ -n "$GH_AUTHOR" ] || exit 1; echo "$GH_AUTHOR"; fi\n`, { mode: 0o755 }, ); const run = spawnSync("bash", ["--noprofile", "--norc", "-c", script], { @@ -1222,22 +1223,38 @@ test("wake-pr-merge dispatches nothing on a fail, no verdict or a pull request n } }); -test("wake-pr-merge wakes only a lanes App pull request when it can read the author", () => { - const read = `api repos/${REPO}/pulls/41 --jq "\\(.user.id) \\(.user.type)"`; - const dispatch = `workflow run pr-merge.yml -R ${REPO} --ref main -f pr=41`; - const pass = { KEY_PR: "41", KEY_VERDICT: "pass", LANES_APP_ID: "7" }; - // The lanes App's pull request: read, then woken. - assert.deepEqual(wake({ ...pass, GH_AUTHOR: "7 Bot" }).gh, [read, dispatch]); - // Another author, or the App's id on a User: read, nothing woken. - for (const author of ["9 User", "7 User", "8 Bot"]) { - const run = wake({ ...pass, GH_AUTHOR: author }); +const WAKE_READ = `api repos/${REPO}/pulls/41 --jq "\\(.user.id) \\(.user.type) \\(.auto_merge != null) \\([.labels[].name] | index("do-not-merge") != null)"`; +const WAKE_DISPATCH = `workflow run pr-merge.yml -R ${REPO} --ref main -f pr=41`; +const WAKE_PASS = { KEY_PR: "41", KEY_VERDICT: "pass", LANES_APP_ID: "7" }; + +test("wake-pr-merge wakes only an unarmed, unheld lanes App pull request when it can read it", () => { + // The lanes App's pull request, unarmed and unheld: read, then woken. + assert.deepEqual(wake({ ...WAKE_PASS, GH_AUTHOR: "7 Bot false false" }).gh, [WAKE_READ, WAKE_DISPATCH]); + // A person's, Dependabot's, another App's, or the App's id on a User: read, nothing woken. + for (const author of ["9 User false false", "7 User false false", "8 Bot false false", "49699333 Bot false false"]) { + const run = wake({ ...WAKE_PASS, GH_AUTHOR: author }); assert.equal(run.status, 0, author); - assert.deepEqual(run.gh, [read], author); - assert.match(run.stdout, /not a lanes App pull request; nothing woken/, author); + assert.deepEqual(run.gh, [WAKE_READ], author); + assert.match(run.stdout, /::notice title=pr-verify wake::pr=41 verdict=pass not a lanes App pull request; nothing woken/, author); + } + // A failed read, or no usable lanes App id: woken anyway, pr-merge decides. + assert.deepEqual(wake(WAKE_PASS).gh, [WAKE_READ, WAKE_DISPATCH]); + // A failed read whose error body spans lines (an edge 502 page) is still a failed read. + assert.deepEqual(wake({ ...WAKE_PASS, GH_ERROR_BODY: "\n
502 Bad Gateway\n\n" }).gh, [WAKE_READ, WAKE_DISPATCH]); + for (const id of ["", "x"]) assert.deepEqual(wake({ ...WAKE_PASS, LANES_APP_ID: id, GH_AUTHOR: "9 User false false" }).gh, [WAKE_DISPATCH], id); +}); + +test("wake-pr-merge wakes no lanes App pull request already armed or held by do-not-merge", () => { + for (const [answer, why] of [ + ["7 Bot true false", "already armed"], + ["7 Bot true true", "already armed"], + ["7 Bot false true", "held \\(do-not-merge\\)"], + ]) { + const run = wake({ ...WAKE_PASS, KEY_VERDICT: "pass-notes", GH_AUTHOR: answer }); + assert.equal(run.status, 0, answer); + assert.deepEqual(run.gh, [WAKE_READ], answer); + assert.match(run.stdout, new RegExp(`::notice title=pr-verify wake::pr=41 verdict=pass-notes ${why}; nothing woken`), answer); } - // A failed read, or no usable lanes App id: woken anyway, pr-merge filters. - assert.deepEqual(wake(pass).gh, [read, dispatch]); - for (const id of ["", "x"]) assert.deepEqual(wake({ ...pass, LANES_APP_ID: id, GH_AUTHOR: "9 User" }).gh, [dispatch], id); }); // pr-verify-capture.yml, the capture the judges read (#12232). It runs the diff --git a/.github/workflows/pr-verify.yml b/.github/workflows/pr-verify.yml index b8367bff8b6..03ec81ee583 100644 --- a/.github/workflows/pr-verify.yml +++ b/.github/workflows/pr-verify.yml @@ -5712,15 +5712,29 @@ jobs: echo "::warning::wake-pr-merge: the passing job named no pull request number; nothing woken" exit 0 fi - # pr-merge arms only lanes App pull requests, so one author read here - # spares every other pass a whole pr-merge run. A failed read, or no - # lanes App id, dispatches anyway: pr-merge filters by author itself. + # pr-merge arms only lanes App pull requests, and the token broker + # mints for this wake only on one, so one read here spares every + # other pass a whole pr-merge run. One already armed or held by + # do-not-merge needs no wake: the scheduled sweep runs the same + # decision on it, disarms included, and ci-status holds do-not-merge + # in the merge queue. A failed read, or no lanes App id, dispatches + # anyway: pr-merge decides all of this itself. if [[ ${LANES_APP_ID:-} =~ ^[1-9][0-9]*$ ]] && - author=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$pr" --jq '"\(.user.id) \(.user.type)"'); then - if [ "$author" != "$LANES_APP_ID Bot" ]; then + answer=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$pr" \ + --jq '"\(.user.id) \(.user.type) \(.auto_merge != null) \([.labels[].name] | index("do-not-merge") != null)"') && + read -r id type armed held <<<"$answer"; then + if [ "$id $type" != "$LANES_APP_ID Bot" ]; then echo "::notice title=pr-verify wake::pr=$pr verdict=$verdict not a lanes App pull request; nothing woken" exit 0 fi + if [ "$armed" = true ]; then + echo "::notice title=pr-verify wake::pr=$pr verdict=$verdict already armed; nothing woken" + exit 0 + fi + if [ "$held" = true ]; then + echo "::notice title=pr-verify wake::pr=$pr verdict=$verdict held (do-not-merge); nothing woken" + exit 0 + fi fi if gh workflow run pr-merge.yml -R "$GITHUB_REPOSITORY" --ref main -f pr="$pr"; then echo "::notice title=pr-verify wake::pr=$pr verdict=$verdict pr-merge dispatched" diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 97dd293411e..dab91b24fa6 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1961,7 +1961,8 @@ the body must still close, or with no closing issue name by a plain `Refs:` line pass judged, linked the same way, alone or beside others (`merge-holds.mjs` `linkHold`), so a body edited after arming to drop its link or name another issue does not merge on that pass. A pass or pass-notes verdict wakes it for that one pull request: `pr-verify.yml`'s `wake-pr-merge` job -dispatches a PR-scoped `pr-merge.yml` run (`-f pr=