diff --git a/.github/CODEOWNERS b/.github/REVIEWERS similarity index 92% rename from .github/CODEOWNERS rename to .github/REVIEWERS index a6dc12f405db..e534da992d3d 100644 --- a/.github/CODEOWNERS +++ b/.github/REVIEWERS @@ -1,3 +1,8 @@ +# Reviewer routing for scripts/assign-reviewers.mjs, in CODEOWNERS syntax. +# This is deliberately not a CODEOWNERS file: GitHub requests every matching +# code owner team as soon as a PR opens, before the workflow requests anyone, +# so the whole team gets notified. + # Browser, replay, feedback, and related packages /packages/browser/ @getsentry/team-javascript-sdks-browser /packages/browser-utils/ @getsentry/team-javascript-sdks-browser diff --git a/.github/workflows/assign-reviewers.yml b/.github/workflows/assign-reviewers.yml index e8d11ae70c20..60e541dcb775 100644 --- a/.github/workflows/assign-reviewers.yml +++ b/.github/workflows/assign-reviewers.yml @@ -34,7 +34,7 @@ jobs: uses: actions/checkout@v7 with: sparse-checkout: | - .github/CODEOWNERS + .github/REVIEWERS scripts/assign-reviewers.mjs scripts/junior-requesters.mjs sparse-checkout-cone-mode: false diff --git a/scripts/assign-reviewers.mjs b/scripts/assign-reviewers.mjs index ee6122f71d39..2120f7bdd5cd 100644 --- a/scripts/assign-reviewers.mjs +++ b/scripts/assign-reviewers.mjs @@ -3,9 +3,10 @@ import * as fs from 'fs'; import { getJuniorRequesters, TEAM_SLUG } from './junior-requesters.mjs'; const REQUIRED_REVIEWERS = 3; -// Managers who are members (and code owners) of the SDK teams, but should never be auto-requested for review. +// Managers who are members of the SDK teams, but should never be auto-requested for review. +// GitHub's team auto-assignment can still pick them, so their pending requests don't count towards the required reviewers. const NEVER_REQUEST = ['mydea', 'stephanie-anderson']; -const CODEOWNERS_PATH = '.github/CODEOWNERS'; +const REVIEWERS_PATH = '.github/REVIEWERS'; function patternToRegExp(pattern) { let p = pattern; @@ -100,12 +101,15 @@ export async function assignReviewers({ github, context, core }) { // Whoever asked Junior for the PR is its de facto author. const requesters = await getJuniorRequesters({ github, core, pr: currentPr }); + const neverRequest = new Set(NEVER_REQUEST.map(login => login.toLowerCase())); + const requestedReviewers = currentPr.requested_reviewers.filter(user => !neverRequest.has(user.login.toLowerCase())); + const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number }); // Requested reviewers drop off `requested_reviewers` once they submit a review, so count both. // Bots (AI reviewers) don't count towards the required reviewers. const existing = [ ...new Set( - [...currentPr.requested_reviewers, ...reviews.map(r => r.user)] + [...requestedReviewers, ...reviews.map(r => r.user)] .filter(user => user && user.type !== 'Bot' && user.login.toLowerCase() !== author.toLowerCase()) .map(user => user.login), ), @@ -120,7 +124,7 @@ export async function assignReviewers({ github, context, core }) { (await github.paginate(github.rest.teams.listMembersInOrg, { org: owner, team_slug })).map(m => m.login); const files = (await github.paginate(github.rest.pulls.listFiles, { owner, repo, pull_number })).map(f => f.filename); - const codeowners = getOwnersForFiles(parseCodeowners(fs.readFileSync(CODEOWNERS_PATH, 'utf8')), files); + const codeowners = getOwnersForFiles(parseCodeowners(fs.readFileSync(REVIEWERS_PATH, 'utf8')), files); const codeownerPool = []; for (const codeowner of codeowners) { diff --git a/scripts/assign-reviewers.test.mjs b/scripts/assign-reviewers.test.mjs index 5ff6877615e6..0628ad394373 100644 --- a/scripts/assign-reviewers.test.mjs +++ b/scripts/assign-reviewers.test.mjs @@ -88,4 +88,24 @@ describe('assignReviewers', () => { expect.objectContaining({ reviewers: expect.arrayContaining(['b', 'c', 'd']) }), ); }); + + it('does not count pending review requests from managers towards the required reviewers', async () => { + const { github, run } = setup(assignReviewers, { + user: { login: 'author', type: 'User' }, + requested_reviewers: [ + { login: 'Mydea', type: 'User' }, + { login: 'a', type: 'User' }, + ], + }); + await run(); + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ reviewers: [expect.any(String), expect.any(String)] }), + ); + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ reviewers: expect.not.arrayContaining(['a']) }), + ); + expect(github.rest.pulls.requestReviewers).toHaveBeenCalledWith( + expect.objectContaining({ reviewers: expect.not.arrayContaining(['Mydea']) }), + ); + }); });