From 8406e4e962efffb58ac802211b85a9dd5d26c00f Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Fri, 25 Feb 2022 10:06:16 -0500 Subject: [PATCH 1/5] Add entry files to process new prs and pr updates --- scripts/ci/pr-bot/.gitignore | 41 ++++++ scripts/ci/pr-bot/Commands.md | 32 +++++ scripts/ci/pr-bot/processNewPrs.ts | 198 ++++++++++++++++++++++++++ scripts/ci/pr-bot/processPrUpdate.ts | 201 +++++++++++++++++++++++++++ 4 files changed, 472 insertions(+) create mode 100644 scripts/ci/pr-bot/.gitignore create mode 100644 scripts/ci/pr-bot/Commands.md create mode 100644 scripts/ci/pr-bot/processNewPrs.ts create mode 100644 scripts/ci/pr-bot/processPrUpdate.ts diff --git a/scripts/ci/pr-bot/.gitignore b/scripts/ci/pr-bot/.gitignore new file mode 100644 index 000000000000..132f5f8e9b26 --- /dev/null +++ b/scripts/ci/pr-bot/.gitignore @@ -0,0 +1,41 @@ +lib-cov +*.seed +*.log +*.csv +*.dat +*.out +*.pid +*.gz +*.swp + +pids +logs +results +tmp + +# Build +public/css/main.css + +# Coverage reports +coverage + +# API keys and secrets +.env + +# Dependency directory +node_modules +bower_components + +# Editors +.idea +*.iml + +# OS metadata +.DS_Store +Thumbs.db + +# Ignore built js files +lib/**/* + +# ignore yarn.lock +yarn.lock \ No newline at end of file diff --git a/scripts/ci/pr-bot/Commands.md b/scripts/ci/pr-bot/Commands.md new file mode 100644 index 000000000000..83d21cb7836d --- /dev/null +++ b/scripts/ci/pr-bot/Commands.md @@ -0,0 +1,32 @@ + + +# PR Bot Commands + +The following commands are available for interaction with the PR Bot +All commands are case insensitive. + +| Command | Description | +| ----------- | ----------- | +| `r: @username` | Ask someone for a review. This will disable the bot for the PR since it assumes you are able to find a reviewer. | +| `assign to next reviewer` | If someone has been assigned to a PR by the bot, this unassigns them and picks a new reviewer. Useful if you don't have the bandwitdth or context to review. | +| `stop reviewer notifications` | This will disable the bot for the PR. | +| `remind me after tests pass` | This will comment after all checks complete and tag the person who commented the command. | +| `waiting on author` | This shifts the attention set to the author. The author can shift the attention set back to the reviewer by commenting anywhere or pushing. | +| `assign set of reviewers` | If the bot has not yet assigned a set of reviewers to the PR, this command will trigger that happening. | \ No newline at end of file diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts new file mode 100644 index 000000000000..43cf64f4b5a8 --- /dev/null +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -0,0 +1,198 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const github = require("./shared/githubUtils"); +const { getChecksStatus } = require("./shared/checks"); +const commentStrings = require("./shared/commentStrings"); +const { ReviewerConfig } = require("./shared/reviewerConfig"); +const { PersistentState } = require("./shared/persistentState"); +const { REPO_OWNER, REPO, PATH_TO_CONFIG_FILE } = require("./shared/constants"); + +// Returns true if the pr needs to be processed or false otherwise. +// We don't need to process PRs that: +// 1) Have WIP in their name +// 2) Are less than 20 minutes old +// 3) Are draft prs +// 4) Are closed +// 5) Have already been processed +// 6) Have notifications stopped +// unless we're supposed to remind the user after tests pass +// (in which case that's all we need to do). +function needsProcessed(pull, prState): boolean { + if (prState.remindAfterTestsPass && prState.remindAfterTestsPass.length > 0) { + return true; + } + if (pull.title.toLowerCase().indexOf("wip") >= 0) { + console.log(`Skipping PR ${pull.number} because it is a WIP`); + return false; + } + let timeCutoff = new Date(new Date().getTime() - 20 * 60000); + if (new Date(pull.created_at) > timeCutoff) { + console.log( + `Skipping PR ${pull.number} because it was created less than 20 minutes ago` + ); + return false; + } + if (pull.state.toLowerCase() != "open") { + console.log(`Skipping PR ${pull.number} because it is closed`); + return false; + } + if (pull.draft) { + console.log(`Skipping PR ${pull.number} because it is a draft`); + return false; + } + if (Object.keys(prState.reviewersAssignedForLabels).length > 0) { + console.log( + `Skipping PR ${pull.number} because it already has been assigned` + ); + return false; + } + if (prState.stopReviewerNotifications) { + console.log( + `Skipping PR ${pull.number} because reviewer notifications have been stopped` + ); + return false; + } + + return true; +} + +// If the checks passed in via checkstate have completed, notifies the users who have configured notifications. +async function remindIfChecksCompleted(pull, stateClient, checkState, prState) { + console.log( + `Notifying reviewers if checks for PR ${pull.number} have completed, then returning` + ); + if (checkState.completed) { + if (checkState.succeeded) { + await github.addPrComment( + pull.number, + commentStrings.allChecksPassed(prState.remindAfterTestsPass) + ); + } else { + await github.addPrComment( + pull.number, + commentStrings.someChecksFailing(prState.remindAfterTestsPass) + ); + } + prState.remindAfterTestsPass = []; + await stateClient.writePrState(pull.number, prState); + } +} + +// If we haven't already +async function notifyChecksFailed(pull, stateClient, prState) { + console.log( + `Checks are failing for PR ${pull.number}. Commenting if we haven't already and skipping.` + ); + if (!prState.commentedAboutFailingChecks) { + await github.addPrComment( + pull.number, + commentStrings.failingChecksCantAssign() + ); + } + prState.commentedAboutFailingChecks = true; + await stateClient.writePrState(pull.number, prState); +} + +// Performs all the business logic of processing a new pull request, including: +// 1) Checking if it needs processed +// 2) Reminding reviewers if checks have completed (if they've subscribed to that) +// 3) Picking/assigning reviewers +// 4) Adding "Next Action: Reviewers label" +// 5) Storing the state of the pull request/reviewers in a dedicated branch. +async function processPull(pull, reviewerConfig, stateClient) { + let prState = await stateClient.getPrState(pull.number); + if (!needsProcessed(pull, prState)) { + return; + } + + let checkState = await getChecksStatus(REPO_OWNER, REPO, pull.head.sha); + + if (prState.remindAfterTestsPass && prState.remindAfterTestsPass.length > 0) { + return await remindIfChecksCompleted( + pull, + stateClient, + checkState, + prState + ); + } + + if (!checkState.succeeded) { + return await notifyChecksFailed(pull, stateClient, prState); + } + prState.commentedAboutFailingChecks = false; + + // Pick reviewers to assign. Store them in reviewerStateToUpdate and update the prState object with those reviewers (and their associated labels) + let reviewerStateToUpdate = {}; + const reviewersForLabels: { [key: string]: string[] } = + reviewerConfig.getReviewersForLabels(pull.labels, [pull.user.login]); + var labels = Object.keys(reviewersForLabels); + if (!labels || labels.length === 0) { + return; + } + for (let i = 0; i < labels.length; i++) { + let label = labels[i]; + let availableReviewers = reviewersForLabels[label]; + let reviewersState = await stateClient.getReviewersForLabelState(label); + let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); + reviewerStateToUpdate[label] = reviewersState; + prState.reviewersAssignedForLabels[label] = chosenReviewer; + } + + console.log(`Assigning reviewers for PR ${pull.number}`); + await github.addPrComment( + pull.number, + commentStrings.assignReviewer(prState.reviewersAssignedForLabels) + ); + + github.nextActionReviewers(pull.number, pull.labels); + prState.nextAction = "Reviewers"; + + await stateClient.writePrState(pull.number, prState); + let labelsToUpdate = Object.keys(reviewerStateToUpdate); + for (let i = 0; i < labelsToUpdate.length; i++) { + let label = labelsToUpdate[i]; + await stateClient.writeReviewersForLabelState( + label, + reviewerStateToUpdate[label] + ); + } +} + +async function processNewPrs() { + const githubClient = github.getGitHubClient(); + let reviewerConfig = new ReviewerConfig(PATH_TO_CONFIG_FILE); + let stateClient = new PersistentState(); + + let openPulls = await githubClient.paginate( + "GET /repos/{owner}/{repo}/pulls", + { + owner: REPO_OWNER, + repo: REPO, + } + ); + + for (let i = 0; i < openPulls.length; i++) { + let pull = openPulls[i]; + await processPull(pull, reviewerConfig, stateClient); + } +} + +processNewPrs(); + +export {}; diff --git a/scripts/ci/pr-bot/processPrUpdate.ts b/scripts/ci/pr-bot/processPrUpdate.ts new file mode 100644 index 000000000000..8e409ffa95ff --- /dev/null +++ b/scripts/ci/pr-bot/processPrUpdate.ts @@ -0,0 +1,201 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const github = require("@actions/github"); +const commentStrings = require("./shared/commentStrings"); +const { processCommand } = require("./shared/userCommand"); +const { addPrComment, nextActionReviewers } = require("./shared/githubUtils"); +const { PersistentState } = require("./shared/persistentState"); +const { ReviewerConfig } = require("./shared/reviewerConfig"); +const { PATH_TO_CONFIG_FILE } = require("./shared/constants"); + +async function areReviewersAssigned( + pullNumber: number, + stateClient: any +): Promise { + const prState = await stateClient.getPrState(pullNumber); + return Object.values(prState.reviewersAssignedForLabels).length > 0; +} + +async function processPrComment( + payload: any, + stateClient: any, + reviewerConfig: any +) { + const commentContents = payload.comment.body; + const commentAuthor = payload.sender.login; + console.log(commentContents); + if ( + await processCommand( + payload, + commentAuthor, + commentContents, + stateClient, + reviewerConfig + ) + ) { + // If we've processed a command, don't worry about trying to change the attention set. + // This is not a meaningful push or comment from the author. + console.log("Processed command"); + return; + } + + // If comment was from the author, we should shift attention back to the reviewers. + console.log( + "No command to be processed, checking if we should shift attention to reviewers" + ); + const pullAuthor = + payload.issue?.user?.login || payload.pull_request?.user?.login; + if (pullAuthor == commentAuthor) { + await setNextActionReviewers(payload, stateClient); + } else { + console.log( + `Comment was from ${commentAuthor}, not author: ${pullAuthor}. No action to take.` + ); + } +} + +// On approval from a reviewer we have assigned, assign committer if one not already assigned +async function processPrReview( + payload: any, + stateClient: any, + reviewerConfig: any +) { + if (payload.review.state != "approved") { + return; + } + + const pullNumber = payload.issue?.number || payload.pull_request?.number; + if (!(await areReviewersAssigned(pullNumber, stateClient))) { + return; + } + + let prState = await stateClient.getPrState(pullNumber); + // TODO(damccorm) - also check if the author is a committer, if they are don't auto-assign a committer + if (await prState.isAnyAssignedReviewerCommitter()) { + return; + } + + let labelOfReviewer = prState.getLabelForReviewer(payload.sender.login); + if (labelOfReviewer) { + let reviewersState = await stateClient.getReviewersForLabelState( + labelOfReviewer + ); + let availableReviewers = + reviewerConfig.getReviewersForLabel(labelOfReviewer); + let chosenCommitter = await reviewersState.assignNextCommitter( + availableReviewers + ); + prState.reviewersAssignedForLabels[labelOfReviewer] = chosenCommitter; + + // Set next action to committer + await addPrComment( + pullNumber, + commentStrings.assignCommitter(chosenCommitter) + ); + const existingLabels = + payload.issue?.labels || payload.pull_request?.labels; + await nextActionReviewers(pullNumber, existingLabels); + prState.nextAction = "Reviewers"; + + // Persist state + await stateClient.writePrState(pullNumber, prState); + await stateClient.writeReviewersForLabelState( + labelOfReviewer, + reviewersState + ); + } +} + +// On pr push or author comment, we should put the attention set back on the reviewers +async function setNextActionReviewers(payload: any, stateClient: any) { + const pullNumber = payload.issue?.number || payload.pull_request?.number; + if (!(await areReviewersAssigned(pullNumber, stateClient))) { + console.log("No reviewers assigned, dont need to manipulate attention set"); + return; + } + const existingLabels = payload.issue?.labels || payload.pull_request?.labels; + await nextActionReviewers(pullNumber, existingLabels); + let prState = await stateClient.getPrState(pullNumber); + prState.nextAction = "Reviewers"; + await stateClient.writePrState(pullNumber, prState); +} + +async function processPrUpdate() { + const reviewerConfig = new ReviewerConfig(PATH_TO_CONFIG_FILE); + const context = github.context; + console.log("Event context:"); + console.log(context); + const payload = context.payload; + + // TODO(damccorm) - remove this when we roll out to more than go + const existingLabels = payload.issue?.labels || payload.pull_request?.labels; + let containsGoLabel = false; + existingLabels.forEach((label) => { + if (label.name.toLowerCase() == "go") { + containsGoLabel = true; + } + }); + if (!containsGoLabel) { + console.log("Does not contain the go label - skipping"); + return; + } + + if (!payload.issue?.pull_request && !payload.pull_request) { + console.log("Issue, not pull request - returning"); + return; + } + const pullNumber = payload.issue?.number || payload.pull_request?.number; + + let stateClient = new PersistentState(); + let prState = await stateClient.getPrState(pullNumber); + if (prState.stopReviewerNotifications) { + console.log("Notifications have been paused for this pull - skipping"); + return; + } + + switch (github.context.eventName) { + case "pull_request_review_comment": + case "issue_comment": + console.log("Processing comment event"); + if (payload.action != "created") { + console.log("Comment wasnt just created, skipping"); + return; + } + await processPrComment(payload, stateClient, reviewerConfig); + break; + case "pull_request_review": + console.log("Processing PR review event"); + await processPrReview(payload, stateClient, reviewerConfig); + break; + case "pull_request_target": + if (payload.action == "synchronize") { + console.log("Processing synchronize action"); + await setNextActionReviewers(payload, stateClient); + } + // TODO(damccorm) - it would be good to eventually handle the following events here, even though they're not part of the normal workflow + // review requested, assigned, label added, label removed + break; + default: + console.log("Not a PR comment, push, or review, doing nothing"); + } +} + +processPrUpdate(); + +export {}; From f262da204a816400e7e668f82c70d8c88401b104 Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Fri, 25 Feb 2022 10:19:31 -0500 Subject: [PATCH 2/5] Typing --- scripts/ci/pr-bot/processNewPrs.ts | 25 ++++++++++++++++++++----- scripts/ci/pr-bot/processPrUpdate.ts | 19 +++++++++++-------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts index 43cf64f4b5a8..7cb602ab02ab 100644 --- a/scripts/ci/pr-bot/processNewPrs.ts +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -21,7 +21,9 @@ const { getChecksStatus } = require("./shared/checks"); const commentStrings = require("./shared/commentStrings"); const { ReviewerConfig } = require("./shared/reviewerConfig"); const { PersistentState } = require("./shared/persistentState"); +const { Pr } = require("./shared/pr"); const { REPO_OWNER, REPO, PATH_TO_CONFIG_FILE } = require("./shared/constants"); +import { CheckStatus } from "./shared/checks"; // Returns true if the pr needs to be processed or false otherwise. // We don't need to process PRs that: @@ -33,7 +35,7 @@ const { REPO_OWNER, REPO, PATH_TO_CONFIG_FILE } = require("./shared/constants"); // 6) Have notifications stopped // unless we're supposed to remind the user after tests pass // (in which case that's all we need to do). -function needsProcessed(pull, prState): boolean { +function needsProcessed(pull, prState: typeof Pr): boolean { if (prState.remindAfterTestsPass && prState.remindAfterTestsPass.length > 0) { return true; } @@ -73,7 +75,12 @@ function needsProcessed(pull, prState): boolean { } // If the checks passed in via checkstate have completed, notifies the users who have configured notifications. -async function remindIfChecksCompleted(pull, stateClient, checkState, prState) { +async function remindIfChecksCompleted( + pull, + stateClient: typeof PersistentState, + checkState: CheckStatus, + prState: typeof Pr +) { console.log( `Notifying reviewers if checks for PR ${pull.number} have completed, then returning` ); @@ -94,8 +101,12 @@ async function remindIfChecksCompleted(pull, stateClient, checkState, prState) { } } -// If we haven't already -async function notifyChecksFailed(pull, stateClient, prState) { +// If we haven't already, let the author know checks are failing. +async function notifyChecksFailed( + pull, + stateClient: typeof PersistentState, + prState: typeof Pr +) { console.log( `Checks are failing for PR ${pull.number}. Commenting if we haven't already and skipping.` ); @@ -115,7 +126,11 @@ async function notifyChecksFailed(pull, stateClient, prState) { // 3) Picking/assigning reviewers // 4) Adding "Next Action: Reviewers label" // 5) Storing the state of the pull request/reviewers in a dedicated branch. -async function processPull(pull, reviewerConfig, stateClient) { +async function processPull( + pull, + reviewerConfig: typeof ReviewerConfig, + stateClient: typeof PersistentState +) { let prState = await stateClient.getPrState(pull.number); if (!needsProcessed(pull, prState)) { return; diff --git a/scripts/ci/pr-bot/processPrUpdate.ts b/scripts/ci/pr-bot/processPrUpdate.ts index 8e409ffa95ff..ae6b51192d31 100644 --- a/scripts/ci/pr-bot/processPrUpdate.ts +++ b/scripts/ci/pr-bot/processPrUpdate.ts @@ -26,16 +26,16 @@ const { PATH_TO_CONFIG_FILE } = require("./shared/constants"); async function areReviewersAssigned( pullNumber: number, - stateClient: any + stateClient: typeof PersistentState ): Promise { const prState = await stateClient.getPrState(pullNumber); return Object.values(prState.reviewersAssignedForLabels).length > 0; } async function processPrComment( - payload: any, - stateClient: any, - reviewerConfig: any + payload, + stateClient: typeof PersistentState, + reviewerConfig: typeof ReviewerConfig ) { const commentContents = payload.comment.body; const commentAuthor = payload.sender.login; @@ -72,9 +72,9 @@ async function processPrComment( // On approval from a reviewer we have assigned, assign committer if one not already assigned async function processPrReview( - payload: any, - stateClient: any, - reviewerConfig: any + payload, + stateClient: typeof PersistentState, + reviewerConfig: typeof ReviewerConfig ) { if (payload.review.state != "approved") { return; @@ -123,7 +123,10 @@ async function processPrReview( } // On pr push or author comment, we should put the attention set back on the reviewers -async function setNextActionReviewers(payload: any, stateClient: any) { +async function setNextActionReviewers( + payload, + stateClient: typeof PersistentState +) { const pullNumber = payload.issue?.number || payload.pull_request?.number; if (!(await areReviewersAssigned(pullNumber, stateClient))) { console.log("No reviewers assigned, dont need to manipulate attention set"); From 665e637175d85ba093b8c04f72ffb664fd6e13cd Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Fri, 25 Feb 2022 10:26:50 -0500 Subject: [PATCH 3/5] Filter non-go prs --- scripts/ci/pr-bot/processNewPrs.ts | 7 +++++++ scripts/ci/pr-bot/processPrUpdate.ts | 8 +------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts index 7cb602ab02ab..19f000fb298a 100644 --- a/scripts/ci/pr-bot/processNewPrs.ts +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -33,9 +33,16 @@ import { CheckStatus } from "./shared/checks"; // 4) Are closed // 5) Have already been processed // 6) Have notifications stopped +// 7) The pr doesn't contain the go label (temporary). TODO(damccorm) - remove this when we're ready to roll this out to everyone. // unless we're supposed to remind the user after tests pass // (in which case that's all we need to do). function needsProcessed(pull, prState: typeof Pr): boolean { + if (!pull.labels.find((label) => label.name.toLowerCase() == "go")) { + console.log( + `Skipping PR ${pull.number} because it doesn't contain the go label` + ); + return false; + } if (prState.remindAfterTestsPass && prState.remindAfterTestsPass.length > 0) { return true; } diff --git a/scripts/ci/pr-bot/processPrUpdate.ts b/scripts/ci/pr-bot/processPrUpdate.ts index ae6b51192d31..91cc07b30939 100644 --- a/scripts/ci/pr-bot/processPrUpdate.ts +++ b/scripts/ci/pr-bot/processPrUpdate.ts @@ -148,13 +148,7 @@ async function processPrUpdate() { // TODO(damccorm) - remove this when we roll out to more than go const existingLabels = payload.issue?.labels || payload.pull_request?.labels; - let containsGoLabel = false; - existingLabels.forEach((label) => { - if (label.name.toLowerCase() == "go") { - containsGoLabel = true; - } - }); - if (!containsGoLabel) { + if (!existingLabels.find((label) => label.name.toLowerCase() == "go")) { console.log("Does not contain the go label - skipping"); return; } From e033edd74b00cf10340f4bd99fcaf8e3f49335e1 Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Sun, 27 Feb 2022 13:08:32 -0500 Subject: [PATCH 4/5] Respond to feedback --- scripts/ci/pr-bot/.gitignore | 3 - scripts/ci/pr-bot/processNewPrs.ts | 98 +++++++++++++------------ scripts/ci/pr-bot/processPrUpdate.ts | 56 ++++++++------ scripts/ci/pr-bot/shared/githubUtils.ts | 8 ++ scripts/ci/pr-bot/shared/userCommand.ts | 6 +- 5 files changed, 95 insertions(+), 76 deletions(-) diff --git a/scripts/ci/pr-bot/.gitignore b/scripts/ci/pr-bot/.gitignore index 132f5f8e9b26..7b665129e694 100644 --- a/scripts/ci/pr-bot/.gitignore +++ b/scripts/ci/pr-bot/.gitignore @@ -13,9 +13,6 @@ logs results tmp -# Build -public/css/main.css - # Coverage reports coverage diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts index 19f000fb298a..411e3b5428ec 100644 --- a/scripts/ci/pr-bot/processNewPrs.ts +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -25,19 +25,21 @@ const { Pr } = require("./shared/pr"); const { REPO_OWNER, REPO, PATH_TO_CONFIG_FILE } = require("./shared/constants"); import { CheckStatus } from "./shared/checks"; -// Returns true if the pr needs to be processed or false otherwise. -// We don't need to process PRs that: -// 1) Have WIP in their name -// 2) Are less than 20 minutes old -// 3) Are draft prs -// 4) Are closed -// 5) Have already been processed -// 6) Have notifications stopped -// 7) The pr doesn't contain the go label (temporary). TODO(damccorm) - remove this when we're ready to roll this out to everyone. -// unless we're supposed to remind the user after tests pass -// (in which case that's all we need to do). -function needsProcessed(pull, prState: typeof Pr): boolean { - if (!pull.labels.find((label) => label.name.toLowerCase() == "go")) { +/* + * Returns true if the pr needs to be processed or false otherwise. + * We don't need to process PRs that: + * 1) Have WIP in their name + * 2) Are less than 20 minutes old + * 3) Are draft prs + * 4) Are closed + * 5) Have already been processed + * 6) Have notifications stopped + * 7) The pr doesn't contain the go label (temporary). TODO(damccorm) - remove this when we're ready to roll this out to everyone. + * unless we're supposed to remind the user after tests pass + * (in which case that's all we need to do). + */ +function needsProcessed(pull: any, prState: typeof Pr): boolean { + if (!pull.labels.find((label) => label.name.toLowerCase() === "go")) { console.log( `Skipping PR ${pull.number} because it doesn't contain the go label` ); @@ -57,7 +59,7 @@ function needsProcessed(pull, prState: typeof Pr): boolean { ); return false; } - if (pull.state.toLowerCase() != "open") { + if (pull.state.toLowerCase() !== "open") { console.log(`Skipping PR ${pull.number} because it is closed`); return false; } @@ -81,9 +83,11 @@ function needsProcessed(pull, prState: typeof Pr): boolean { return true; } -// If the checks passed in via checkstate have completed, notifies the users who have configured notifications. +/* + * If the checks passed in via checkstate have completed, notifies the users who have configured notifications. + */ async function remindIfChecksCompleted( - pull, + pull: any, stateClient: typeof PersistentState, checkState: CheckStatus, prState: typeof Pr @@ -91,26 +95,29 @@ async function remindIfChecksCompleted( console.log( `Notifying reviewers if checks for PR ${pull.number} have completed, then returning` ); - if (checkState.completed) { - if (checkState.succeeded) { - await github.addPrComment( - pull.number, - commentStrings.allChecksPassed(prState.remindAfterTestsPass) - ); - } else { - await github.addPrComment( - pull.number, - commentStrings.someChecksFailing(prState.remindAfterTestsPass) - ); - } - prState.remindAfterTestsPass = []; - await stateClient.writePrState(pull.number, prState); + if (!checkState.completed) { + return; + } + if (checkState.succeeded) { + await github.addPrComment( + pull.number, + commentStrings.allChecksPassed(prState.remindAfterTestsPass) + ); + } else { + await github.addPrComment( + pull.number, + commentStrings.someChecksFailing(prState.remindAfterTestsPass) + ); } + prState.remindAfterTestsPass = []; + await stateClient.writePrState(pull.number, prState); } -// If we haven't already, let the author know checks are failing. +/* + * If we haven't already, let the author know checks are failing. + */ async function notifyChecksFailed( - pull, + pull: any, stateClient: typeof PersistentState, prState: typeof Pr ) { @@ -127,14 +134,16 @@ async function notifyChecksFailed( await stateClient.writePrState(pull.number, prState); } -// Performs all the business logic of processing a new pull request, including: -// 1) Checking if it needs processed -// 2) Reminding reviewers if checks have completed (if they've subscribed to that) -// 3) Picking/assigning reviewers -// 4) Adding "Next Action: Reviewers label" -// 5) Storing the state of the pull request/reviewers in a dedicated branch. +/* + * Performs all the business logic of processing a new pull request, including: + * 1) Checking if it needs processed + * 2) Reminding reviewers if checks have completed (if they've subscribed to that) + * 3) Picking/assigning reviewers + * 4) Adding "Next Action: Reviewers label" + * 5) Storing the state of the pull request/reviewers in a dedicated branch. + */ async function processPull( - pull, + pull: any, reviewerConfig: typeof ReviewerConfig, stateClient: typeof PersistentState ) { @@ -160,15 +169,14 @@ async function processPull( prState.commentedAboutFailingChecks = false; // Pick reviewers to assign. Store them in reviewerStateToUpdate and update the prState object with those reviewers (and their associated labels) - let reviewerStateToUpdate = {}; + let reviewerStateToUpdate: { [key: string]: typeof ReviewersForLabel } = {}; const reviewersForLabels: { [key: string]: string[] } = reviewerConfig.getReviewersForLabels(pull.labels, [pull.user.login]); var labels = Object.keys(reviewersForLabels); if (!labels || labels.length === 0) { return; } - for (let i = 0; i < labels.length; i++) { - let label = labels[i]; + for (const label of labels) { let availableReviewers = reviewersForLabels[label]; let reviewersState = await stateClient.getReviewersForLabelState(label); let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); @@ -187,8 +195,7 @@ async function processPull( await stateClient.writePrState(pull.number, prState); let labelsToUpdate = Object.keys(reviewerStateToUpdate); - for (let i = 0; i < labelsToUpdate.length; i++) { - let label = labelsToUpdate[i]; + for (const label of labelsToUpdate) { await stateClient.writeReviewersForLabelState( label, reviewerStateToUpdate[label] @@ -209,8 +216,7 @@ async function processNewPrs() { } ); - for (let i = 0; i < openPulls.length; i++) { - let pull = openPulls[i]; + for (const pull of openPulls) { await processPull(pull, reviewerConfig, stateClient); } } diff --git a/scripts/ci/pr-bot/processPrUpdate.ts b/scripts/ci/pr-bot/processPrUpdate.ts index 91cc07b30939..154444ee076b 100644 --- a/scripts/ci/pr-bot/processPrUpdate.ts +++ b/scripts/ci/pr-bot/processPrUpdate.ts @@ -19,11 +19,18 @@ const github = require("@actions/github"); const commentStrings = require("./shared/commentStrings"); const { processCommand } = require("./shared/userCommand"); -const { addPrComment, nextActionReviewers } = require("./shared/githubUtils"); +const { + addPrComment, + nextActionReviewers, + getPullAuthorFromPayload, + getPullNumberFromPayload, +} = require("./shared/githubUtils"); const { PersistentState } = require("./shared/persistentState"); const { ReviewerConfig } = require("./shared/reviewerConfig"); const { PATH_TO_CONFIG_FILE } = require("./shared/constants"); +const reviewerAction = "Reviewers"; + async function areReviewersAssigned( pullNumber: number, stateClient: typeof PersistentState @@ -33,7 +40,7 @@ async function areReviewersAssigned( } async function processPrComment( - payload, + payload: any, stateClient: typeof PersistentState, reviewerConfig: typeof ReviewerConfig ) { @@ -59,9 +66,8 @@ async function processPrComment( console.log( "No command to be processed, checking if we should shift attention to reviewers" ); - const pullAuthor = - payload.issue?.user?.login || payload.pull_request?.user?.login; - if (pullAuthor == commentAuthor) { + const pullAuthor = getPullAuthorFromPayload(payload); + if (pullAuthor === commentAuthor) { await setNextActionReviewers(payload, stateClient); } else { console.log( @@ -70,17 +76,19 @@ async function processPrComment( } } -// On approval from a reviewer we have assigned, assign committer if one not already assigned +/* + * On approval from a reviewer we have assigned, assign committer if one not already assigned + */ async function processPrReview( - payload, + payload: any, stateClient: typeof PersistentState, reviewerConfig: typeof ReviewerConfig ) { - if (payload.review.state != "approved") { + if (payload.review.state !== "approved") { return; } - const pullNumber = payload.issue?.number || payload.pull_request?.number; + const pullNumber = getPullNumberFromPayload(payload); if (!(await areReviewersAssigned(pullNumber, stateClient))) { return; } @@ -91,14 +99,14 @@ async function processPrReview( return; } - let labelOfReviewer = prState.getLabelForReviewer(payload.sender.login); + const labelOfReviewer = prState.getLabelForReviewer(payload.sender.login); if (labelOfReviewer) { let reviewersState = await stateClient.getReviewersForLabelState( labelOfReviewer ); - let availableReviewers = + const availableReviewers = reviewerConfig.getReviewersForLabel(labelOfReviewer); - let chosenCommitter = await reviewersState.assignNextCommitter( + const chosenCommitter = await reviewersState.assignNextCommitter( availableReviewers ); prState.reviewersAssignedForLabels[labelOfReviewer] = chosenCommitter; @@ -111,7 +119,7 @@ async function processPrReview( const existingLabels = payload.issue?.labels || payload.pull_request?.labels; await nextActionReviewers(pullNumber, existingLabels); - prState.nextAction = "Reviewers"; + prState.nextAction = reviewerAction; // Persist state await stateClient.writePrState(pullNumber, prState); @@ -122,12 +130,14 @@ async function processPrReview( } } -// On pr push or author comment, we should put the attention set back on the reviewers +/* + * On pr push or author comment, we should put the attention set back on the reviewers + */ async function setNextActionReviewers( - payload, + payload: any, stateClient: typeof PersistentState ) { - const pullNumber = payload.issue?.number || payload.pull_request?.number; + const pullNumber = getPullNumberFromPayload(payload); if (!(await areReviewersAssigned(pullNumber, stateClient))) { console.log("No reviewers assigned, dont need to manipulate attention set"); return; @@ -135,7 +145,7 @@ async function setNextActionReviewers( const existingLabels = payload.issue?.labels || payload.pull_request?.labels; await nextActionReviewers(pullNumber, existingLabels); let prState = await stateClient.getPrState(pullNumber); - prState.nextAction = "Reviewers"; + prState.nextAction = reviewerAction; await stateClient.writePrState(pullNumber, prState); } @@ -148,7 +158,7 @@ async function processPrUpdate() { // TODO(damccorm) - remove this when we roll out to more than go const existingLabels = payload.issue?.labels || payload.pull_request?.labels; - if (!existingLabels.find((label) => label.name.toLowerCase() == "go")) { + if (!existingLabels.find((label) => label.name.toLowerCase() === "go")) { console.log("Does not contain the go label - skipping"); return; } @@ -157,10 +167,10 @@ async function processPrUpdate() { console.log("Issue, not pull request - returning"); return; } - const pullNumber = payload.issue?.number || payload.pull_request?.number; + const pullNumber = getPullNumberFromPayload(payload); - let stateClient = new PersistentState(); - let prState = await stateClient.getPrState(pullNumber); + const stateClient = new PersistentState(); + const prState = await stateClient.getPrState(pullNumber); if (prState.stopReviewerNotifications) { console.log("Notifications have been paused for this pull - skipping"); return; @@ -170,7 +180,7 @@ async function processPrUpdate() { case "pull_request_review_comment": case "issue_comment": console.log("Processing comment event"); - if (payload.action != "created") { + if (payload.action !== "created") { console.log("Comment wasnt just created, skipping"); return; } @@ -181,7 +191,7 @@ async function processPrUpdate() { await processPrReview(payload, stateClient, reviewerConfig); break; case "pull_request_target": - if (payload.action == "synchronize") { + if (payload.action === "synchronize") { console.log("Processing synchronize action"); await setNextActionReviewers(payload, stateClient); } diff --git a/scripts/ci/pr-bot/shared/githubUtils.ts b/scripts/ci/pr-bot/shared/githubUtils.ts index e650ca52b0d0..667255b16924 100644 --- a/scripts/ci/pr-bot/shared/githubUtils.ts +++ b/scripts/ci/pr-bot/shared/githubUtils.ts @@ -85,6 +85,14 @@ export async function checkIfCommitter(username: string): Promise { ); } +export function getPullAuthorFromPayload(payload: any) { + return payload.issue?.user?.login || payload.pull_request?.user?.login; +} + +export function getPullNumberFromPayload(payload: any) { + return payload.issue?.number || payload.pull_request?.number; +} + function removeNextActionLabel(existingLabels: Label[]): string[] { return existingLabels .filter( diff --git a/scripts/ci/pr-bot/shared/userCommand.ts b/scripts/ci/pr-bot/shared/userCommand.ts index 34e2741a3f6d..e32746eb7fce 100644 --- a/scripts/ci/pr-bot/shared/userCommand.ts +++ b/scripts/ci/pr-bot/shared/userCommand.ts @@ -81,8 +81,7 @@ async function assignToNextReviewer( let reviewersState = await stateClient.getReviewersForLabelState( labelOfReviewer ); - const pullAuthor = - payload.issue?.user?.login || payload.pull_request?.user?.login; + const pullAuthor = github.getPullAuthorFromPayload(payload); let availableReviewers = reviewerConfig.getReviewersForLabel( labelOfReviewer, [commentAuthor, pullAuthor] @@ -187,8 +186,7 @@ async function assignReviewerSet( } const existingLabels = payload.issue?.labels || payload.pull_request?.labels; - const pullAuthor = - payload.issue?.user?.login || payload.pull_request?.user?.login; + const pullAuthor = github.getPullAuthorFromPayload(payload); const reviewersForLabels = reviewerConfig.getReviewersForLabels( existingLabels, [pullAuthor] From 8608439309caf97ef145d8b100ff164fbfa5a611 Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Wed, 2 Mar 2022 09:24:25 -0500 Subject: [PATCH 5/5] Turn pr bot on for go prs --- .github/workflows/pr-bot-new-prs.yml | 39 ++++++++++++++++++++++++ .github/workflows/pr-bot-pr-updates.yml | 40 +++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 .github/workflows/pr-bot-new-prs.yml create mode 100644 .github/workflows/pr-bot-pr-updates.yml diff --git a/.github/workflows/pr-bot-new-prs.yml b/.github/workflows/pr-bot-new-prs.yml new file mode 100644 index 000000000000..297042bb0ad8 --- /dev/null +++ b/.github/workflows/pr-bot-new-prs.yml @@ -0,0 +1,39 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: pr-bot-new-prs + +# Run every 30 minutes +on: + schedule: + - cron: '30 * * * *' + workflow_dispatch: + +jobs: + process-prs: + # Don't run on forks + if: github.repository == 'apache/beam' + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + - run: npm install + working-directory: 'scripts/ci/pr-bot' + + # Runs a set of commands using the runners shell + - run: npm run processNewPrs + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + working-directory: 'scripts/ci/pr-bot' diff --git a/.github/workflows/pr-bot-pr-updates.yml b/.github/workflows/pr-bot-pr-updates.yml new file mode 100644 index 000000000000..e6f8610ef0c6 --- /dev/null +++ b/.github/workflows/pr-bot-pr-updates.yml @@ -0,0 +1,40 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: pr-bot-pr-updates + +on: + pull_request_review: + pull_request_review_comment: + pull_request_target: + types: ["synchronize"] # Synchronize is the action that runs after pushes to the user branch + issue_comment: + +jobs: + process-pr-update: + # Don't run on forks + if: github.repository == 'apache/beam' + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + - run: npm install + working-directory: 'scripts/ci/pr-bot' + + # Runs a set of commands using the runners shell + - run: npm run processPrUpdate + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + working-directory: 'scripts/ci/pr-bot'