diff --git a/README.md b/README.md index 3efebd0..e922bd0 100644 --- a/README.md +++ b/README.md @@ -29,8 +29,15 @@ reviewers: - reviewerB - reviewerC +# A list of team reviewers to be added to pull requests (GitHub team slug) +team_reviewers: + - org/teamReviewerA + - org/teamReviewerB + - teamReviewerC + # A number of reviewers added to the pull request # Set 0 to add all the reviewers (default: 0) +# Note: This only affects individual reviewers, not team reviewers numberOfReviewers: 0 # A list of assignees, overrides reviewers if set diff --git a/auto_assign_example.yml b/auto_assign_example.yml new file mode 100644 index 0000000..652f686 --- /dev/null +++ b/auto_assign_example.yml @@ -0,0 +1,42 @@ +# Configuration example for auto-assign with separate team_reviewers + +# Set to true to add reviewers to pull requests +addReviewers: true + +# Set to true to add assignees to pull requests +addAssignees: false + +# A list of individual reviewers to be added to pull requests (GitHub user name) +reviewers: + - reviewerA + - reviewerB + - reviewerC + +# A list of team reviewers to be added to pull requests (GitHub team slug) +# Teams can be specified with or without the org prefix +team_reviewers: + - frontend-team + - backend-team + - org/security-team + +# A number of individual reviewers added to the pull request +# Set 0 to add all the reviewers (default: 0) +# Note: This only affects individual reviewers, not team reviewers +numberOfReviewers: 2 + +# A list of assignees, overrides reviewers if set +# assignees: +# - assigneeA + +# A number of assignees to add to the pull request +# Set to 0 to add all of the assignees. +# Uses numberOfReviewers if unset. +# numberOfAssignees: 2 + +# A list of keywords to be skipped the process that add reviewers if pull requests include it +# skipKeywords: +# - wip + +# A list of users to be skipped by both the add reviewers and add assignees processes +# skipUsers: +# - dependabot[bot] diff --git a/src/handler.ts b/src/handler.ts index 25c5299..894215d 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -9,6 +9,7 @@ export interface Config { addReviewers: boolean addAssignees: boolean reviewers: string[] + teamReviewers: string[] assignees: string[] numberOfAssignees: number numberOfReviewers: number diff --git a/src/util.ts b/src/util.ts index 7d9e38e..1a3e511 100644 --- a/src/util.ts +++ b/src/util.ts @@ -62,7 +62,13 @@ export function chooseReviewers( reviewers: string[] team_reviewers: string[] } { - const { useReviewGroups, reviewGroups, numberOfReviewers, reviewers } = config + const { + useReviewGroups, + reviewGroups, + numberOfReviewers, + reviewers, + teamReviewers: team_reviewers, + } = config const useGroups: boolean = useReviewGroups && Object.keys(reviewGroups).length > 0 @@ -79,11 +85,21 @@ export function chooseReviewers( } } - const chosenReviewers = chooseUsers(reviewers, numberOfReviewers, owner) + // Filter out the owner from individual reviewers + const filteredReviewers = reviewers + ? reviewers.filter((reviewer) => reviewer !== owner) + : [] + + // Choose individual reviewers + const chosenIndividualReviewers = + numberOfReviewers > 0 + ? _.sampleSize(filteredReviewers, numberOfReviewers) + : filteredReviewers + // Return both individual reviewers and team reviewers return { - reviewers: chosenReviewers.users, - team_reviewers: chosenReviewers.teams, + reviewers: chosenIndividualReviewers, + team_reviewers: team_reviewers || [], } } diff --git a/test/handler.test.ts b/test/handler.test.ts index b568ff6..f977f7e 100644 --- a/test/handler.test.ts +++ b/test/handler.test.ts @@ -142,7 +142,8 @@ describe('handlePullRequest', () => { addAssignees: false, addReviewers: true, numberOfReviewers: 0, - reviewers: ['/team_reviewer1'], + reviewers: [], + teamReviewers: ['team_reviewer1'], skipKeywords: ['wip'], } }) @@ -1008,4 +1009,73 @@ describe('handlePullRequest', () => { expect(requestReviewersSpy).not.toBeCalled() expect(addAssigneesSpy).not.toBeCalled() }) + + test('adds team reviewers from separate team_reviewers config', async () => { + // GIVEN + context.config = jest.fn().mockImplementation(async () => { + return { + addReviewers: true, + addAssignees: false, + reviewers: ['reviewer1', 'reviewer2'], + teamReviewers: ['team1', 'org/team2'], + numberOfReviewers: 0, + } + }) + + context.octokit.pulls = { + requestReviewers: jest.fn().mockImplementation(async () => ({})), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any + + const requestReviewersSpy = jest.spyOn( + context.octokit.pulls, + 'requestReviewers' + ) + + // WHEN + await handlePullRequest(context) + + // THEN + expect(requestReviewersSpy).toBeCalledTimes(1) + expect(requestReviewersSpy.mock.calls[0][0]?.reviewers).toEqual([ + 'reviewer1', + 'reviewer2', + ]) + expect(requestReviewersSpy.mock.calls[0][0]?.team_reviewers).toEqual([ + 'team1', + 'org/team2', + ]) + }) + + test('handles empty team_reviewers config gracefully', async () => { + // GIVEN + context.config = jest.fn().mockImplementation(async () => { + return { + addReviewers: true, + addAssignees: false, + reviewers: ['reviewer1'], + numberOfReviewers: 0, + } + }) + + context.octokit.pulls = { + requestReviewers: jest.fn().mockImplementation(async () => ({})), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any + + const requestReviewersSpy = jest.spyOn( + context.octokit.pulls, + 'requestReviewers' + ) + + // WHEN + await handlePullRequest(context) + + // THEN + expect(requestReviewersSpy).toBeCalledTimes(1) + expect(requestReviewersSpy.mock.calls[0][0]?.reviewers).toEqual([ + 'reviewer1', + ]) + expect(requestReviewersSpy.mock.calls[0][0]?.team_reviewers).toEqual([]) + }) }) diff --git a/test/util.test.ts b/test/util.test.ts index 20f09a7..3d646c5 100644 --- a/test/util.test.ts +++ b/test/util.test.ts @@ -1,4 +1,9 @@ -import { chooseUsers, chooseUsersFromGroups, includesSkipKeywords } from '../src/util' +import { + chooseUsers, + chooseUsersFromGroups, + chooseReviewers, + includesSkipKeywords, +} from '../src/util' describe('chooseUsers', () => { test('returns the reviewer list without the PR creator', () => { @@ -98,13 +103,8 @@ describe('chooseUsersFromGroups', () => { // GIVEN const owner = 'owner' const reviewers = { - groupA: [ - 'owner', - 'reviewer1' - ], - groupB: [ - 'reviewer2' - ] + groupA: ['owner', 'reviewer1'], + groupB: ['reviewer2'], } const numberOfReviewers = 1 @@ -119,12 +119,8 @@ describe('chooseUsersFromGroups', () => { // GIVEN const owner = 'owner' const reviewers = { - groupA: [ - 'owner' - ], - groupB: [ - 'reviewer2' - ] + groupA: ['owner'], + groupB: ['reviewer2'], } const numberOfReviewers = 1 @@ -140,20 +136,10 @@ describe('chooseUsersFromGroups', () => { // GIVEN const owner = 'owner' const reviewers = { - groupA: [ - 'owner', - 'groupA-1', - 'groupA-2' - ], - groupB: [ - 'groupB-1', - 'groupB-2' - ], + groupA: ['owner', 'groupA-1', 'groupA-2'], + groupB: ['groupB-1', 'groupB-2'], groupC: [], - groupD: [ - 'groupD-1', - 'groupD-2' - ] + groupD: ['groupD-1', 'groupD-2'], } const numberOfReviewers = 1 @@ -171,11 +157,8 @@ describe('chooseUsersFromGroups', () => { // GIVEN const owner = 'owner' const reviewers = { - groupA: [ - 'owner', - 'reviewer1' - ], - groupB: [] + groupA: ['owner', 'reviewer1'], + groupB: [], } const numberOfReviewers = 1 @@ -192,10 +175,7 @@ describe('chooseUsersFromGroups', () => { const owner = 'owner' const reviewers = { groupA: [], - groupB: [ - 'owner', - 'reviewer1' - ] + groupB: ['owner', 'reviewer1'], } const numberOfReviewers = 2 @@ -212,7 +192,7 @@ describe('chooseUsersFromGroups', () => { const owner = 'owner' const reviewers = { groupA: [], - groupB: [] + groupB: [], } const numberOfReviewers = 2 @@ -224,3 +204,86 @@ describe('chooseUsersFromGroups', () => { expect(list).toEqual([]) }) }) + +describe('chooseReviewers', () => { + test('returns separate reviewers and team_reviewers arrays', () => { + // GIVEN + const owner = 'pr-creator' + const config = { + addReviewers: true, + addAssignees: false, + reviewers: ['reviewer1', 'reviewer2', 'pr-creator'], + teamReviewers: ['team1', 'org/team2'], + assignees: [], + numberOfAssignees: 0, + numberOfReviewers: 0, + skipKeywords: [], + useReviewGroups: false, + useAssigneeGroups: false, + reviewGroups: {}, + assigneeGroups: {}, + skipUsers: [], + } + + // WHEN + const result = chooseReviewers(owner, config) + + // THEN + expect(result.reviewers).toEqual(['reviewer1', 'reviewer2']) + expect(result.team_reviewers).toEqual(['team1', 'org/team2']) + }) + + test('handles empty team_reviewers config', () => { + // GIVEN + const owner = 'pr-creator' + const config = { + addReviewers: true, + addAssignees: false, + reviewers: ['reviewer1'], + teamReviewers: [], + assignees: [], + numberOfAssignees: 0, + numberOfReviewers: 0, + skipKeywords: [], + useReviewGroups: false, + useAssigneeGroups: false, + reviewGroups: {}, + assigneeGroups: {}, + skipUsers: [], + } + + // WHEN + const result = chooseReviewers(owner, config) + + // THEN + expect(result.reviewers).toEqual(['reviewer1']) + expect(result.team_reviewers).toEqual([]) + }) + + test('respects numberOfReviewers for individual reviewers', () => { + // GIVEN + const owner = 'pr-creator' + const config = { + addReviewers: true, + addAssignees: false, + reviewers: ['reviewer1', 'reviewer2', 'reviewer3'], + teamReviewers: ['team1', 'team2'], + assignees: [], + numberOfAssignees: 0, + numberOfReviewers: 1, + skipKeywords: [], + useReviewGroups: false, + useAssigneeGroups: false, + reviewGroups: {}, + assigneeGroups: {}, + skipUsers: [], + } + + // WHEN + const result = chooseReviewers(owner, config) + + // THEN + expect(result.reviewers).toHaveLength(1) + expect(result.team_reviewers).toEqual(['team1', 'team2']) // Team reviewers are not affected by numberOfReviewers + }) +})