diff --git a/README.md b/README.md index 94f71f2..a286686 100644 --- a/README.md +++ b/README.md @@ -169,11 +169,12 @@ interface Report { ## Rule configuration file This is the file where all the available rules are written. -**This file is only read from the main branch.** So if you modify the file the changes won’t happen until it is merged into the main branch. -This is done to stop users from modifying the rules in their own PRs. +**This file is only read from the main branch.** So if you modify the file, the changes won’t happen until it is merged into the main branch. +This is done to stop users from modifying the rules in their PRs. -It contains an object called `rules` which has an array of rules. Every rule has a same base structure: +It contains an object called `rules` which has an array of rules. Every rule has a same base structure. There is also a second optional field called `preventReviewRequests`. ```yaml +rules: - name: Rule name condition: include: @@ -181,8 +182,17 @@ It contains an object called `rules` which has an array of rules. Every rule has exclude: - 'README.md' type: the type of the rule + +preventReviewRequests: + users: + - user-a + - user-b + teams: + - team-a + - team-b ``` +#### Rules fields - **name**: Name of the rule. This value must be unique per rule. - **condition**: This is an object that contains two values: - **include**: An array of regex expressions of the files that match this rule. @@ -197,8 +207,16 @@ It contains an object called `rules` which has an array of rules. Every rule has - **or**: Has many review options, requires at least *one option* to be fulfilled. - **and**: Has many review options, requires *all the options* to be fulfilled. - **and-distinct**: Has many review options, requires *all the options* to be fulfilled *by different people*. + +#### preventReviewRequests +This is a special field that applies to all the rules. + +This field is **optional** and currently not used. Pending on https://github.com/paritytech/review-bot/issues/53 + + ### Types Every type has a *slightly* different configuration and works for different scenarios, so let’s analyze all of them. + #### Basic rule As the name implies, this type is elementary. All the files that fall under the rule evaluation must receive a given number of approvals by the listed users and/or team members. diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 3a32c8e..24c307f 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -10,9 +10,10 @@ import { AndRule, BasicRule, ConfigurationFile, DebugRule, Reviewers, Rule, Rule const reviewersObj = { users: Joi.array().items(Joi.string()).optional().empty(null), teams: Joi.array().items(Joi.string()).optional().empty(null), - min_approvals: Joi.number().min(1).default(1), }; +const reviewerConditionObj = { ...reviewersObj, min_approvals: Joi.number().min(1).default(1) }; + /** Base rule condition. * This are the minimum requirements that all the rules must have. * After we evaluated this, we can run a custom evaluation per rule @@ -34,20 +35,20 @@ const ruleSchema = Joi.object().keys({ */ export const generalSchema = Joi.object().keys({ rules: Joi.array().items(ruleSchema).unique("name").required(), - preventReviewRequests: Joi.object().keys(reviewersObj).optional().xor("users", "teams"), + preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"), }); /** Basic rule schema * This rule is quite simple as it only has the min_approvals field and the required reviewers */ export const basicRuleSchema = Joi.object() - .keys({ ...reviewersObj, countAuthor: Joi.boolean().default(false) }) + .keys({ ...reviewerConditionObj, countAuthor: Joi.boolean().default(false) }) .or("users", "teams"); /** As, with the exception of basic, every other schema has the same structure, we can recycle this */ export const otherRulesSchema = Joi.object().keys({ reviewers: Joi.array() - .items(Joi.object().keys(reviewersObj).or("users", "teams")) + .items(Joi.object().keys(reviewerConditionObj).or("users", "teams")) .min(2) .required(), countAuthor: Joi.boolean().default(false), diff --git a/src/runner.ts b/src/runner.ts index 6dbc81a..800bd52 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -67,8 +67,10 @@ export class ActionRunner { * @returns an array of error reports for each failed rule. An empty array means no errors */ async validatePullRequest({ rules }: ConfigurationFile): Promise { - const errorReports: RuleReport[] = []; const modifiedFiles = await this.prApi.listModifiedFiles(); + + const errorReports: RuleReport[] = []; + ruleCheck: for (const rule of rules) { try { this.logger.info(`Validating rule '${rule.name}'`); @@ -146,7 +148,7 @@ export class ActionRunner { } /** WIP - Class that will assign the requests for review */ - requestReviewers(reports: RuleReport[]): void { + requestReviewers(reports: RuleReport[], preventReviewRequests: ConfigurationFile["preventReviewRequests"]): void { if (reports.length === 0) { return; } @@ -159,7 +161,29 @@ export class ActionRunner { finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest); } - const { teamsToRequest, usersToRequest } = finalReport; + let { teamsToRequest, usersToRequest } = finalReport; + + /** + * Evaluates if the user belongs to the special rule of preventReviewRequests + * and if the request for a review should be skipped + */ + if (preventReviewRequests) { + if ( + preventReviewRequests.teams && + teamsToRequest?.some((team) => preventReviewRequests.teams?.indexOf(team) !== -1) + ) { + this.logger.info("Filtering teams to request a review from."); + teamsToRequest = teamsToRequest?.filter((team) => preventReviewRequests.teams?.indexOf(team) === -1); + } + if ( + preventReviewRequests.users && + usersToRequest?.some((user) => preventReviewRequests.users?.indexOf(user) !== -1) + ) { + this.logger.info("Filtering users to request a review from."); + usersToRequest = usersToRequest?.filter((user) => preventReviewRequests.users?.indexOf(user) === -1); + } + } + const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0; const reviewersLog = [ validArray(teamsToRequest) ? `Teams: ${JSON.stringify(teamsToRequest)}` : "", @@ -448,7 +472,7 @@ export class ActionRunner { const checkRunData = this.generateCheckRunData(reports); await this.checks.generateCheckRun(checkRunData); - this.requestReviewers(reports); + this.requestReviewers(reports, config.preventReviewRequests); setOutput("report", JSON.stringify(prValidation)); diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index d3e50fd..ee13322 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -169,7 +169,7 @@ describe("Config Parsing", () => { expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]); }); - test("should fail with both users and teams", async () => { + test("should get both users and teams", async () => { api.getConfigFile.mockResolvedValue(` rules: - name: Default review @@ -187,12 +187,12 @@ describe("Config Parsing", () => { - user-a - user-b teams: - - team-a - - team-b + - team-a + - team-b `); - await expect(runner.getConfigFile("")).rejects.toThrowError( - '"preventReviewRequests" contains a conflict between exclusive peers [users, teams]', - ); + const config = await runner.getConfigFile(""); + expect(config.preventReviewRequests?.users).toEqual(["user-a", "user-b"]); + expect(config.preventReviewRequests?.teams).toEqual(["team-a", "team-b"]); }); test("should pass if preventReviewRequests is not assigned", async () => { diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index b846f20..35619e2 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/unbound-method */ import { mock, MockProxy } from "jest-mock-extended"; import { GitHubChecksApi } from "../../github/check"; @@ -10,10 +11,13 @@ import { ActionRunner } from "../../runner"; describe("Shared validations", () => { let api: MockProxy; let teamsApi: MockProxy; + let logger: MockProxy; let runner: ActionRunner; beforeEach(() => { api = mock(); - runner = new ActionRunner(api, teamsApi, mock(), mock()); + logger = mock(); + teamsApi = mock(); + runner = new ActionRunner(api, teamsApi, mock(), logger); }); test("validatePullRequest should return true if no rule matches any files", async () => { @@ -59,4 +63,40 @@ describe("Shared validations", () => { expect(result).not.toContain(".github/workflows/review-bot.yml"); }); }); + + describe("Validation in requestReviewers", () => { + const exampleReport = { + name: "Example", + missingUsers: ["user-1", "user-2", "user-3"], + missingReviews: 2, + teamsToRequest: ["team-1"], + usersToRequest: ["user-1"], + }; + + test("should request reviewers if object is not defined", () => { + runner.requestReviewers([exampleReport], undefined); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"]))); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"]))); + }); + + test("should not request user if he is defined", () => { + runner.requestReviewers([exampleReport], { users: ["user-1"] }); + expect(logger.info).toHaveBeenCalledWith("Filtering users to request a review from."); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"]))); + expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"]))); + }); + + test("should not request team if it is defined", () => { + runner.requestReviewers([exampleReport], { teams: ["team-1"] }); + expect(logger.info).toHaveBeenCalledWith("Filtering teams to request a review from."); + expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"]))); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"]))); + }); + + test("should request reviewers if the team and user are not the same", () => { + runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] }); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"]))); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"]))); + }); + }); });