Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/CODEOWNERS → .github/REVIEWERS
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/assign-reviewers.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 8 additions & 4 deletions scripts/assign-reviewers.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
),
Expand All @@ -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) {
Expand Down
20 changes: 20 additions & 0 deletions scripts/assign-reviewers.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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']) }),
);
Comment thread
cursor[bot] marked this conversation as resolved.
Resolved
});
});
Loading