diff --git a/AssignReviewers/README.md b/AssignReviewers/README.md index 82b9461..cb162c2 100644 --- a/AssignReviewers/README.md +++ b/AssignReviewers/README.md @@ -20,9 +20,12 @@ What it does each time a PR is opened ready or marked ready: doesn't count). PRs merge quickly, so most people are at 0 most of the time and ties are common: they go to whoever was assigned to someone else's PR longest ago (anyone not assigned recently first), then rotate by PR number. -4. **Requests reviewers.** It requests the review team (default `embeddedreviewers`) unless someone - from it has already been requested or has reviewed, so restacks don't re-request approvers. An - assignee from outside the team is requested individually. +4. **Requests reviewers.** It requests each member of the review team (default `embeddedreviewers`) + individually, plus any assignee from outside the team. It leaves out the author, anyone already + requested, and anyone whose approval stands, so reruns and draft round-trips don't re-request + approvers, while a comment left early doesn't keep the rest of the team from being asked. The + team itself isn't requested: its auto-assignment would turn that back into requests for + everyone, approvers included. The author is never an assignee; a self-assignment is removed. Drafts, bot PRs and Graphite merge-queue PRs are skipped. Team review auto-assignment settings are left as they are. diff --git a/AssignReviewers/action.yml b/AssignReviewers/action.yml index 45eda07..6d192be 100644 --- a/AssignReviewers/action.yml +++ b/AssignReviewers/action.yml @@ -16,7 +16,9 @@ inputs: description: Team the domain approver is picked from. default: embeddedreviewersstaff review-team: - description: Team requested as reviewers on every ready PR. Defaults to the rotation team. + description: > + Team whose members are each requested as reviewers on every ready PR, except the author, + anyone already requested and anyone whose approval stands. Defaults to the rotation team. default: '' max-stack-depth: description: How many PRs down the stack to look for assignees to inherit. diff --git a/AssignReviewers/assign.js b/AssignReviewers/assign.js index 36dacaf..be6201b 100644 --- a/AssignReviewers/assign.js +++ b/AssignReviewers/assign.js @@ -7,8 +7,9 @@ // - Otherwise it picks the least-loaded available member of each team: fewest open, ready PRs // already assigned to them (not counting their own), skipping anyone the PTO calendar has out // today or next business day. Ties go to whoever was assigned longest ago. -// - The author is never an assignee (a self-assignment is removed). The review team is requested -// unless someone from it already is, and an assignee outside the team is requested individually. +// - The author is never an assignee (a self-assignment is removed). Each member of the review team, +// and any assignee outside it, is requested individually, unless they already are or their +// approval stands. // - A PR that already has a domain approver and a second assignee only gets the review requests // topped up, so reruns are harmless and hand-picked assignees are kept. @@ -127,24 +128,29 @@ module.exports = async ({ github, context, core, inputs, sleep = (ms) => new Pro await github.rest.issues.removeAssignees({ owner, repo, issue_number: pr.number, assignees: [author] }); notes.push(`removed author ${author} from assignees`); } - const reviewed = new Set( - humanLogins((await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100 })).map((r) => r.user)), - ); + // Everyone on the review team, and any assignee from outside it (a designated domain owner, say), + // is requested individually rather than as a team: the team's auto-assignment turns a team + // request back into the same individual requests, approvers included, so the only way to leave + // out the people who don't need asking is per person. That's the author, anyone already + // requested, and anyone whose approval stands. A review that's only a comment, or an approval + // since dismissed or replaced by a change request, doesn't count, so an early comment doesn't + // keep anyone else from being asked. + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100 }); + const decisive = new Map(); + for (const review of reviews) { + if (!isBot(review.user) && (review.state === 'APPROVED' || review.state === 'CHANGES_REQUESTED')) decisive.set(review.user.login, review.state); + } + const approved = (login) => decisive.get(login) === 'APPROVED'; const requested = new Set(humanLogins(current.requested_reviewers)); - const involved = (login) => requested.has(login) || reviewed.has(login); - // The whole review team is requested, as authors used to do by hand, unless someone from it - // already has been or has reviewed (so reruns and restacks don't re-request approvers). + // A team that's already requested (by hand, say) already covers its members. const reviewTeamMembers = await teamMembers(github, owner, reviewTeam); - const teamAlreadyRequested = (current.requested_teams || []).some((t) => t.slug === reviewTeam); - if (!teamAlreadyRequested && !reviewTeamMembers.some((login) => login !== author && involved(login))) { - await github.rest.pulls.requestReviewers({ owner, repo, pull_number: pr.number, team_reviewers: [reviewTeam] }); - for (const login of reviewTeamMembers) requested.add(login); - notes.push(`requested team ${reviewTeam}`); - } - // Assignees from outside the team (a designated domain owner, say) are requested individually. - const toRequest = [...named].filter((login) => !involved(login)); + if ((current.requested_teams || []).some((t) => t.slug === reviewTeam)) for (const login of reviewTeamMembers) requested.add(login); + const toRequest = [...new Set([...reviewTeamMembers, ...named])].filter( + (login) => login !== author && !requested.has(login) && !approved(login), + ); if (toRequest.length > 0) { await github.rest.pulls.requestReviewers({ owner, repo, pull_number: pr.number, reviewers: toRequest }); + notes.push(`requested ${toRequest.join(', ')}`); } core.info(`Assignees: ${[...named].join(', ') || 'none'}${notes.length ? ` (${notes.join('; ')})` : ''}.`); diff --git a/AssignReviewers/assign.test.js b/AssignReviewers/assign.test.js index 63553f9..a7af712 100644 --- a/AssignReviewers/assign.test.js +++ b/AssignReviewers/assign.test.js @@ -165,7 +165,10 @@ test('bottom of stack: least-loaded staff approver and rotation reviewer, whole }); await run(fake, 10); assert.deepStrictEqual(fake.pr(10).assignees, ['staffB', 'devF']); - assert.deepStrictEqual(fake.pr(10).teams, ['embeddedreviewers']); + // Each member but the author, individually, in one request; never the team itself. + assert.deepStrictEqual(fake.pr(10).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF']); + assert.deepStrictEqual(fake.pr(10).teams, []); + assert.strictEqual(fake.calls.review.length, 1); }); test('staff author is never an assignee', async () => { @@ -342,7 +345,7 @@ test("replaces an author's self-assignment", async () => { assert.strictEqual(fake.pr(70).assignees.length, 2); }); -test('keeps hand-picked assignees and does not re-request a team someone already reviewed for', async () => { +test('keeps hand-picked assignees and does not re-request someone whose approval stands', async () => { const fake = fakeGithub({ prs: [ { @@ -351,15 +354,39 @@ test('keeps hand-picked assignees and does not re-request a team someone already head: 'a', base: 'main', assignees: ['staffB', 'devF'], + requested: ['devD'], reviews: [['staffB', 'APPROVED']], }, ], }); await run(fake, 71); assert.deepStrictEqual(fake.calls.assign, []); - assert.ok(fake.calls.review.every((c) => c.team_reviewers.length === 0)); - // devF hasn't reviewed or been requested, so they're requested individually. - assert.deepStrictEqual(fake.pr(71).requested, ['devF']); + // staffB approved and devD is already requested; everyone else on the team is asked. + assert.deepStrictEqual(fake.calls.review, [{ pull_number: 71, reviewers: ['staffA', 'staffC', 'devF'], team_reviewers: [] }]); +}); + +test('an early comment doesn\'t keep the rest of the team from being asked, and neither does a superseded approval', async () => { + // staffA commented while it was a draft; devD approved and then requested changes. + const fake = fakeGithub({ + prs: [{ number: 74, author: 'devE', head: 'a', base: 'main', assignees: ['staffA', 'devD'], reviews: [['staffA', 'COMMENTED'], ['devD', 'APPROVED'], ['devD', 'CHANGES_REQUESTED']] }], + }); + await run(fake, 74); + assert.deepStrictEqual(fake.pr(74).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF']); +}); + +test('a rerun after everyone was asked requests no one', async () => { + const fake = fakeGithub({ prs: [{ number: 75, author: 'devE', head: 'a', base: 'main' }] }); + await run(fake, 75); + await run(fake, 75); + assert.strictEqual(fake.calls.review.length, 1); +}); + +test('when the team itself is already requested, its members aren\'t asked again individually', async () => { + const fake = fakeGithub({ + prs: [{ number: 76, author: 'devE', head: 'a', base: 'main', assignees: ['staffA', 'ownerX'], teams: ['embeddedreviewers'] }], + }); + await run(fake, 76); + assert.deepStrictEqual(fake.pr(76).requested, ['ownerX']); }); test('requests an assignee from outside the team individually', async () => { @@ -370,8 +397,8 @@ test('requests an assignee from outside the team individually', async () => { ], }); await run(fake, 73); - assert.deepStrictEqual(fake.pr(73).teams, ['embeddedreviewers']); - assert.deepStrictEqual(fake.pr(73).requested, ['ownerX']); + assert.deepStrictEqual(fake.pr(73).teams, []); + assert.deepStrictEqual(fake.pr(73).requested, ['staffA', 'staffB', 'staffC', 'devD', 'devF', 'ownerX']); }); test('ignores drafts, bots and merge-queue PRs', async () => {