diff --git a/dist/index.js b/dist/index.js index a190e25..2015cc7 100644 --- a/dist/index.js +++ b/dist/index.js @@ -17512,39 +17512,72 @@ async function fetch_reviewers() { return [ ...reviewers ]; } -async function assign_reviewers(reviewers) { +async function filter_only_collaborators(reviewers) { const context = get_context(); const octokit = get_octokit(); const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - const request_review_responses = []; - - // Github's requestReviewers API will fail to add all reviewers if any of the aliases are not collaborators. - // Github also does not support a batch call to determine which aliases in the list are not collaborators. - - // We therefore make each call individually so that we add all reviewers that are collaborators, - // and log failure for aliases that no longer have access. + // Create a list of requests for all available aliases and teams to see if they have permission + // to the PR associated with this action + const collaborator_responses = []; teams.forEach((team) => { - request_review_responses.push(octokit.pulls.requestReviewers({ + collaborator_responses.push(octokit.teams.checkPermissionsForRepoInOrg({ + org: context.repo.owner, + team_slug: team, owner: context.repo.owner, repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - team_reviewers: [ team ], + }).then((response) => { + // https://docs.github.com/en/rest/teams/teams?apiVersion=2022-11-28#check-team-permissions-for-a-repository + // Its expected that a team with permission will return 204 + core.info(`Received successful status code ${response?.status ?? 'Unknown'} for team: ${team}`); + return 'team:'.concat(team); }).catch((error) => core.error(`Team: ${team} failed to be added with error: ${error}`))); }); - - individuals.forEach((login) => { - request_review_responses.push(octokit.pulls.requestReviewers({ + individuals.forEach((alias) => { + collaborator_responses.push(octokit.repos.checkCollaborator({ owner: context.repo.owner, repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: [ login ], - }).catch((error) => core.error(`Individual: ${login} failed to be added with error: ${error}`))); + username: alias, + }).then((response) => { + // https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#check-if-a-user-is-a-repository-collaborator + // Its expected that a collaborator with permission will return 204 + core.info(`Received successful status code ${response?.status ?? 'Unknown'} for alias: ${alias}`); + return alias; + }).catch((error) => core.error(`Individual: ${alias} failed to be added with error: ${error}`))); }); - return Promise.allSettled(request_review_responses); + // Store the aliases and teams of all successful responses + const collaborators = []; + await Promise.allSettled(collaborator_responses).then((results) => { + results.forEach((result) => { + if (result?.value) { + collaborators.push(result?.value); + } + }); + }); + + // Only include aliases and teams that exist as collaborators + const filtered_reviewers = reviewers.filter((reviewer) => collaborators.includes(reviewer)); + core.info(`Filtered list of only collaborators ${filtered_reviewers.join(', ')}`); + return filtered_reviewers; +} + +async function assign_reviewers(reviewers) { + const context = get_context(); + const octokit = get_octokit(); + + const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); + const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + + return octokit.pulls.requestReviewers({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + reviewers: individuals, + team_reviewers: teams, + }); } /* Private */ @@ -17595,6 +17628,7 @@ module.exports = { fetch_config, fetch_changed_files, fetch_reviewers, + filter_only_collaborators, assign_reviewers, clear_cache, }; @@ -17683,6 +17717,9 @@ async function run() { core.info(`Possible Reviewers ${reviewers.join(', ')}, prepare filtering out already requested reviewers or approved reviewers`); reviewers = reviewers.filter((reviewer) => !requested_approved_reviewers.includes(reviewer)); + core.info(`Possible New Reviewers ${reviewers.join(', ')}, prepare to filter to only collaborators`); + reviewers = await github.filter_only_collaborators(reviewers); + core.info('Randomly picking reviewers if the number of reviewers is set'); reviewers = randomly_pick_reviewers({ reviewers, config }); diff --git a/src/github.js b/src/github.js index 3f2b051..6d31037 100644 --- a/src/github.js +++ b/src/github.js @@ -163,39 +163,72 @@ async function fetch_reviewers() { return [ ...reviewers ]; } -async function assign_reviewers(reviewers) { +async function filter_only_collaborators(reviewers) { const context = get_context(); const octokit = get_octokit(); const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); - const request_review_responses = []; - - // Github's requestReviewers API will fail to add all reviewers if any of the aliases are not collaborators. - // Github also does not support a batch call to determine which aliases in the list are not collaborators. - - // We therefore make each call individually so that we add all reviewers that are collaborators, - // and log failure for aliases that no longer have access. + // Create a list of requests for all available aliases and teams to see if they have permission + // to the PR associated with this action + const collaborator_responses = []; teams.forEach((team) => { - request_review_responses.push(octokit.pulls.requestReviewers({ + collaborator_responses.push(octokit.teams.checkPermissionsForRepoInOrg({ + org: context.repo.owner, + team_slug: team, owner: context.repo.owner, repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - team_reviewers: [ team ], + }).then((response) => { + // https://docs.github.com/en/rest/teams/teams?apiVersion=2022-11-28#check-team-permissions-for-a-repository + // Its expected that a team with permission will return 204 + core.info(`Received successful status code ${response?.status ?? 'Unknown'} for team: ${team}`); + return 'team:'.concat(team); }).catch((error) => core.error(`Team: ${team} failed to be added with error: ${error}`))); }); - - individuals.forEach((login) => { - request_review_responses.push(octokit.pulls.requestReviewers({ + individuals.forEach((alias) => { + collaborator_responses.push(octokit.repos.checkCollaborator({ owner: context.repo.owner, repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - reviewers: [ login ], - }).catch((error) => core.error(`Individual: ${login} failed to be added with error: ${error}`))); + username: alias, + }).then((response) => { + // https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#check-if-a-user-is-a-repository-collaborator + // Its expected that a collaborator with permission will return 204 + core.info(`Received successful status code ${response?.status ?? 'Unknown'} for alias: ${alias}`); + return alias; + }).catch((error) => core.error(`Individual: ${alias} failed to be added with error: ${error}`))); + }); + + // Store the aliases and teams of all successful responses + const collaborators = []; + await Promise.allSettled(collaborator_responses).then((results) => { + results.forEach((result) => { + if (result?.value) { + collaborators.push(result?.value); + } + }); }); - return Promise.allSettled(request_review_responses); + // Only include aliases and teams that exist as collaborators + const filtered_reviewers = reviewers.filter((reviewer) => collaborators.includes(reviewer)); + core.info(`Filtered list of only collaborators ${filtered_reviewers.join(', ')}`); + return filtered_reviewers; +} + +async function assign_reviewers(reviewers) { + const context = get_context(); + const octokit = get_octokit(); + + const [ teams_with_prefix, individuals ] = partition(reviewers, (reviewer) => reviewer.startsWith('team:')); + const teams = teams_with_prefix.map((team_with_prefix) => team_with_prefix.replace('team:', '')); + + return octokit.pulls.requestReviewers({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + reviewers: individuals, + team_reviewers: teams, + }); } /* Private */ @@ -246,6 +279,7 @@ module.exports = { fetch_config, fetch_changed_files, fetch_reviewers, + filter_only_collaborators, assign_reviewers, clear_cache, }; diff --git a/src/index.js b/src/index.js index ece7a0e..2e3c35d 100644 --- a/src/index.js +++ b/src/index.js @@ -75,6 +75,9 @@ async function run() { core.info(`Possible Reviewers ${reviewers.join(', ')}, prepare filtering out already requested reviewers or approved reviewers`); reviewers = reviewers.filter((reviewer) => !requested_approved_reviewers.includes(reviewer)); + core.info(`Possible New Reviewers ${reviewers.join(', ')}, prepare to filter to only collaborators`); + reviewers = await github.filter_only_collaborators(reviewers); + core.info('Randomly picking reviewers if the number of reviewers is set'); reviewers = randomly_pick_reviewers({ reviewers, config }); diff --git a/test/github.test.js b/test/github.test.js index 85a3a73..223bfca 100644 --- a/test/github.test.js +++ b/test/github.test.js @@ -278,11 +278,15 @@ describe('github', function() { }); }); - describe('assign_reviewers()', function() { - const stub = sinon.stub(); + describe('filter_only_collaborators()', function() { + const teamStub = sinon.stub(); + const aliasStub = sinon.stub(); const octokit = { - pulls: { - requestReviewers: stub, + repos: { + checkCollaborator: aliasStub, + }, + teams: { + checkPermissionsForRepoInOrg: teamStub, }, }; @@ -291,144 +295,145 @@ describe('github', function() { restoreModule = rewired_github.__set__('octokit_cache', octokit); }); afterEach(function() { - stub.reset(); + teamStub.reset(); + aliasStub.reset(); restoreModule(); }); - it('assigns reviewers - mixed success', async function() { - stub.onFirstCall().resolves({}); - stub.onSecondCall().resolves({}); - stub.onThirdCall().resolves({}); - - const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; - await rewired_github.assign_reviewers(reviewers); + it('remove non collaborators - individual', async function() { + const allCandidates = [ 'bowser', 'peach', 'luigi', 'mario' ]; - expect(stub.calledThrice).to.be.true; - expect(stub.firstCall.args[0]).to.deep.equal({ + aliasStub.withArgs({ owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - team_reviewers: [ - 'koopa-troop', - ], - }); - - expect(stub.secondCall.args[0]).to.deep.equal({ + username: 'bowser', + }).rejects(); + aliasStub.withArgs({ owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - reviewers: [ - 'mario', - ], - }); - - expect(stub.thirdCall.args[0]).to.deep.equal({ + username: 'peach', + }).resolves({ status: '204' }); + aliasStub.withArgs({ owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - reviewers: [ - 'princess-peach', - ], - }); + username: 'luigi', + }).rejects(); + aliasStub.withArgs({ + owner: 'necojackarc', + repo: 'auto-request-review', + username: 'mario', + }).resolves({ status: '204' }); + + const actual = await rewired_github.filter_only_collaborators(allCandidates); + expect(actual).to.deep.equal([ 'peach', 'mario' ]); + expect(teamStub.called).to.be.false; + expect(aliasStub.callCount).to.be.equal(4); }); - it('assigns reviewers - continues after failure individual', async function() { - stub.onFirstCall().resolves({}); - let isResolved = false; - let isRejected = false; - const error = new Error('Alias is not a collaborator'); - const failedNetworkCall = Promise.reject(error).then( - function(value) { - isResolved = true; return value; - }, - function(error) { - isRejected = true; throw error; - } - ); - stub.onSecondCall().returns(failedNetworkCall); - stub.onThirdCall().resolves({}); - - const reviewers = [ 'team:super_marios', 'princess-peach', 'luigi' ]; - await rewired_github.assign_reviewers(reviewers); + it('remove non collaborators - teams', async function() { + const allCandidates = [ 'team:koopa-troop', 'team:toads', 'team:peach-alliance', 'team:bowser-and-co' ]; - expect(stub.calledThrice).to.be.true; - expect(stub.firstCall.args[0]).to.deep.equal({ + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'koopa-troop', owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - team_reviewers: [ - 'super_marios', - ], - }); - - expect(stub.secondCall.args[0]).to.deep.equal({ + }).resolves({ status: '204' }); + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'toads', owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - reviewers: [ - 'princess-peach', - ], - }); - expect(isRejected).to.be.true; - expect(isResolved).to.be.false; - - expect(stub.thirdCall.args[0]).to.deep.equal({ + }).rejects(); + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'peach-alliance', owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - reviewers: [ - 'luigi', - ], - }); - }); + }).rejects(); + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'bowser-and-co', + owner: 'necojackarc', + repo: 'auto-request-review', + }).resolves({ status: '204' }); - it('assigns reviewers - continues after failure team', async function() { - let isResolved = false; - let isRejected = false; - const error = new Error('Alias is not a collaborator'); - const failedNetworkCall = Promise.reject(error).then( - function(value) { - isResolved = true; return value; - }, - function(error) { - isRejected = true; throw error; - } - ); - stub.onFirstCall().returns(failedNetworkCall); - stub.onSecondCall().resolves({}); - stub.onThirdCall().resolves({}); - - const reviewers = [ 'team:toads', 'team:koopa-troop', 'mario' ]; - await rewired_github.assign_reviewers(reviewers); + const actual = await rewired_github.filter_only_collaborators(allCandidates); + expect(actual).to.deep.equal([ 'team:koopa-troop', 'team:bowser-and-co' ]); + expect(teamStub.callCount).to.be.equal(4); + expect(aliasStub.called).to.be.false; + }); - expect(stub.calledThrice).to.be.true; + it('remove non collaborators - mixed', async function() { + const allCandidates = [ 'peach', 'team:peach-alliance', 'luigi', 'mario', 'team:bowser-and-co' ]; - expect(stub.firstCall.args[0]).to.deep.equal({ + aliasStub.withArgs({ owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - team_reviewers: [ - 'toads', - ], - }); - expect(isRejected).to.be.true; - expect(isResolved).to.be.false; + username: 'peach', + }).resolves({ status: '204' }); + aliasStub.withArgs({ + owner: 'necojackarc', + repo: 'auto-request-review', + username: 'luigi', + }).rejects(); + aliasStub.withArgs({ + owner: 'necojackarc', + repo: 'auto-request-review', + username: 'mario', + }).rejects(); - expect(stub.secondCall.args[0]).to.deep.equal({ + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'peach-alliance', owner: 'necojackarc', - pull_number: 18, repo: 'auto-request-review', - team_reviewers: [ - 'koopa-troop', - ], - }); + }).resolves({ status: '204' }); + teamStub.withArgs({ + org: 'necojackarc', + team_slug: 'bowser-and-co', + owner: 'necojackarc', + repo: 'auto-request-review', + }).rejects(); - expect(stub.thirdCall.args[0]).to.deep.equal({ + const actual = await rewired_github.filter_only_collaborators(allCandidates); + expect(actual).to.deep.equal([ 'peach', 'team:peach-alliance' ]); + expect(teamStub.callCount).to.be.equal(2); + expect(aliasStub.callCount).to.be.equal(3); + }); + }); + + describe('assign_reviewers()', function() { + const spy = sinon.spy(); + const octokit = { + pulls: { + requestReviewers: spy, + }, + }; + + let restoreModule; + beforeEach(function() { + restoreModule = rewired_github.__set__('octokit_cache', octokit); + }); + afterEach(function() { + restoreModule(); + }); + + it('assigns reviewers', async function() { + const reviewers = [ 'mario', 'princess-peach', 'team:koopa-troop' ]; + await rewired_github.assign_reviewers(reviewers); + + expect(spy.calledOnce).to.be.true; + expect(spy.lastCall.args[0]).to.deep.equal({ owner: 'necojackarc', pull_number: 18, repo: 'auto-request-review', reviewers: [ 'mario', + 'princess-peach', + ], + team_reviewers: [ + 'koopa-troop', ], }); }); diff --git a/test/index.test.js b/test/index.test.js index 7d1c377..6ad387c 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -15,6 +15,7 @@ describe('index', function() { sinon.stub(github, 'fetch_config'); sinon.stub(github, 'fetch_changed_files'); sinon.stub(github, 'fetch_reviewers'); + sinon.stub(github, 'filter_only_collaborators'); sinon.stub(github, 'assign_reviewers'); }); @@ -23,6 +24,7 @@ describe('index', function() { github.fetch_config.restore(); github.fetch_changed_files.restore(); github.fetch_reviewers.restore(); + github.filter_only_collaborators.restore(); github.assign_reviewers.restore(); }); @@ -54,6 +56,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -88,6 +92,8 @@ describe('index', function() { const current_reviewers = [ 'princess-peach' ]; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -122,13 +128,15 @@ describe('index', function() { const current_reviewers = [ 'team:bowser-and-co' ]; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; expect(github.assign_reviewers.lastCall.args[0]).to.have.members([ 'mario', 'team:peach-alliance', 'wario', 'waluigi' ]); }); - it('skips caling assign if no reviewers', async function() { + it('skips calling assign if no reviewers', async function() { const config = { reviewers: { defaults: [ 'dr-mario' ], @@ -156,11 +164,133 @@ describe('index', function() { const current_reviewers = [ 'princess-peach', 'mario' ]; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.false; }); + it('removes non collaborators - individual', async function() { + const config = { + reviewers: { + defaults: [ 'dr-mario' ], + groups: { + 'mario-brothers': [ 'mario', 'luigi' ], + }, + }, + files: { + '**/*.js': [ 'mario-brothers', 'princess-peach' ], + '**/*.rb': [ 'wario', 'waluigi' ], + }, + }; + github.fetch_config.returns(config); + + const pull_request = { + title: 'Nice Pull Request', + is_draft: false, + author: 'luigi', + }; + github.get_pull_request.returns(pull_request); + + const changed_files = [ 'path/to/file.js' ]; + github.fetch_changed_files.returns(changed_files); + + const current_reviewers = [ ]; + github.fetch_reviewers.returns(current_reviewers); + + const collaborators = [ 'mario' ]; + github.filter_only_collaborators.returns(collaborators); + + await run(); + + expect(github.filter_only_collaborators.calledOnce).to.be.true; + expect(github.filter_only_collaborators.lastCall.args[0]).to.have.members([ 'mario', 'princess-peach' ]); + + expect(github.assign_reviewers.calledOnce).to.be.true; + expect(github.assign_reviewers.lastCall.args[0]).to.have.members([ 'mario' ]); + }); + + it('removes non collaborators - team', async function() { + const config = { + reviewers: { + defaults: [ 'dr-mario' ], + groups: { + 'mario-brothers': [ 'mario', 'luigi' ], + }, + }, + files: { + '**/*.js': [ 'mario-brothers', 'team:peach-alliance' ], + '**/*.rb': [ 'wario', 'waluigi', 'team:bowser-and-co' ], + }, + }; + github.fetch_config.returns(config); + + const pull_request = { + title: 'Nice Pull Request', + is_draft: false, + author: 'luigi', + }; + github.get_pull_request.returns(pull_request); + + const changed_files = [ 'path/to/file.js', 'path/to/file.rb' ]; + github.fetch_changed_files.returns(changed_files); + + const current_reviewers = [ ]; + github.fetch_reviewers.returns(current_reviewers); + + const collaborators = [ 'team:peach-alliance' ]; + github.filter_only_collaborators.returns(collaborators); + + await run(); + + expect(github.filter_only_collaborators.calledOnce).to.be.true; + expect(github.filter_only_collaborators.lastCall.args[0]).to.have.members([ 'mario', 'team:peach-alliance', 'wario', 'waluigi', 'team:bowser-and-co' ]); + + expect(github.assign_reviewers.calledOnce).to.be.true; + expect(github.assign_reviewers.lastCall.args[0]).to.have.members([ 'team:peach-alliance' ]); + }); + + it('removes non collaborators + previous review mix', async function() { + const config = { + reviewers: { + defaults: [ 'dr-mario' ], + groups: { + 'mario-brothers': [ 'mario', 'luigi' ], + }, + }, + files: { + '**/*.js': [ 'mario-brothers', 'team:peach-alliance' ], + '**/*.rb': [ 'wario', 'waluigi', 'team:bowser-and-co' ], + }, + }; + github.fetch_config.returns(config); + + const pull_request = { + title: 'Nice Pull Request', + is_draft: false, + author: 'luigi', + }; + github.get_pull_request.returns(pull_request); + + const changed_files = [ 'path/to/file.js', 'path/to/file.rb' ]; + github.fetch_changed_files.returns(changed_files); + + const current_reviewers = [ 'waluigi', 'team:peach-alliance' ]; + github.fetch_reviewers.returns(current_reviewers); + + const collaborators = [ 'mario', 'team:bowser-and-co' ]; + github.filter_only_collaborators.returns(collaborators); + + await run(); + + expect(github.filter_only_collaborators.calledOnce).to.be.true; + expect(github.filter_only_collaborators.lastCall.args[0]).to.have.members([ 'mario', 'wario', 'team:bowser-and-co' ]); + + expect(github.assign_reviewers.calledOnce).to.be.true; + expect(github.assign_reviewers.lastCall.args[0]).to.have.members([ 'mario', 'team:bowser-and-co' ]); + }); + it('requests review based on groups that author belongs to', async function() { const config = { reviewers: { @@ -189,6 +319,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -248,6 +380,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.false; @@ -280,6 +414,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.false; @@ -313,6 +449,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -347,6 +485,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -381,6 +521,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true; @@ -413,6 +555,8 @@ describe('index', function() { const current_reviewers = []; github.fetch_reviewers.returns(current_reviewers); + github.filter_only_collaborators.returnsArg(0); + await run(); expect(github.assign_reviewers.calledOnce).to.be.true;