KonradJanica commented on a change in pull request #16898: URL: https://github.com/apache/beam/pull/16898#discussion_r814211130
########## File path: scripts/ci/pr-bot/shared/reviewerConfig.ts ########## @@ -0,0 +1,102 @@ +/* + * 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 yaml = require("js-yaml"); +const fs = require("fs"); +import { Label } from "./githubUtils"; + +export class ReviewerConfig { + private config: any; + constructor(pathToConfigFile) { + this.config = yaml.load( + fs.readFileSync(pathToConfigFile, { encoding: "utf-8" }) + ); + } + + // Given a list of labels and an exclusion list of reviewers not to include (e.g. the author) + // returns all possible reviewers for each label + getReviewersForLabels( + labels: Label[], + exclusionList: string[] + ): { [key: string]: string[] } { + let reviewersFound = false; + let labelToReviewerMapping = {}; + labels.forEach((label) => { + let reviewers = this.getReviewersForLabel(label.name, exclusionList); + if (reviewers.length > 0) { + labelToReviewerMapping[label.name] = reviewers; + reviewersFound = true; + } + }); + if (!reviewersFound) { + const fallbackReviewers = this.getFallbackReviewers(exclusionList); + if (fallbackReviewers.length > 0) { + labelToReviewerMapping["no-matching-label"] = + this.getFallbackReviewers(exclusionList); + } + } + return labelToReviewerMapping; + } + + // Get possible reviewers excluding the author. + getReviewersForLabel(label: string, exclusionList: string[]): string[] { + var labelObjects = this.config.labels; + for (var i = 0; i < labelObjects.length; i++) { + var labelObject = labelObjects[i]; + if (labelObject.name.toLowerCase() === label.toLowerCase()) { + return this.excludeFromReviewers(labelObject.reviewers, exclusionList); + } + } + return []; + } + + getExclusionListForLabel(label: string): string[] { + var labelObjects = this.config.labels; + for (var i = 0; i < labelObjects.length; i++) { + var labelObject = labelObjects[i]; + if (labelObject.name.toLowerCase() === label.toLowerCase()) { + return labelObject.exclusionList; + } + } + return []; + } + + // Get fallback reviewers excluding the author. + getFallbackReviewers(exclusionList: string[]): string[] { + return this.excludeFromReviewers( + this.config.fallbackReviewers, + exclusionList + ); + } + + private excludeFromReviewers( + reviewers: string[], + exclusionList: string[] + ): string[] { + if (!exclusionList) { + return reviewers; + } + exclusionList.forEach((reviewer) => { + const reviewerIndex = reviewers.indexOf(reviewer); + if (reviewerIndex > -1) { + reviewers.splice(reviewerIndex, 1); + } + }); + return reviewers; Review comment: Perhaps this is cleaner: ``` return reviewers.filter(reviewer => exclusionList.has(reviewer)) ``` ########## File path: scripts/ci/pr-bot/shared/reviewersForLabel.ts ########## @@ -0,0 +1,102 @@ +/* + * 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("./githubUtils"); + +export class ReviewersForLabel { + public label: string; + public dateOfLastReviewAssignment: { [key: string]: number }; + + constructor( + label: string, + propertyDictionary: { + dateOfLastReviewAssignment: { [key: string]: number }; + } + ) { + this.label = label; + this.dateOfLastReviewAssignment = {}; // map of reviewer to date + + if (!propertyDictionary) { + return; + } + if ("dateOfLastReviewAssignment" in propertyDictionary) { + this.dateOfLastReviewAssignment = + propertyDictionary["dateOfLastReviewAssignment"]; + } + } + + // Given a list of available reviewers, + // returns the next reviewer up based on who has reviewed least recently. + // Updates this object to reflect their assignment. + assignNextReviewer(availableReviewers: string[]): string { + if (availableReviewers.length == 0) { + throw new Error(`No reviewers available for label ${this.label}`); + } + + if (!this.dateOfLastReviewAssignment[availableReviewers[0]]) { + this.dateOfLastReviewAssignment[availableReviewers[0]] = Date.now(); + return availableReviewers[0]; + } + + let earliestDate = this.dateOfLastReviewAssignment[availableReviewers[0]]; + let earliestReviewer = availableReviewers[0]; + + for (let i = 0; i < availableReviewers.length; i++) { + let availableReviewer = availableReviewers[i]; + if (!this.dateOfLastReviewAssignment[availableReviewer]) { + this.dateOfLastReviewAssignment[availableReviewer] = Date.now(); + return availableReviewer; + } + if (earliestDate > this.dateOfLastReviewAssignment[availableReviewer]) { + earliestDate = this.dateOfLastReviewAssignment[availableReviewer]; + earliestReviewer = availableReviewer; + } + } + + this.dateOfLastReviewAssignment[earliestReviewer] = Date.now(); + return earliestReviewer; + } + + // Given the up to date list of available reviewers (excluding the author), + // returns the next reviewer up based on who has reviewed least recently. + // Updates this object to reflect their assignment. + assignNextCommitter(availableReviewers: string[]): string { + let earliestDate = Date.now(); + let earliestCommitter: string = ""; + + for (let i = 0; i < availableReviewers.length; i++) { + let availableReviewer = availableReviewers[i]; + if (github.checkIfCommitter(availableReviewer)) { Review comment: Isn't this async? I think an await is missing here. ########## File path: scripts/ci/pr-bot/shared/userCommand.ts ########## @@ -0,0 +1,231 @@ +/* + * 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("./githubUtils"); +const commentStrings = require("./commentStrings"); +const { BOT_NAME } = require("./constants"); +const { StateClient } = require("./persistentState"); +const { ReviewerConfig } = require("./reviewerConfig"); + +// Reads the comment and processes the command if one is contained in it. +// Returns true if it runs a command, false otherwise. +export async function processCommand( + payload: any, + commentAuthor: string, + commentText: string, + stateClient: typeof StateClient, + reviewerConfig: typeof ReviewerConfig +) { + // Don't process any commands from our bot. + if (commentAuthor === BOT_NAME) { + return false; + } + console.log(commentAuthor); + + const pullNumber = payload.issue?.number || payload.pull_request?.number; + commentText = commentText.toLowerCase(); + if (commentText.indexOf("r: @") > -1) { + await manuallyAssignedToReviewer(pullNumber, stateClient); + } else if (commentText.indexOf("assign to next reviewer") > -1) { + await assignToNextReviewer( + payload, + commentAuthor, + pullNumber, + stateClient, + reviewerConfig + ); + } else if (commentText.indexOf("stop reviewer notifications") > -1) { + await stopReviewerNotifications( + pullNumber, + stateClient, + "requested by reviewer" + ); + } else if (commentText.indexOf("remind me after tests pass") > -1) { + await remindAfterTestsPass(pullNumber, commentAuthor, stateClient); + } else if (commentText.indexOf("waiting on author") > -1) { + await waitOnAuthor(payload, pullNumber, stateClient); + } else if (commentText.indexOf("assign set of reviewers") > -1) { + await assignReviewerSet(payload, pullNumber, stateClient, reviewerConfig); + } else { + return false; + } + + return true; +} + +async function assignToNextReviewer( + payload: any, Review comment: `any` => `unknown` ########## File path: scripts/ci/pr-bot/test/reviewerConfigTest.ts ########## @@ -0,0 +1,218 @@ +/* + * 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. + */ + +var assert = require("assert"); +var fs = require("fs"); +const { ReviewerConfig } = require("../shared/reviewerConfig"); +const configPath = "test-config.yml"; +const configContents = `labels: +- name: "Go" + reviewers: ["testReviewer1", "testReviewer2"] + exclusionList: ["testReviewer3"] # 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 +- name: "Java" + reviewers: ["testReviewer3", "testReviewer2"] + exclusionList: [] # These users will never be suggested as reviewers +- name: "Python" + reviewers: ["testReviewer4"] + exclusionList: [] # These users will never be suggested as reviewers +fallbackReviewers: ["testReviewer5", "testReviewer1", "testReviewer3"] # List of committers to use when no label matches +`; +describe("ReviewerConfig", function () { + before(function () { + if (fs.existsSync(configPath)) { + fs.rmSync(configPath); + } + fs.writeFileSync(configPath, configContents); + }); + + after(function () { + fs.rmSync(configPath); + }); + + describe("getReviewersForLabels()", function () { + it("should return all reviewers configured for all labels", function () { + const config = new ReviewerConfig(configPath); + const reviewersForLabels = config.getReviewersForLabels( + [{ name: "Go" }, { name: "Java" }], + [] + ); + assert( + reviewersForLabels["Go"].indexOf("testReviewer1") > -1, Review comment: Using `find` instead of `indexOf` would be cleaner because it returns null which is false. ########## File path: scripts/ci/pr-bot/shared/userCommand.ts ########## @@ -0,0 +1,231 @@ +/* + * 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("./githubUtils"); +const commentStrings = require("./commentStrings"); +const { BOT_NAME } = require("./constants"); +const { StateClient } = require("./persistentState"); +const { ReviewerConfig } = require("./reviewerConfig"); + +// Reads the comment and processes the command if one is contained in it. +// Returns true if it runs a command, false otherwise. +export async function processCommand( + payload: any, + commentAuthor: string, + commentText: string, + stateClient: typeof StateClient, + reviewerConfig: typeof ReviewerConfig +) { + // Don't process any commands from our bot. + if (commentAuthor === BOT_NAME) { + return false; + } + console.log(commentAuthor); + + const pullNumber = payload.issue?.number || payload.pull_request?.number; + commentText = commentText.toLowerCase(); + if (commentText.indexOf("r: @") > -1) { + await manuallyAssignedToReviewer(pullNumber, stateClient); + } else if (commentText.indexOf("assign to next reviewer") > -1) { + await assignToNextReviewer( + payload, + commentAuthor, + pullNumber, + stateClient, + reviewerConfig + ); + } else if (commentText.indexOf("stop reviewer notifications") > -1) { + await stopReviewerNotifications( + pullNumber, + stateClient, + "requested by reviewer" + ); + } else if (commentText.indexOf("remind me after tests pass") > -1) { + await remindAfterTestsPass(pullNumber, commentAuthor, stateClient); + } else if (commentText.indexOf("waiting on author") > -1) { + await waitOnAuthor(payload, pullNumber, stateClient); + } else if (commentText.indexOf("assign set of reviewers") > -1) { + await assignReviewerSet(payload, pullNumber, stateClient, reviewerConfig); + } else { + return false; + } + + return true; +} + +async function assignToNextReviewer( + payload: any, + commentAuthor: string, + pullNumber: number, + stateClient: typeof StateClient, + reviewerConfig: typeof ReviewerConfig +) { + let prState = await stateClient.getPrState(pullNumber); + let labelOfReviewer = prState.getLabelForReviewer(payload.sender.login); + if (labelOfReviewer) { + let reviewersState = await stateClient.getReviewersForLabelState( + labelOfReviewer + ); + const pullAuthor = + payload.issue?.user?.login || payload.pull_request?.user?.login; + let availableReviewers = reviewerConfig.getReviewersForLabel( + labelOfReviewer, + [commentAuthor, pullAuthor] + ); + let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); + prState.reviewersAssignedForLabels[labelOfReviewer] = chosenReviewer; + + // Comment assigning reviewer + console.log(`Assigning ${chosenReviewer}`); + await github.addPrComment( + pullNumber, + commentStrings.assignReviewer(prState.reviewersAssignedForLabels) + ); + + // Set next action to reviewer + const existingLabels = + payload.issue?.labels || payload.pull_request?.labels; + await github.nextActionReviewers(pullNumber, existingLabels); + prState.nextAction = "Reviewers"; + + // Persist state + await stateClient.writePrState(pullNumber, prState); + await stateClient.writeReviewersForLabelState( + labelOfReviewer, + reviewersState + ); + } +} + +// If they've manually assigned a reviewer, just silence notifications and ignore this pr going forward. +// TODO(damccorm) - we could try to do something more intelligent here like figuring out which label that reviewer belongs to. +async function manuallyAssignedToReviewer( + pullNumber: number, + stateClient: typeof StateClient +) { + await stopReviewerNotifications( + pullNumber, + stateClient, + "review requested by someone other than the bot, ceding control" + ); +} + +async function stopReviewerNotifications( + pullNumber: number, + stateClient: typeof StateClient, + reason: string +) { + let prState = await stateClient.getPrState(pullNumber); + prState.stopReviewerNotifications = true; + await stateClient.writePrState(pullNumber, prState); + + // Comment acknowledging command + await github.addPrComment( + pullNumber, + commentStrings.stopNotifications(reason) + ); +} + +async function remindAfterTestsPass( + pullNumber: number, + username: string, + stateClient: typeof StateClient +) { + let prState = await stateClient.getPrState(pullNumber); + prState.remindAfterTestsPass.push(username); + await stateClient.writePrState(pullNumber, prState); + + // Comment acknowledging command + await github.addPrComment( + pullNumber, + commentStrings.remindReviewerAfterTestsPass(username) + ); +} + +async function waitOnAuthor( + payload: any, + pullNumber: number, + stateClient: typeof StateClient +) { + const existingLabels = payload.issue?.labels || payload.pull_request?.labels; + await github.nextActionAuthor(pullNumber, existingLabels); + let prState = await stateClient.getPrState(pullNumber); + prState.nextAction = "Author"; + await stateClient.writePrState(pullNumber, prState); +} + +async function assignReviewerSet( + payload: any, + pullNumber: number, + stateClient: typeof StateClient, + reviewerConfig: typeof ReviewerConfig +) { + let prState = await stateClient.getPrState(pullNumber); + if (Object.values(prState.reviewersAssignedForLabels).length > 0) { + await github.addPrComment( + pullNumber, + commentStrings.reviewersAlreadyAssigned( + Object.values(prState.reviewersAssignedForLabels) + ) + ); + return; + } + + const existingLabels = payload.issue?.labels || payload.pull_request?.labels; + const pullAuthor = + payload.issue?.user?.login || payload.pull_request?.user?.login; + const reviewersForLabels = reviewerConfig.getReviewersForLabels( + existingLabels, + [pullAuthor] + ); + let reviewerStateToUpdate = {}; + var labels = Object.keys(reviewersForLabels); + if (!labels || labels.length == 0) { Review comment: `labels?.length > 0` ########## File path: scripts/ci/pr-bot/shared/reviewersForLabel.ts ########## @@ -0,0 +1,102 @@ +/* + * 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("./githubUtils"); + +export class ReviewersForLabel { + public label: string; + public dateOfLastReviewAssignment: { [key: string]: number }; + + constructor( + label: string, + propertyDictionary: { + dateOfLastReviewAssignment: { [key: string]: number }; + } + ) { + this.label = label; + this.dateOfLastReviewAssignment = {}; // map of reviewer to date + + if (!propertyDictionary) { + return; + } + if ("dateOfLastReviewAssignment" in propertyDictionary) { + this.dateOfLastReviewAssignment = + propertyDictionary["dateOfLastReviewAssignment"]; + } + } + + // Given a list of available reviewers, + // returns the next reviewer up based on who has reviewed least recently. + // Updates this object to reflect their assignment. + assignNextReviewer(availableReviewers: string[]): string { + if (availableReviewers.length == 0) { Review comment: Triple equals for strict number comparison. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
