Skip to content

Commit

Permalink
Add support for required reviews (#160)
Browse files Browse the repository at this point in the history
  • Loading branch information
@stephtr
stephtr authored Sep 3, 2021
1 parent e88d173 commit e7220c3
Show file tree
Hide file tree
Showing 9 changed files with 200 additions and 49 deletions.
3 changes: 3 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ The following merge options are supported:
The default is `5000` (5 seconds) and setting it to `0` disables sleeping
between retries.

- `MERGE_REQUIRED_APPROVALS`: Count of required approvals. The default is `0`.

- `MERGE_DELETE_BRANCH`: Automatic deletion of branches does not work for all
repositories. Set this option to `true` to automatically delete branches
after they have been merged. The default value is `false`.
Expand Down Expand Up @@ -209,6 +211,7 @@ You can configure the environment variables in the workflow file like this:
MERGE_FORKS: "false"
MERGE_RETRIES: "6"
MERGE_RETRY_SLEEP: "10000"
MERGE_REQUIRED_APPROVALS: "0"
UPDATE_LABELS: ""
UPDATE_METHOD: "rebase"
PULL_REQUEST: "1234"
Expand Down
73 changes: 59 additions & 14 deletions dist/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ async function handlePullRequestUpdate(context, eventName, event) {
event.pull_request.base.repo,
event.pull_request
);

const approvalCount = await fetchApprovalReviewCount(context, pullRequest);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
}

async function handleArbitraryPullRequestUpdate(context, eventData) {
Expand All @@ -125,9 +125,9 @@ async function handleArbitraryPullRequestUpdate(context, eventData) {
pull_number: pullRequestNumber
});
logger.trace("Full PR:", pullRequest);

const approvalCount = await fetchApprovalReviewCount(context, pullRequest);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
} catch (e) {
logger.error(
`Error fetching pull request: ${repoOwner}/${repoName}/${pullRequestNumber}`
Expand Down Expand Up @@ -160,9 +160,9 @@ async function handleCheckOrWorkflowUpdate(context, eventName, event) {
const { octokit } = context;
const { data: pullRequest } = await octokit.request(eventPullRequest.url);
logger.trace("PR:", pullRequest);

const approvalCount = await fetchApprovalReviewCount(context, pullRequest);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
} else {
const branchName = payload.head_branch;
if (branchName != null) {
Expand All @@ -186,8 +186,12 @@ async function handlePullRequestReviewUpdate(context, eventName, event) {
event.pull_request.base.repo,
event.pull_request
);
const approvalCount = await fetchApprovalReviewCount(
context,
pullRequest
);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
} else {
logger.info("Review state is not approved:", review.state);
logger.info("Action ignored:", eventName, action);
Expand Down Expand Up @@ -232,8 +236,12 @@ async function checkPullRequestsForBranches(context, event, branchName) {
let updated = 0;
for (const pullRequest of pullRequests) {
try {
const approvalCount = await fetchApprovalReviewCount(
context,
pullRequest
);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
++updated;
} catch (e) {
logger.error(e);
Expand Down Expand Up @@ -265,8 +273,12 @@ async function checkPullRequestsForHeadSha(context, repo, head_sha) {
}
foundPR = true;
try {
const approvalCount = await fetchApprovalReviewCount(
context,
pullRequest
);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
++updated;
} catch (e) {
logger.error(e);
Expand Down Expand Up @@ -370,8 +382,12 @@ async function handleScheduleTriggerOrRepositoryDispatch(context) {
let updated = 0;
for (const pullRequest of pullRequests) {
try {
const approvalCount = await fetchApprovalReviewCount(
context,
pullRequest
);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
++updated;
} catch (e) {
logger.error(e);
Expand All @@ -391,8 +407,12 @@ async function handleIssueComment(context, eventName, event) {
logger.info("Comment not on a PR, skipping");
} else {
const pullRequest = await fetchPullRequest(context, repository, issue);
const approvalCount = await fetchApprovalReviewCount(
context,
pullRequest
);
await update(context, pullRequest);
await merge(context, pullRequest);
await merge(context, pullRequest, approvalCount);
}
} else {
logger.info("Action ignored:", eventName, action);
Expand All @@ -414,6 +434,23 @@ async function fetchPullRequest(context, repository, issue) {
return pullRequest;
}

async function fetchApprovalReviewCount(context, pullRequest) {
const { octokit } = context;
const { number } = pullRequest;

logger.debug("Getting reviews for", number, "...");
let { data: reviews } = await octokit.pulls.listReviews({
owner: pullRequest.base.repo.owner.login,
repo: pullRequest.base.repo.name,
pull_number: number
});

reviews = reviews.filter(review => review.state === "APPROVED");

logger.trace("Approval reviews:", reviews);
return reviews.length;
}

module.exports = { executeLocally, executeGitHubAction };


Expand Down Expand Up @@ -579,6 +616,7 @@ function createConfig(env = {}) {
const mergeFilterAuthor = env.MERGE_FILTER_AUTHOR || "";
const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6);
const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 5000);
const mergeRequiredApprovals = parsePositiveInt("MERGE_REQUIRED_APPROVALS", 0);
const mergeDeleteBranch = env.MERGE_DELETE_BRANCH === "true";
const mergeMethodLabels = parseMergeMethodLabels(env.MERGE_METHOD_LABELS);
const mergeMethodLabelRequired = env.MERGE_METHOD_LABEL_REQUIRED === "true";
Expand All @@ -602,6 +640,7 @@ function createConfig(env = {}) {
mergeFilterAuthor,
mergeRetries,
mergeRetrySleep,
mergeRequiredApprovals,
mergeDeleteBranch,
updateLabels,
updateMethod,
Expand Down Expand Up @@ -886,8 +925,8 @@ const NOT_READY = ["dirty", "draft"];

const PR_PROPERTY = new RegExp("{pullRequest.([^}]+)}", "g");

async function merge(context, pullRequest) {
if (skipPullRequest(context, pullRequest)) {
async function merge(context, pullRequest, approvalCount) {
if (skipPullRequest(context, pullRequest, approvalCount)) {
return false;
}

Expand Down Expand Up @@ -1034,11 +1073,12 @@ async function deleteBranch(octokit, pullRequest) {
}
}

function skipPullRequest(context, pullRequest) {
function skipPullRequest(context, pullRequest, approvalCount) {
const {
config: {
mergeForks,
mergeLabels,
mergeRequiredApprovals,
mergeMethodLabelRequired,
mergeMethodLabels
}
Expand Down Expand Up @@ -1079,6 +1119,11 @@ function skipPullRequest(context, pullRequest) {
}
}

if (approvalCount < mergeRequiredApprovals) {
logger.info(`Skipping PR merge, missing ${mergeRequiredApprovals - approvalCount} approvals`);
skip = true;
}

const numberMethodLabelsFound = mergeMethodLabels
.map(lm => labels.includes(lm.label))
.filter(x => x).length;
Expand Down
1 change: 1 addition & 0 deletions it/it.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ async function main() {
const config = createConfig({
UPDATE_LABELS: "it-update",
MERGE_LABELS: "it-merge",
MERGE_REQUIRED_APPROVALS: "0",
MERGE_REMOVE_LABELS: "it-merge",
MERGE_RETRIES: "3",
MERGE_RETRY_SLEEP: "2000"
Expand Down
62 changes: 43 additions & 19 deletions lib/api.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ async function executeLocally(context, url) {
}
}

async function updateAndMerge(context, pullRequest) {
const approvalCount = await fetchApprovalReviewCount(context, pullRequest);

await update(context, pullRequest);
await merge(context, pullRequest, approvalCount);
}

async function executeGitHubAction(context, eventName, eventData) {
logger.info("Event name:", eventName);
logger.trace("Event data:", eventData);
Expand Down Expand Up @@ -96,9 +103,7 @@ async function handlePullRequestUpdate(context, eventName, event) {
event.pull_request.base.repo,
event.pull_request
);

await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
}

async function handleArbitraryPullRequestUpdate(context, eventData) {
Expand All @@ -118,9 +123,7 @@ async function handleArbitraryPullRequestUpdate(context, eventData) {
pull_number: pullRequestNumber
});
logger.trace("Full PR:", pullRequest);

await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
} catch (e) {
logger.error(
`Error fetching pull request: ${repoOwner}/${repoName}/${pullRequestNumber}`
Expand Down Expand Up @@ -153,9 +156,7 @@ async function handleCheckOrWorkflowUpdate(context, eventName, event) {
const { octokit } = context;
const { data: pullRequest } = await octokit.request(eventPullRequest.url);
logger.trace("PR:", pullRequest);

await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
} else {
const branchName = payload.head_branch;
if (branchName != null) {
Expand All @@ -179,8 +180,7 @@ async function handlePullRequestReviewUpdate(context, eventName, event) {
event.pull_request.base.repo,
event.pull_request
);
await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
} else {
logger.info("Review state is not approved:", review.state);
logger.info("Action ignored:", eventName, action);
Expand Down Expand Up @@ -225,8 +225,7 @@ async function checkPullRequestsForBranches(context, event, branchName) {
let updated = 0;
for (const pullRequest of pullRequests) {
try {
await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
++updated;
} catch (e) {
logger.error(e);
Expand Down Expand Up @@ -258,8 +257,7 @@ async function checkPullRequestsForHeadSha(context, repo, head_sha) {
}
foundPR = true;
try {
await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
++updated;
} catch (e) {
logger.error(e);
Expand Down Expand Up @@ -363,8 +361,7 @@ async function handleScheduleTriggerOrRepositoryDispatch(context) {
let updated = 0;
for (const pullRequest of pullRequests) {
try {
await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
++updated;
} catch (e) {
logger.error(e);
Expand All @@ -384,8 +381,7 @@ async function handleIssueComment(context, eventName, event) {
logger.info("Comment not on a PR, skipping");
} else {
const pullRequest = await fetchPullRequest(context, repository, issue);
await update(context, pullRequest);
await merge(context, pullRequest);
await updateAndMerge(context, pullRequest);
}
} else {
logger.info("Action ignored:", eventName, action);
Expand All @@ -407,4 +403,32 @@ async function fetchPullRequest(context, repository, issue) {
return pullRequest;
}

async function fetchApprovalReviewCount(context, pullRequest) {
const {
octokit,
config: { mergeRequiredApprovals }
} = context;
const { number } = pullRequest;

if (mergeRequiredApprovals === 0) {
// If we don't care about review approvals, let's short circuit.
return 0;
}

logger.debug("Getting reviews for", number, "...");
let { data: reviews } = await octokit.pulls.listReviews({
owner: pullRequest.base.repo.owner.login,
repo: pullRequest.base.repo.name,
pull_number: number
});

const approvingReviewers = reviews
.filter(review => review.state === "APPROVED")
.map(review => review.user.login);
const uniqueApprovingReviewers = [...new Set(approvingReviewers)];

logger.trace("Approval reviewers:", uniqueApprovingReviewers);
return uniqueApprovingReviewers.length;
}

module.exports = { executeLocally, executeGitHubAction };
5 changes: 5 additions & 0 deletions lib/common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ function createConfig(env = {}) {
const mergeFilterAuthor = env.MERGE_FILTER_AUTHOR || "";
const mergeRetries = parsePositiveInt("MERGE_RETRIES", 6);
const mergeRetrySleep = parsePositiveInt("MERGE_RETRY_SLEEP", 5000);
const mergeRequiredApprovals = parsePositiveInt(
"MERGE_REQUIRED_APPROVALS",
0
);
const mergeDeleteBranch = env.MERGE_DELETE_BRANCH === "true";
const mergeMethodLabels = parseMergeMethodLabels(env.MERGE_METHOD_LABELS);
const mergeMethodLabelRequired = env.MERGE_METHOD_LABEL_REQUIRED === "true";
Expand All @@ -178,6 +182,7 @@ function createConfig(env = {}) {
mergeFilterAuthor,
mergeRetries,
mergeRetrySleep,
mergeRequiredApprovals,
mergeDeleteBranch,
updateLabels,
updateMethod,
Expand Down
16 changes: 13 additions & 3 deletions lib/merge.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const NOT_READY = ["dirty", "draft"];

const PR_PROPERTY = new RegExp("{pullRequest.([^}]+)}", "g");

async function merge(context, pullRequest) {
if (skipPullRequest(context, pullRequest)) {
async function merge(context, pullRequest, approvalCount) {
if (skipPullRequest(context, pullRequest, approvalCount)) {
return false;
}

Expand Down Expand Up @@ -155,11 +155,12 @@ async function deleteBranch(octokit, pullRequest) {
}
}

function skipPullRequest(context, pullRequest) {
function skipPullRequest(context, pullRequest, approvalCount) {
const {
config: {
mergeForks,
mergeLabels,
mergeRequiredApprovals,
mergeMethodLabelRequired,
mergeMethodLabels
}
Expand Down Expand Up @@ -200,6 +201,15 @@ function skipPullRequest(context, pullRequest) {
}
}

if (approvalCount < mergeRequiredApprovals) {
logger.info(
`Skipping PR merge, missing ${
mergeRequiredApprovals - approvalCount
} approvals`
);
skip = true;
}

const numberMethodLabelsFound = mergeMethodLabels
.map(lm => labels.includes(lm.label))
.filter(x => x).length;
Expand Down
Loading

0 comments on commit e7220c3

Please sign in to comment.