From 64a3c1b3bddc79e293e0bf878f48b4afd48f18df Mon Sep 17 00:00:00 2001 From: Jonathan Perret Date: Thu, 12 May 2022 17:29:54 +0200 Subject: [PATCH 1/2] Retrieve all PR reviews when checking approvals The results from octokit.pulls.listReviews are paginated; if there were more than 30 "reviews" (note that this includes standalone comments on the PR) before the requested number of approvals, the PR would be considered unapproved and wouldn't get merged. By using the `octokit.paginate` helper we make sure to retrieve all reviews. --- lib/api.js | 2 +- test/api.test.js | 25 +++++++++++-------------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/lib/api.js b/lib/api.js index 7f8e7654..fe7a39cf 100644 --- a/lib/api.js +++ b/lib/api.js @@ -461,7 +461,7 @@ async function fetchApprovalReviewCount(context, pullRequest) { } logger.debug("Getting reviews for", number, "..."); - let { data: reviews } = await octokit.pulls.listReviews({ + const reviews = await octokit.paginate(octokit.pulls.listReviews, { owner: pullRequest.base.repo.owner.login, repo: pullRequest.base.repo.name, pull_number: number diff --git a/test/api.test.js b/test/api.test.js index 54fa6a2f..2d183a63 100644 --- a/test/api.test.js +++ b/test/api.test.js @@ -48,8 +48,9 @@ test("only merge PRs with required approvals", async () => { pulls: { list: jest.fn(() => ({ data: [pr] })), merge: jest.fn(() => (merged = true)), - listReviews: jest.fn(() => ({ data: [] })) - } + listReviews: Symbol("listReviews") + }, + paginate: jest.fn(() => []) }; const event = { @@ -63,24 +64,20 @@ test("only merge PRs with required approvals", async () => { expect(merged).toEqual(false); // if there's no approval, it should fail merged = false; - octokit.pulls.listReviews.mockReturnValueOnce({ - data: [ - { state: "APPROVED", user: { login: "approval_user" } }, - { state: "APPROVED", user: { login: "approval_user2" } } - ] - }); + octokit.paginate.mockReturnValueOnce([ + { state: "APPROVED", user: { login: "approval_user" } }, + { state: "APPROVED", user: { login: "approval_user2" } } + ]); // WHEN await api.executeGitHubAction({ config, octokit }, "check_suite", event); expect(merged).toEqual(true); // if there are two approvals, it should succeed merged = false; - octokit.pulls.listReviews.mockReturnValueOnce({ - data: [ - { state: "APPROVED", user: { login: "approval_user" } }, - { state: "APPROVED", user: { login: "approval_user" } } - ] - }); + octokit.paginate.mockReturnValueOnce([ + { state: "APPROVED", user: { login: "approval_user" } }, + { state: "APPROVED", user: { login: "approval_user" } } + ]); // WHEN a user has given await api.executeGitHubAction({ config, octokit }, "check_suite", event); From 20e2de901d0d7c499f6fadc58004d186effa1067 Mon Sep 17 00:00:00 2001 From: Jonathan Perret Date: Thu, 12 May 2022 17:45:10 +0200 Subject: [PATCH 2/2] Regenerate dist/index.js --- dist/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index 8544c192..e129864c 100755 --- a/dist/index.js +++ b/dist/index.js @@ -468,7 +468,7 @@ async function fetchApprovalReviewCount(context, pullRequest) { } logger.debug("Getting reviews for", number, "..."); - let { data: reviews } = await octokit.pulls.listReviews({ + const reviews = await octokit.paginate(octokit.pulls.listReviews, { owner: pullRequest.base.repo.owner.login, repo: pullRequest.base.repo.name, pull_number: number @@ -22095,7 +22095,7 @@ module.exports = JSON.parse('[[[0,44],"disallowed_STD3_valid"],[[45,46],"valid"] /***/ ((module) => { "use strict"; -module.exports = JSON.parse('{"name":"automerge-action","version":"0.15.1","description":"GitHub action to automatically merge pull requests","main":"lib/api.js","author":"Pascal","license":"MIT","private":true,"bin":{"automerge-action":"./bin/automerge.js"},"scripts":{"test":"jest","it":"node it/it.js","lint":"prettier -l lib/** test/** && eslint .","compile":"ncc build bin/automerge.js --license LICENSE -o dist","prepublish":"yarn lint && yarn test && yarn compile"},"dependencies":{"@actions/core":"^1.6.0","@octokit/rest":"^18.12.0","argparse":"^2.0.1","fs-extra":"^10.0.1","object-resolve-path":"^1.1.1","tmp":"^0.2.1"},"devDependencies":{"@vercel/ncc":"^0.33.3","dotenv":"^16.0.0","eslint":"^8.11.0","eslint-plugin-jest":"^26.1.3","jest":"^27.5.1","prettier":"^2.6.0"},"prettier":{"trailingComma":"none","arrowParens":"avoid"}}'); +module.exports = JSON.parse('{"name":"automerge-action","version":"0.15.2","description":"GitHub action to automatically merge pull requests","main":"lib/api.js","author":"Pascal","license":"MIT","private":true,"bin":{"automerge-action":"./bin/automerge.js"},"scripts":{"test":"jest","it":"node it/it.js","lint":"prettier -l lib/** test/** && eslint .","compile":"ncc build bin/automerge.js --license LICENSE -o dist","prepublish":"yarn lint && yarn test && yarn compile"},"dependencies":{"@actions/core":"^1.6.0","@octokit/rest":"^18.12.0","argparse":"^2.0.1","fs-extra":"^10.0.1","object-resolve-path":"^1.1.1","tmp":"^0.2.1"},"devDependencies":{"@vercel/ncc":"^0.33.3","dotenv":"^16.0.0","eslint":"^8.11.0","eslint-plugin-jest":"^26.1.3","jest":"^27.5.1","prettier":"^2.6.0"},"prettier":{"trailingComma":"none","arrowParens":"avoid"}}'); /***/ })