diff --git a/.github/REVIEWERS.yml b/.github/REVIEWERS.yml index f3c27bec3cf9..90034a624f16 100644 --- a/.github/REVIEWERS.yml +++ b/.github/REVIEWERS.yml @@ -17,14 +17,18 @@ # 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. +# Priority determines which label is used to select a reviewer when a PR has multiple labels +# (lower number = higher priority). labels: - name: Go + priority: 2 reviewers: - jrmccluskey - lostluck - shunping exclusionList: [] - name: Python + priority: 2 reviewers: - claudevdm - damccorm @@ -33,6 +37,7 @@ labels: - shunping exclusionList: [] - name: Java + priority: 2 reviewers: - Abacn - ahmedabu98 @@ -41,28 +46,34 @@ labels: - kennknowles exclusionList: [] - name: spanner + priority: 1 reviewers: - nielm - name: bigtable + priority: 1 reviewers: - igorbernstein2 - mutianf exclusionList: [] - name: delta + priority: 1 reviewers: - ahmedabu98 - chamikaramj exclusionList: [] - name: iceberg + priority: 1 reviewers: - ahmedabu98 - chamikaramj exclusionList: [] - name: yaml + priority: 1 reviewers: - chamikaramj - derrickaw - name: Build + priority: 3 reviewers: - Abacn - Amar3tto @@ -70,6 +81,7 @@ labels: - derrickaw exclusionList: [] - name: website + priority: 3 reviewers: - Abacn - damccorm diff --git a/scripts/ci/pr-bot/findPrsNeedingAttention.ts b/scripts/ci/pr-bot/findPrsNeedingAttention.ts index 4b1d97201f9d..829a69d6d59b 100644 --- a/scripts/ci/pr-bot/findPrsNeedingAttention.ts +++ b/scripts/ci/pr-bot/findPrsNeedingAttention.ts @@ -112,9 +112,11 @@ async function assignToNewReviewers( reviewersToExclude.push(pull.user.login); const reviewersForLabels: { [key: string]: string[] } = reviewerConfig.getReviewersForLabels(labelObjects, reviewersToExclude); - const fallbackReviewers = reviewerConfig.getFallbackReviewers(); - for (const labelObject of labelObjects) { - const label = labelObject.name; + const fallbackReviewers = + reviewerConfig.getFallbackReviewers(reviewersToExclude); + let backupReviewersForLabels: { [key: string]: string } = {}; + prState.reviewersAssignedForLabels = {}; + for (const label of Object.keys(reviewersForLabels)) { let availableReviewers = reviewersForLabels[label]; if (availableReviewers && availableReviewers.length > 0) { let reviewersState = await stateClient.getReviewersForLabelState(label); @@ -122,8 +124,16 @@ async function assignToNewReviewers( availableReviewers, fallbackReviewers ); + let backupReviewer = reviewersState.getBackupReviewer( + availableReviewers, + chosenReviewer, + fallbackReviewers + ); reviewerStateToUpdate[label] = reviewersState; prState.reviewersAssignedForLabels[label] = chosenReviewer; + if (backupReviewer) { + backupReviewersForLabels[label] = backupReviewer; + } } } @@ -132,6 +142,7 @@ async function assignToNewReviewers( pull.number, commentStrings.assignNewReviewer(prState.reviewersAssignedForLabels, { labels: pull.labels, + backupReviewers: backupReviewersForLabels, }) ); await github.requestPrReviewers( diff --git a/scripts/ci/pr-bot/processNewPrs.ts b/scripts/ci/pr-bot/processNewPrs.ts index 2f24f8e08f45..f027b75f4fa2 100644 --- a/scripts/ci/pr-bot/processNewPrs.ts +++ b/scripts/ci/pr-bot/processNewPrs.ts @@ -344,8 +344,11 @@ export async function processPull( // Pick reviewers to assign. Store them in reviewerStateToUpdate and update the prState object with those reviewers (and their associated labels) let reviewerStateToUpdate: { [key: string]: typeof ReviewersForLabel } = {}; + let backupReviewersForLabels: { [key: string]: string } = {}; + const exclusionList = [pull.user.login]; const reviewersForLabels: { [key: string]: string[] } = - reviewerConfig.getReviewersForLabels(pull.labels, [pull.user.login]); + reviewerConfig.getReviewersForLabels(pull.labels, exclusionList); + const fallbackReviewers = reviewerConfig.getFallbackReviewers(exclusionList); var labels = Object.keys(reviewersForLabels); if (!labels || labels.length === 0) { return; @@ -354,8 +357,16 @@ export async function processPull( let availableReviewers = reviewersForLabels[label]; let reviewersState = await stateClient.getReviewersForLabelState(label); let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); + let backupReviewer = reviewersState.getBackupReviewer( + availableReviewers, + chosenReviewer, + fallbackReviewers + ); reviewerStateToUpdate[label] = reviewersState; prState.reviewersAssignedForLabels[label] = chosenReviewer; + if (backupReviewer) { + backupReviewersForLabels[label] = backupReviewer; + } } console.log(`Assigning reviewers for PR ${pull.number}`); @@ -363,6 +374,7 @@ export async function processPull( pull.number, commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { labels: pull.labels, + backupReviewers: backupReviewersForLabels, }) ); await github.requestPrReviewers( diff --git a/scripts/ci/pr-bot/shared/commentStrings.ts b/scripts/ci/pr-bot/shared/commentStrings.ts index f1a39be4c55e..edf16d27b839 100644 --- a/scripts/ci/pr-bot/shared/commentStrings.ts +++ b/scripts/ci/pr-bot/shared/commentStrings.ts @@ -22,6 +22,7 @@ const { NO_MATCHING_LABEL } = require("./constants"); export interface AssignReviewerOptions { labels?: (string | Label)[]; notices?: string[]; + backupReviewers?: { [label: string]: string }; } // Custom notices for specific labels @@ -63,11 +64,15 @@ export function assignReviewer( for (let label in labelToReviewerMapping) { let reviewer = labelToReviewerMapping[label]; + let backupReviewer = options?.backupReviewers?.[label]; if (label === NO_MATCHING_LABEL) { commentString += `R: @${reviewer} added as fallback since no labels match configuration\n`; } else { commentString += `R: @${reviewer} for label ${label}.\n`; } + if (backupReviewer) { + commentString += `Backup reviewer: ${backupReviewer} (if the main reviewer is unresponsive, you can escalate to them by tagging their username)\n`; + } } commentString += formatNotices(labelToReviewerMapping, options); @@ -158,11 +163,15 @@ export function assignNewReviewer( for (const label in labelToReviewerMapping) { const reviewer = labelToReviewerMapping[label]; + const backupReviewer = options?.backupReviewers?.[label]; if (label === NO_MATCHING_LABEL) { commentString += `R: @${reviewer} added as fallback since no labels match configuration\n`; } else { commentString += `R: @${reviewer} for label ${label}.\n`; } + if (backupReviewer) { + commentString += `Backup reviewer: ${backupReviewer} (if the main reviewer is unresponsive, you can escalate to them by tagging their username)\n`; + } } commentString += formatNotices(labelToReviewerMapping, options); diff --git a/scripts/ci/pr-bot/shared/reviewerConfig.ts b/scripts/ci/pr-bot/shared/reviewerConfig.ts index 8fc0d32b014b..8feb7c2df6e4 100644 --- a/scripts/ci/pr-bot/shared/reviewerConfig.ts +++ b/scripts/ci/pr-bot/shared/reviewerConfig.ts @@ -44,30 +44,52 @@ export class ReviewerConfig { } // 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 + // returns all possible reviewers for the single highest-priority matching label + // (lower priority number = higher priority; ties broken randomly). getReviewersForLabels( labels: Label[], exclusionList: string[] ): { [key: string]: string[] } { - let reviewersFound = false; - let labelToReviewerMapping = {}; - labels.forEach((label) => { + let labelToReviewerMapping: { [key: string]: string[] } = {}; + let bestPriority: number = Number.MAX_SAFE_INTEGER; + let bestMatches: { label: string; reviewers: string[] }[] = []; + + for (const label of labels || []) { let reviewers = this.getReviewersForLabel(label.name, exclusionList); if (reviewers.length > 0) { - labelToReviewerMapping[label.name] = reviewers; - reviewersFound = true; + const priority = this.getPriorityForLabel(label.name); + if (bestMatches.length === 0 || priority < bestPriority) { + bestPriority = priority; + bestMatches = [{ label: label.name, reviewers }]; + } else if (priority === bestPriority) { + bestMatches.push({ label: label.name, reviewers }); + } } - }); - if (!reviewersFound) { + } + if (bestMatches.length > 0) { + const chosenMatch = + bestMatches[Math.floor(Math.random() * bestMatches.length)]; + labelToReviewerMapping[chosenMatch.label] = chosenMatch.reviewers; + } else { const fallbackReviewers = this.getFallbackReviewers(exclusionList); if (fallbackReviewers.length > 0) { - labelToReviewerMapping[NO_MATCHING_LABEL] = - this.getFallbackReviewers(exclusionList); + labelToReviewerMapping[NO_MATCHING_LABEL] = fallbackReviewers; } } return labelToReviewerMapping; } + getPriorityForLabel(label: string): number { + const labelObjects = this.config.labels; + const labelObject = labelObjects.find( + (labelObject) => labelObject.name.toLowerCase() === label.toLowerCase() + ); + if (!labelObject || typeof labelObject.priority !== "number") { + return Number.MAX_SAFE_INTEGER; + } + return labelObject.priority; + } + // Get possible reviewers excluding the author. getReviewersForLabel(label: string, exclusionList: string[]): string[] { const labelObjects = this.config.labels; @@ -106,6 +128,8 @@ export class ReviewerConfig { # 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. +# Priority determines which label is used to select a reviewer when a PR has multiple labels +# (lower number = higher priority). ${yaml.dump(this.config)}`; fs.writeFileSync(this.configPath, contents, { encoding: "utf-8" }); diff --git a/scripts/ci/pr-bot/shared/reviewersForLabel.ts b/scripts/ci/pr-bot/shared/reviewersForLabel.ts index f46f5359a8b9..40b0a6c2a576 100644 --- a/scripts/ci/pr-bot/shared/reviewersForLabel.ts +++ b/scripts/ci/pr-bot/shared/reviewersForLabel.ts @@ -40,16 +40,14 @@ export class ReviewersForLabel { } } - // 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}`); + private findLeastRecentlyAssignedReviewer( + availableReviewers: string[] + ): string { + if (!availableReviewers || availableReviewers.length === 0) { + return ""; } if (!this.dateOfLastReviewAssignment[availableReviewers[0]]) { - this.dateOfLastReviewAssignment[availableReviewers[0]] = Date.now(); return availableReviewers[0]; } @@ -59,7 +57,6 @@ export class ReviewersForLabel { 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]) { @@ -68,10 +65,44 @@ export class ReviewersForLabel { } } - this.dateOfLastReviewAssignment[earliestReviewer] = Date.now(); return earliestReviewer; } + // 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}`); + } + + const chosenReviewer = + this.findLeastRecentlyAssignedReviewer(availableReviewers); + this.dateOfLastReviewAssignment[chosenReviewer] = Date.now(); + return chosenReviewer; + } + + // Returns a backup reviewer from the same label (excluding the main reviewer) + // without updating dateOfLastReviewAssignment. Falls back to fallbackReviewers + // if the label has no other eligible reviewers. + getBackupReviewer( + availableReviewers: string[], + mainReviewer: string, + fallbackReviewers: string[] = [] + ): string { + const remainingLabelReviewers = (availableReviewers || []).filter( + (reviewer) => reviewer !== mainReviewer + ); + if (remainingLabelReviewers.length > 0) { + return this.findLeastRecentlyAssignedReviewer(remainingLabelReviewers); + } + + const remainingFallbackReviewers = (fallbackReviewers || []).filter( + (reviewer) => reviewer !== mainReviewer + ); + return this.findLeastRecentlyAssignedReviewer(remainingFallbackReviewers); + } + // 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. diff --git a/scripts/ci/pr-bot/shared/userCommand.ts b/scripts/ci/pr-bot/shared/userCommand.ts index 8c09013553e8..79852c475f48 100644 --- a/scripts/ci/pr-bot/shared/userCommand.ts +++ b/scripts/ci/pr-bot/shared/userCommand.ts @@ -92,12 +92,23 @@ async function assignToNextReviewer( labelOfReviewer ); const pullAuthor = github.getPullAuthorFromPayload(payload); + const exclusionList = [commentAuthor, pullAuthor]; let availableReviewers = reviewerConfig.getReviewersForLabel( labelOfReviewer, - [commentAuthor, pullAuthor] + exclusionList ); + const fallbackReviewers = + reviewerConfig.getFallbackReviewers(exclusionList); let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); + let backupReviewer = reviewersState.getBackupReviewer( + availableReviewers, + chosenReviewer, + fallbackReviewers + ); prState.reviewersAssignedForLabels[labelOfReviewer] = chosenReviewer; + const backupReviewersForLabels: { [key: string]: string } = backupReviewer + ? { [labelOfReviewer]: backupReviewer } + : {}; // Comment assigning reviewer console.log(`Assigning ${chosenReviewer}`); @@ -107,6 +118,7 @@ async function assignToNextReviewer( pullNumber, commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { labels: existingLabels, + backupReviewers: backupReviewersForLabels, }) ); await github.requestPrReviewers(pullNumber, [chosenReviewer]); @@ -206,11 +218,14 @@ async function assignReviewerSet( const existingLabels = payload.issue?.labels || payload.pull_request?.labels; const pullAuthor = github.getPullAuthorFromPayload(payload); + const exclusionList = [pullAuthor]; const reviewersForLabels = reviewerConfig.getReviewersForLabels( existingLabels, - [pullAuthor] + exclusionList ); + const fallbackReviewers = reviewerConfig.getFallbackReviewers(exclusionList); let reviewerStateToUpdate = {}; + let backupReviewersForLabels: { [key: string]: string } = {}; var labels = Object.keys(reviewersForLabels); if (!labels || labels.length == 0) { await github.addPrComment( @@ -224,14 +239,23 @@ async function assignReviewerSet( let availableReviewers = reviewersForLabels[label]; let reviewersState = await stateClient.getReviewersForLabelState(label); let chosenReviewer = reviewersState.assignNextReviewer(availableReviewers); + let backupReviewer = reviewersState.getBackupReviewer( + availableReviewers, + chosenReviewer, + fallbackReviewers + ); reviewerStateToUpdate[label] = reviewersState; prState.reviewersAssignedForLabels[label] = chosenReviewer; + if (backupReviewer) { + backupReviewersForLabels[label] = backupReviewer; + } } console.log(`Assigning reviewers for pr ${pullNumber}`); await github.addPrComment( pullNumber, commentStrings.assignReviewer(prState.reviewersAssignedForLabels, { labels: existingLabels, + backupReviewers: backupReviewersForLabels, }) ); await github.requestPrReviewers( diff --git a/scripts/ci/pr-bot/test/commentStringsTest.ts b/scripts/ci/pr-bot/test/commentStringsTest.ts index f2a16ca1c634..62456fd49944 100644 --- a/scripts/ci/pr-bot/test/commentStringsTest.ts +++ b/scripts/ci/pr-bot/test/commentStringsTest.ts @@ -41,5 +41,18 @@ describe("commentStrings", function () { const comment = commentStrings.assignReviewer({ core: "reviewer1" }); assert(comment.includes("review with scrutiny")); }); + + it("should mention backup reviewer without tagging them with @", function () { + const comment = commentStrings.assignReviewer( + { Java: "mainReviewer" }, + { + labels: [{ name: "Java" }], + backupReviewers: { Java: "backupUser" }, + } + ); + assert(comment.includes("R: @mainReviewer for label Java.")); + assert(comment.includes("Backup reviewer: backupUser")); + assert(!comment.includes("@backupUser")); + }); }); }); diff --git a/scripts/ci/pr-bot/test/reviewerConfigTest.ts b/scripts/ci/pr-bot/test/reviewerConfigTest.ts index 589aab635bc9..95c39188824f 100644 --- a/scripts/ci/pr-bot/test/reviewerConfigTest.ts +++ b/scripts/ci/pr-bot/test/reviewerConfigTest.ts @@ -22,15 +22,22 @@ const { ReviewerConfig } = require("../shared/reviewerConfig"); const configPath = "test-config.yml"; const configContents = `labels: - name: "Go" + priority: 2 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" + priority: 3 reviewers: ["testReviewer3", "testReviewer2"] exclusionList: [] # These users will never be suggested as reviewers - name: "Python" + priority: 1 reviewers: ["testReviewer4"] exclusionList: [] # These users will never be suggested as reviewers +- name: "Kafka" + priority: 1 + reviewers: ["testReviewer5"] + exclusionList: [] fallbackReviewers: ["testReviewer5", "testReviewer1", "testReviewer3"] # List of committers to use when no label matches `; describe("ReviewerConfig", function () { @@ -46,12 +53,13 @@ describe("ReviewerConfig", function () { }); describe("getReviewersForLabels()", function () { - it("should return all reviewers configured for all labels", function () { + it("should return reviewers only for the highest-priority matching label", function () { const config = new ReviewerConfig(configPath); const reviewersForLabels = config.getReviewersForLabels( - [{ name: "Go" }, { name: "Java" }], + [{ name: "Java" }, { name: "Go" }], [] ); + assert.deepEqual(Object.keys(reviewersForLabels), ["Go"]); assert( reviewersForLabels["Go"].find( (reviewer) => reviewer === "testReviewer1" @@ -76,44 +84,51 @@ describe("ReviewerConfig", function () { ), "Return value for Go label should not include testReviewer4" ); - - assert( - reviewersForLabels["Java"].find( - (reviewer) => reviewer === "testReviewer3" - ), - "Return value for Java label should include testReviewer3" - ); - assert( - reviewersForLabels["Java"].find( - (reviewer) => reviewer === "testReviewer2" - ), - "Return value for Java label should include testReviewer2" - ); - assert( - !reviewersForLabels["Java"].find( - (reviewer) => reviewer === "testReviewer4" - ), - "Return value for Java label should not include testReviewer4" - ); - assert( - !reviewersForLabels["Java"].find( - (reviewer) => reviewer === "testReviewer1" - ), - "Return value for Java label should not include testReviewer1" - ); assert( - !reviewersForLabels["Java"].find( - (reviewer) => reviewer === "testReviewer5" - ), - "Return value for Java label should not include testReviewer5" + Object.keys(reviewersForLabels).indexOf("Java") == -1, + "Lower-priority Java label should not be included when Go matches" ); - assert( Object.keys(reviewersForLabels).indexOf("Python") == -1, "No reviewers should be included for python" ); }); + it("should choose randomly when multiple labels share the highest priority", function () { + const config = new ReviewerConfig(configPath); + const origRandom = Math.random; + try { + Math.random = () => 0.1; + const firstChoice = config.getReviewersForLabels( + [{ name: "Python" }, { name: "Kafka" }, { name: "Java" }], + [] + ); + assert.deepEqual(Object.keys(firstChoice), ["Python"]); + + Math.random = () => 0.9; + const secondChoice = config.getReviewersForLabels( + [{ name: "Python" }, { name: "Kafka" }, { name: "Java" }], + [] + ); + assert.deepEqual(Object.keys(secondChoice), ["Kafka"]); + } finally { + Math.random = origRandom; + } + }); + + it("should fall back to next highest-priority label if top priority label has all reviewers excluded", function () { + const config = new ReviewerConfig(configPath); + const reviewersForLabels = config.getReviewersForLabels( + [{ name: "Python" }, { name: "Java" }], + ["testReviewer4"] + ); + assert.deepEqual(Object.keys(reviewersForLabels), ["Java"]); + assert.deepEqual(reviewersForLabels["Java"], [ + "testReviewer3", + "testReviewer2", + ]); + }); + it("should return no entry if a label is not configured", function () { const config = new ReviewerConfig(configPath); const reviewersForLabels = config.getReviewersForLabels( diff --git a/scripts/ci/pr-bot/test/reviewersForLabelTest.ts b/scripts/ci/pr-bot/test/reviewersForLabelTest.ts index f42575050592..899b0519c1f2 100644 --- a/scripts/ci/pr-bot/test/reviewersForLabelTest.ts +++ b/scripts/ci/pr-bot/test/reviewersForLabelTest.ts @@ -132,4 +132,50 @@ describe("ReviewersForLabel", function () { assert.throws(() => reviewersForGo.assignNextReviewer([])); }); }); + + describe("getBackupReviewer()", function () { + it("should pick the next least recently assigned reviewer from the label without updating timestamps", function () { + const dateOfLastReviewAssignment = { + testReviewer1: 10, + testReviewer2: 20, + testReviewer3: 30, + }; + let reviewersForGo = new ReviewersForLabel("Go", { + dateOfLastReviewAssignment: { ...dateOfLastReviewAssignment }, + }); + + const backup = reviewersForGo.getBackupReviewer( + ["testReviewer1", "testReviewer2", "testReviewer3"], + "testReviewer1", + ["fallback1"] + ); + assert.equal(backup, "testReviewer2"); + assert.deepEqual( + reviewersForGo.dateOfLastReviewAssignment, + dateOfLastReviewAssignment + ); + }); + + it("should fall back to fallbackReviewers when label only has one available reviewer", function () { + const dateOfLastReviewAssignment = { + testReviewer1: 10, + fallback1: 50, + fallback2: 15, + }; + let reviewersForSpanner = new ReviewersForLabel("spanner", { + dateOfLastReviewAssignment: { ...dateOfLastReviewAssignment }, + }); + + const backup = reviewersForSpanner.getBackupReviewer( + ["testReviewer1"], + "testReviewer1", + ["testReviewer1", "fallback1", "fallback2"] + ); + assert.equal(backup, "fallback2"); + assert.deepEqual( + reviewersForSpanner.dateOfLastReviewAssignment, + dateOfLastReviewAssignment + ); + }); + }); });