From 4e7cc4190e522373f3815676312d57ed1d63bb29 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 12:27:07 +0100 Subject: [PATCH] implemented action on itself (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves #6 ## Changes - Fixed validation of rules not setting default values. - Fixed basic rule amount of review to be a positive integer (min value 1) - Added a test to verify this - Implemented GitHub Action to run on Pull Requests. - Added a log which reports what teams and users are missing. - Fixed a bug where the approvals wasn’t being properly filtered. - Fixed a bug where the system asked the author to review the PR - Fixed the wrong default name of the config file - Was set to `review.yml` instead of `review-bot.yml` - Changed rule type validation to have forced string values. - Added logs which inform when a rule is running and if it fails. - Rules are executed in order, so the logs will always correspond to the current rule. - Created tickets #32, #33 and #34 as foreseeable problems. - Created utility method to concatenate two arrays and removing duplicates. - @mutantcornholio I don’t know if `EngAutomation/js` has this (or if we want to add it to it) - Fixed a bug where the users who had approved the PR was not caching. - Fixed a glitch where the required reviews could be 0 or a negative number. - Fixed a glitch where the rule type could be invalid. --- .github/workflows/review-bot.yml | 24 +++++++++++++ action.yml | 2 +- src/file/validator.ts | 14 ++++---- src/github/pullRequest.ts | 12 ++++++- src/github/teams.ts | 10 +++--- src/runner.ts | 46 +++++++++++++++++++------ src/test/runner/basicRule.test.ts | 56 +++++++++++++++++++++++++++++++ src/test/runner/config.test.ts | 37 ++++++++++++++++++++ 8 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 .github/workflows/review-bot.yml diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml new file mode 100644 index 0000000..279bbae --- /dev/null +++ b/.github/workflows/review-bot.yml @@ -0,0 +1,24 @@ +name: Review PR +on: + pull_request: + types: + - opened + - reopened + - synchronize + - review_requested + - review_request_removed + - ready_for_review + pull_request_review: + +permissions: + contents: read + +jobs: + review-approvals: + runs-on: ubuntu-latest + steps: + # TODO: Set this to main + - uses: paritytech/review-bot@self-implementation + with: + repo-token: ${{ secrets.GITHUB_TOKEN }} + team-token: ${{ secrets.TEST_TEAM_TOKEN }} diff --git a/action.yml b/action.yml index cadbd84..ce4e77e 100644 --- a/action.yml +++ b/action.yml @@ -14,7 +14,7 @@ inputs: config-file: description: 'Location of the configuration file' required: false - default: '.github/review.yml' + default: '.github/review-bot.yml' outputs: repo: description: 'The name of the repo in owner/repo pattern' diff --git a/src/file/validator.ts b/src/file/validator.ts index 551b758..2828cf5 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -2,7 +2,7 @@ import { validate } from "@eng-automation/js"; import Joi from "joi"; import { ActionLogger } from "../github/types"; -import { BasicRule, ConfigurationFile, Rule } from "./types"; +import { BasicRule, ConfigurationFile, DebugRule, Rule, RuleTypes } from "./types"; /** For the users or team schema. Will be recycled A LOT * Remember to add `.or("users", "teams")` to force at least one of the two to be defined @@ -22,7 +22,7 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - type: Joi.string().required(), + type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug).required(), }); /** General Configuration schema. @@ -38,7 +38,7 @@ export const generalSchema = Joi.object().keys({ * This rule is quite simple as it only has the min_approvals field and the required reviewers */ export const basicRuleSchema = Joi.object() - .keys({ min_approvals: Joi.number().empty(1), ...reviewersObj }) + .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj }) .or("users", "teams"); /** @@ -54,19 +54,19 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n message: "Configuration file is invalid", }); - for (const rule of validatedConfig.rules) { + for (let i = 0; i < validatedConfig.rules.length; i++) { + const rule = validatedConfig.rules[i]; const { name, type } = rule; const message = `Configuration for rule '${rule.name}' is invalid`; if (type === "basic") { - validate(rule, basicRuleSchema, { message }); + validatedConfig.rules[i] = validate(rule, basicRuleSchema, { message }); } else if (type === "debug") { - validate(rule, ruleSchema, { message }); + validatedConfig.rules[i] = validate(rule, ruleSchema, { message }); } else { // eslint-disable-next-line @typescript-eslint/restrict-template-expressions throw new Error(`Rule ${name} has an invalid type: ${type}`); } } - return validatedConfig; }; diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 02973b9..bba1e7b 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -20,6 +20,7 @@ export class PullRequestApi { private usersThatApprovedThePr: string[] | null = null; async getConfigFile(configFilePath: string): Promise { + this.logger.info(`Fetching config file in ${configFilePath}`); const { data } = await this.api.rest.repos.getContent({ owner: this.pr.base.repo.owner.login, repo: this.pr.base.repo.name, @@ -53,9 +54,18 @@ export class PullRequestApi { if (!this.usersThatApprovedThePr) { const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number }); const reviews = request.data as PullRequestReview[]; - const approvals = reviews.filter((review) => review.state === "approved"); + this.logger.debug(`List of reviews: ${JSON.stringify(reviews)}`); + const approvals = reviews.filter( + (review) => review.state.localeCompare("approved", undefined, { sensitivity: "accent" }) === 0, + ); this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); } + this.logger.debug(`PR approvals are ${JSON.stringify(this.usersThatApprovedThePr)}`); return this.usersThatApprovedThePr; } + + /** Returns the login of the PR's author */ + getAuthor(): string { + return this.pr.user.login; + } } diff --git a/src/github/teams.ts b/src/github/teams.ts index 49d0596..4eb537a 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -31,10 +31,12 @@ export class GitHubTeamsApi implements TeamApi { async getTeamMembers(teamName: string): Promise { // We first verify that this information hasn't been fetched yet - if (this.teamsCache.has(teamName)) { - return this.teamsCache.get(teamName) as string[]; + if (!this.teamsCache.has(teamName)) { + this.logger.debug(`Fetching team '${teamName}'`); + const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); + const members = data.map((d) => d.login); + this.teamsCache.set(teamName, members); } - const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); - return data.map((d) => d.login); + return this.teamsCache.get(teamName) as string[]; } } diff --git a/src/runner.ts b/src/runner.ts index 9959e3e..7e97f60 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -54,13 +54,15 @@ export class ActionRunner { * @returns a true/false statement if the rule failed. This WILL BE CHANGED for an object with information (see issue #26) */ async validatePullRequest({ rules }: ConfigurationFile): Promise { + const errorReports: ReviewReport[] = []; for (const rule of rules) { try { + this.logger.info(`Validating rule '${rule.name}'`); // We get all the files that were modified and match the rules condition const files = await this.listFilesThatMatchRuleCondition(rule); // We check if there are any matches if (files.length === 0) { - this.logger.debug(`Skipping rule ${rule.name} as no condition matched`); + this.logger.info(`Skipping rule ${rule.name} as no condition matched`); // If there are no matches, we simply skip the check continue; } @@ -68,38 +70,55 @@ export class ActionRunner { const [result, missingData] = await this.evaluateCondition(rule); if (!result) { this.logger.error(`Missing the reviews from ${JSON.stringify(missingData.missingUsers)}`); - return false; + errorReports.push(missingData); } } } catch (error: unknown) { // We only throw if there was an unexpected error, not if the check fails - this.logger.error(`Rule ${rule.name} failed with error`); + this.logger.error(`Rule '${rule.name}' failed with error`); throw error; } + this.logger.info(`Finish validating '${rule.name}'`); + } + if (errorReports.length > 0) { + const finalReport = this.aggregateReports(errorReports); + // Preview, this will be improved in a future commit + this.logger.warn(`Missing reviews: ${JSON.stringify(finalReport)}`); + return false; } - // TODO: Convert this into a list of users/teams missing and convert the output into a nice summary object -> Issue #26 return true; } + /** Aggregates all the reports and generate a final combined one */ + aggregateReports(reports: ReviewReport[]): ReviewReport { + const finalReport: ReviewReport = { missingReviews: 0, missingUsers: [], teamsToRequest: [], usersToRequest: [] }; + + for (const report of reports) { + finalReport.missingReviews += report.missingReviews; + finalReport.missingUsers = concatArraysUniquely(finalReport.missingUsers, report.missingUsers); + finalReport.teamsToRequest = concatArraysUniquely(finalReport.teamsToRequest, report.teamsToRequest); + finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest); + } + + return finalReport; + } + /** Evaluates if the required reviews for a condition have been meet * @param rule Every rule check has this values which consist on the min required approvals and the reviewers. * @returns a [bool, error data] tuple which evaluates if the condition (not the rule itself) has fulfilled the requirements * @see-also ReviewError */ async evaluateCondition(rule: { min_approvals: number } & Reviewers): Promise { + this.logger.debug(JSON.stringify(rule)); + // This is a list of all the users that need to approve a PR let requiredUsers: string[] = []; // If team is set, we fetch the members of such team if (rule.teams) { for (const team of rule.teams) { const members = await this.teamApi.getTeamMembers(team); - for (const member of members) { - // simple check to stop us from having duplicates - if (requiredUsers.indexOf(member) < 0) { - requiredUsers.push(member); - } - } + requiredUsers = concatArraysUniquely(requiredUsers, members); } // If, instead, users are set, we simply push them to the array as we don't need to scan a team } @@ -125,6 +144,7 @@ export class ActionRunner { // We get the list of users that approved the PR const approvals = await this.prApi.listApprovedReviewsAuthors(); + this.logger.info(`Found ${approvals.length} approvals.`); // This is the amount of reviews required. To succeed this should be 0 or lower let missingReviews = rule.min_approvals; @@ -137,18 +157,22 @@ export class ActionRunner { // Now we verify if we have any remaining missing review. if (missingReviews > 0) { + const author = this.prApi.getAuthor(); + this.logger.warn(`${missingReviews} reviews are missing.`); // If we have at least one missing review, we return an object with the list of missing reviewers, and // which users/teams we should request to review return [ false, { missingReviews, - missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0), + // Remove all the users who approved the PR + the author (if he belongs to the group) + missingUsers: requiredUsers.filter((u) => approvals.indexOf(u) < 0).filter((u) => u !== author), teamsToRequest: rule.teams ? rule.teams : undefined, usersToRequest: rule.users ? rule.users.filter((u) => approvals.indexOf(u)) : undefined, }, ]; } else { + this.logger.info("Rule requirements fulfilled"); // If we don't have any missing reviews, we return the succesful case return [true]; } diff --git a/src/test/runner/basicRule.test.ts b/src/test/runner/basicRule.test.ts index 87852e7..6ed5221 100644 --- a/src/test/runner/basicRule.test.ts +++ b/src/test/runner/basicRule.test.ts @@ -87,4 +87,60 @@ describe("Basic rule parsing", () => { `); await expect(runner.getConfigFile("")).rejects.toThrowError('"value" must contain at least one of [users, teams]'); }); + + test("should default min_approvals to 1", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + users: + - user-example + `); + const config = await runner.getConfigFile(""); + const [rule] = config.rules; + if (rule.type === "basic") { + expect(rule.min_approvals).toEqual(1); + } else { + throw new Error(`Rule type ${rule.type} is invalid`); + } + }); + + test("should fail with min_approvals in negative", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + min_approvals: -99 + users: + - user-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1'); + }); + + test("should fail with min_approvals in 0", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + min_approvals: 0 + users: + - user-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"min_approvals" must be greater than or equal to 1'); + }); }); diff --git a/src/test/runner/config.test.ts b/src/test/runner/config.test.ts index db2eb33..a152d31 100644 --- a/src/test/runner/config.test.ts +++ b/src/test/runner/config.test.ts @@ -40,6 +40,43 @@ describe("Config Parsing", () => { expect(api.getConfigFile).toHaveBeenCalledWith("example-location"); }); + describe("rule type", () => { + test("should fail with no rule type", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + teams: + - team-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError( + 'Configuration file is invalid: "rules[0].type" is required', + ); + }); + + test("should fail with no valid rule type", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: example-for-invalid + teams: + - team-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError( + 'Configuration file is invalid: "rules[0].type" must be one of [basic, debug]', + ); + }); + }); + describe("regular expressions validator", () => { test("should fail with invalid regular expression", async () => { const invalidRegex = "(?(";