From 518c7ff0e320e6af85e8ee1f43753ad832d57b58 Mon Sep 17 00:00:00 2001 From: Danny McCormick Date: Fri, 18 Mar 2022 20:01:33 -0400 Subject: [PATCH] [BEAM-13925] Add weekly automation to update our reviewer config (#17034) --- .github/REVIEWERS.yml | 18 +- .github/workflows/pr-bot-update-reviewers.yml | 39 +++ scripts/ci/pr-bot/package.json | 1 + scripts/ci/pr-bot/shared/commentStrings.ts | 36 +++ scripts/ci/pr-bot/shared/reviewerConfig.ts | 48 ++- scripts/ci/pr-bot/updateReviewers.ts | 302 ++++++++++++++++++ 6 files changed, 437 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/pr-bot-update-reviewers.yml create mode 100644 scripts/ci/pr-bot/updateReviewers.ts diff --git a/.github/REVIEWERS.yml b/.github/REVIEWERS.yml index 009489798a45..77d39975abc7 100644 --- a/.github/REVIEWERS.yml +++ b/.github/REVIEWERS.yml @@ -13,9 +13,17 @@ # See the License for the specific language governing permissions and # limitations under the License. +# If you would like to be excluded from consideration for reviewing a certain label, +# add yourself to that label's exclusionList +# FallbackReviewers is for reviewers who can review any area of the code base that might +# not receive a label. These should generally be more experienced committers. labels: -- name: "Go" - reviewers: ["damccorm", "lostluck", "jrmccluskey", "youngoli", "riteshghorse"] - exclusionList: [] # These users will never be suggested as reviewers -# I don't know the other areas well enough to assess who the normal committers/contributors who might want to be reviewers are -fallbackReviewers: [] # List of committers to use when no label matches + - name: Go + reviewers: + - damccorm + - lostluck + - jrmccluskey + - youngoli + - riteshghorse + exclusionList: [] +fallbackReviewers: [] diff --git a/.github/workflows/pr-bot-update-reviewers.yml b/.github/workflows/pr-bot-update-reviewers.yml new file mode 100644 index 000000000000..701f2f1c560a --- /dev/null +++ b/.github/workflows/pr-bot-update-reviewers.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-update-reviewers + +# Run every week on Sunday at 16:05 UTC +on: + schedule: + - cron: '5 16 * * 0' + workflow_dispatch: + +jobs: + update-reviewers: + # 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 updateReviewers + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + working-directory: 'scripts/ci/pr-bot' diff --git a/scripts/ci/pr-bot/package.json b/scripts/ci/pr-bot/package.json index acb7a2d31b4a..2ae28ff2f704 100644 --- a/scripts/ci/pr-bot/package.json +++ b/scripts/ci/pr-bot/package.json @@ -10,6 +10,7 @@ "processNewPrs": "npm run build && node lib/processNewPrs.js", "processPrUpdate": "npm run build && node lib/processPrUpdate.js", "gatherMetrics": "npm run build && node lib/gatherMetrics.js", + "updateReviewers": "npm run build && node lib/updateReviewers.js", "findPrsNeedingAttention": "npm run build && node lib/findPrsNeedingAttention.js" }, "dependencies": { diff --git a/scripts/ci/pr-bot/shared/commentStrings.ts b/scripts/ci/pr-bot/shared/commentStrings.ts index 9ea139f880a9..39f3d5a51293 100644 --- a/scripts/ci/pr-bot/shared/commentStrings.ts +++ b/scripts/ci/pr-bot/shared/commentStrings.ts @@ -73,6 +73,42 @@ export function noLegalReviewers(): string { return "No reviewers could be found from any of the labels on the PR or in the fallback reviewers list. Check the config file to make sure reviewers are configured"; } +export function updateReviewerConfig( + reviewersAddedForLabels: { [reviewer: string]: string[] }, + reviewersRemovedForLabels: { [reviewer: string]: string[] } +) { + let commentString = `Adds and/or removes reviewers based on activity in the repo. +If you have been added and would prefer not to be, you can avoid getting repeatedly suggested by adding yourself to that label's exclusionList. +`; + + if (Object.keys(reviewersAddedForLabels).length > 0) { + commentString += ` +The following users have been added as reviewers to the configuration. +If you choose to accept being added, you will be added to the rotation of users who are automatically added to pull requests for an initial review. +A committer will still have to approve after your review (if you are not a committer), but you will be the initial touchpoint for PRs to which you are assigned. +`; + for (const reviewer of Object.keys(reviewersAddedForLabels)) { + commentString += `${reviewer} added for label(s): ${reviewersAddedForLabels[ + reviewer + ].join(",")}\n`; + } + } + + if (Object.keys(reviewersRemovedForLabels).length > 0) { + commentString += ` +The following users have been removed as reviewers from the configuration. +Users are removed if they haven't reviewed or completed a PR in the last 3 months. +`; + for (const reviewer of Object.keys(reviewersRemovedForLabels)) { + commentString += `@${reviewer} removed for label(s): ${reviewersRemovedForLabels[ + reviewer + ].join(",")}\n`; + } + } + + return commentString; +} + export function assignNewReviewer(labelToReviewerMapping: { [label: string]: string; }): string { diff --git a/scripts/ci/pr-bot/shared/reviewerConfig.ts b/scripts/ci/pr-bot/shared/reviewerConfig.ts index a412ee6acac7..8fc0d32b014b 100644 --- a/scripts/ci/pr-bot/shared/reviewerConfig.ts +++ b/scripts/ci/pr-bot/shared/reviewerConfig.ts @@ -23,10 +23,24 @@ const { NO_MATCHING_LABEL } = require("./constants"); export class ReviewerConfig { private config: any; + private configPath: string; constructor(pathToConfigFile) { this.config = yaml.load( fs.readFileSync(pathToConfigFile, { encoding: "utf-8" }) ); + this.configPath = pathToConfigFile; + } + + // Returns all possible reviewers for each label configured. + getReviewersForAllLabels(): { [key: string]: string[] } { + const labelObjects = this.config.labels; + let reviewersForLabels = {}; + for (const labelObject of labelObjects) { + reviewersForLabels[labelObject.name.toLowerCase()] = + labelObject.reviewers; + } + + return reviewersForLabels; } // Given a list of labels and an exclusion list of reviewers not to include (e.g. the author) @@ -56,7 +70,7 @@ export class ReviewerConfig { // Get possible reviewers excluding the author. getReviewersForLabel(label: string, exclusionList: string[]): string[] { - var labelObjects = this.config.labels; + const labelObjects = this.config.labels; const labelObject = labelObjects.find( (labelObject) => labelObject.name.toLowerCase() === label.toLowerCase() ); @@ -67,8 +81,38 @@ export class ReviewerConfig { return this.excludeFromReviewers(labelObject.reviewers, exclusionList); } + updateReviewerForLabel(label: string, reviewers: string[]) { + const labelIndex = this.config.labels.findIndex( + (labelObject) => labelObject.name.toLowerCase() === label.toLowerCase() + ); + this.config.labels[labelIndex].reviewers = reviewers; + + const contents = `# 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. + +# If you would like to be excluded from consideration for reviewing a certain label, +# add yourself to that label's exclusionList +# FallbackReviewers is for reviewers who can review any area of the code base that might +# not receive a label. These should generally be more experienced committers. +${yaml.dump(this.config)}`; + + fs.writeFileSync(this.configPath, contents, { encoding: "utf-8" }); + } + getExclusionListForLabel(label: string): string[] { - var labelObjects = this.config.labels; + const labelObjects = this.config.labels; const labelObject = labelObjects.find( (labelObject) => labelObject.name.toLowerCase() === label.toLowerCase() ); diff --git a/scripts/ci/pr-bot/updateReviewers.ts b/scripts/ci/pr-bot/updateReviewers.ts new file mode 100644 index 000000000000..c49f8feba24f --- /dev/null +++ b/scripts/ci/pr-bot/updateReviewers.ts @@ -0,0 +1,302 @@ +/* + * 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 exec = require("@actions/exec"); +const github = require("./shared/githubUtils"); +const commentStrings = require("./shared/commentStrings"); +const { + REPO_OWNER, + REPO, + BOT_NAME, + PATH_TO_CONFIG_FILE, +} = require("./shared/constants"); +const { ReviewerConfig } = require("./shared/reviewerConfig"); + +async function getPullsFromLastThreeMonths(): Promise { + const cutoffDate = new Date(); + cutoffDate.setMonth(cutoffDate.getMonth() - 3); + console.log(`Getting PRs newer than ${cutoffDate}`); + const githubClient = github.getGitHubClient(); + let result = await githubClient.rest.pulls.list({ + owner: REPO_OWNER, + repo: REPO, + state: "closed", + per_page: 100, // max allowed + }); + let page = 2; + let retries = 0; + let pulls = result.data; + while ( + result.data.length > 0 && + new Date(result.data[result.data.length - 1].created_at) > cutoffDate + ) { + if (retries === 0) { + console.log(`Getting PRs, page: ${page}`); + console.log( + `Current oldest PR = ${new Date( + result.data[result.data.length - 1].created_at + )}` + ); + } + try { + result = await githubClient.rest.pulls.list({ + owner: REPO_OWNER, + repo: REPO, + state: "closed", + per_page: 100, // max allowed + page: page, + }); + pulls = pulls.concat(result.data); + page++; + retries = 0; + } catch (err) { + if (retries >= 3) { + throw err; + } + retries++; + } + } + return pulls; +} + +interface pullActivity { + reviews: number; + pullsAuthored: number; +} + +interface reviewerActivity { + reviewers: { [reviewer: string]: pullActivity }; +} + +async function getReviewersForPull(pull: any): Promise { + let reviewers = new Set(); + const githubClient = github.getGitHubClient(); + let comments = ( + await githubClient.rest.issues.listComments({ + owner: REPO_OWNER, + repo: REPO, + issue_number: pull.number, + }) + ).data; + const reviewComments = ( + await githubClient.rest.pulls.listReviewComments({ + owner: REPO_OWNER, + repo: REPO, + pull_number: pull.number, + }) + ).data; + comments = comments.concat(reviewComments); + + for (const comment of comments) { + if ( + comment.user.login && + comment.user.login !== pull.user.login && + comment.user.login !== BOT_NAME + ) { + reviewers.add(comment.user.login.toLowerCase()); + } + } + + return [...reviewers]; +} + +function addReviewerActivity( + reviewerActivity: reviewerActivity, + reviewers: string[], + author: string +): reviewerActivity { + if (!reviewerActivity) { + reviewerActivity = { + reviewers: {}, + }; + } + + if (author in reviewerActivity.reviewers) { + reviewerActivity.reviewers[author].pullsAuthored++; + } else { + reviewerActivity.reviewers[author] = { + reviews: 0, + pullsAuthored: 1, + }; + } + + for (const reviewer of reviewers) { + if (reviewer !== author) { + if (reviewer in reviewerActivity.reviewers) { + reviewerActivity.reviewers[reviewer].reviews++; + } else { + reviewerActivity.reviewers[reviewer] = { + reviews: 1, + pullsAuthored: 0, + }; + } + } + } + + return reviewerActivity; +} + +async function getReviewerActivityByLabel( + pulls: any[] +): Promise<{ [label: string]: reviewerActivity }> { + let reviewerActivityByLabel: { [label: string]: reviewerActivity } = {}; + for (const pull of pulls) { + console.log(`Processing PR ${pull.number}`); + const author = pull.user.login.toLowerCase(); + if (author !== BOT_NAME) { + const reviewers = await getReviewersForPull(pull); + const labels = pull.labels; + for (const label of labels) { + const labelName = label.name.toLowerCase(); + reviewerActivityByLabel[labelName] = addReviewerActivity( + reviewerActivityByLabel[labelName], + reviewers, + author + ); + } + } + } + + return reviewerActivityByLabel; +} + +interface configUpdates { + reviewersAddedForLabels: { [reviewer: string]: string[] }; + reviewersRemovedForLabels: { [reviewer: string]: string[] }; +} + +function reviewerIsBot(reviewer: string): boolean { + if ( + ["codecov", "github-actions"].find( + (bot) => reviewer.toLowerCase().indexOf(bot) != -1 + ) + ) { + return true; + } + return false; +} + +function updateReviewerConfig( + reviewerActivityByLabel: { [label: string]: reviewerActivity }, + reviewerConfig: typeof ReviewerConfig +): configUpdates { + let updates: configUpdates = { + reviewersAddedForLabels: {}, + reviewersRemovedForLabels: {}, + }; + const currentReviewersForLabels = reviewerConfig.getReviewersForAllLabels(); + for (const label of Object.keys(currentReviewersForLabels)) { + // Remove any reviewers with no reviews or pulls created + let reviewers = currentReviewersForLabels[label]; + let updatedReviewers: string[] = []; + const exclusionList = reviewerConfig.getExclusionListForLabel(label); + for (const reviewer of reviewers) { + if (reviewerActivityByLabel[label].reviewers[reviewer]) { + updatedReviewers.push(reviewer); + } else { + if (reviewer in updates.reviewersRemovedForLabels) { + updates.reviewersRemovedForLabels[reviewer].push(label); + } else { + updates.reviewersRemovedForLabels[reviewer] = [label]; + } + } + } + + // Add any reviewers who have at least 5 combined reviews + pulls authored + for (const reviewer of Object.keys( + reviewerActivityByLabel[label].reviewers + )) { + const reviewerContributions = + reviewerActivityByLabel[label].reviewers[reviewer].reviews + + reviewerActivityByLabel[label].reviewers[reviewer].pullsAuthored; + + if ( + reviewerContributions >= 5 && + updatedReviewers.indexOf(reviewer) < 0 && + exclusionList.indexOf(reviewer) < 0 && + !reviewerIsBot(reviewer) + ) { + updatedReviewers.push(reviewer); + if (reviewer in updates.reviewersAddedForLabels) { + updates.reviewersAddedForLabels[reviewer].push(label); + } else { + updates.reviewersAddedForLabels[reviewer] = [label]; + } + } + } + + console.log( + `Updated reviewers for label ${label}: ${updatedReviewers.join(",")}` + ); + + reviewerConfig.updateReviewerForLabel(label, updatedReviewers); + } + + return updates; +} + +async function openPull(updates: configUpdates) { + const curDate = new Date(); + const branch = `pr-bot-${ + curDate.getMonth() + 1 + }-${curDate.getDay()}-${curDate.getFullYear()}-${curDate.getHours()}-${curDate.getMinutes()}-${curDate.getSeconds()}`; + await exec.exec(`git config user.email ${BOT_NAME}@github.com`); + await exec.exec("git config pull.rebase false"); + await exec.exec(`git checkout -b ${branch}`); + await exec.exec(`git add ${PATH_TO_CONFIG_FILE}`); + await exec.exec( + `git commit -m "Updating reviewer config based on historical trends"` + ); + await exec.exec(`git push origin ${branch}`); + + const prBody = commentStrings.updateReviewerConfig( + updates.reviewersAddedForLabels, + updates.reviewersRemovedForLabels + ); + await github.getGitHubClient().rest.pulls.create({ + owner: REPO_OWNER, + repo: REPO, + head: branch, + base: "master", + title: "Update reviewer config to reviewers with activity in repo areas", + body: prBody, + maintainer_can_modify: true, + }); +} + +async function updateReviewers() { + const pulls = await getPullsFromLastThreeMonths(); + console.log("Got all PRs, moving to the processing stage"); + const reviewerActivityByLabel = await getReviewerActivityByLabel(pulls); + console.log("Processed all PRs to determine reviewer activity"); + let reviewerConfig = new ReviewerConfig(PATH_TO_CONFIG_FILE); + const updates = updateReviewerConfig(reviewerActivityByLabel, reviewerConfig); + if ( + Object.keys(updates.reviewersAddedForLabels).length > 0 || + Object.keys(updates.reviewersRemovedForLabels).length > 0 + ) { + console.log(`Suggested updates to ${PATH_TO_CONFIG_FILE}`); + await openPull(updates); + } else { + console.log("No updates to suggest"); + } +} + +updateReviewers(); + +export {};