From 6ac675006f4e10ab4c44bc36941f2f6f9ee8af2b Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 10:32:58 +0000 Subject: [PATCH 01/25] implemented action on itself --- .github/workflows/review-bot.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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..1c3c926 --- /dev/null +++ b/.github/workflows/review-bot.yml @@ -0,0 +1,12 @@ +name: Review PR +on: + pull_request: + +jobs: + test-image: + runs-on: ubuntu-latest + steps: + - uses: paritytech/review-bot@main + with: + repo-token: ${{ secrets.GITHUB_TOKEN }} + team-token: ${{ secrets.TEST_TEAM_TOKEN }} From 04480561541a6d684ec1abbe0b1bb53eeed898b4 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 10:37:03 +0000 Subject: [PATCH 02/25] renamed action step name --- .github/workflows/review-bot.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 1c3c926..e095383 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -3,7 +3,7 @@ on: pull_request: jobs: - test-image: + review-approvals: runs-on: ubuntu-latest steps: - uses: paritytech/review-bot@main From 3a9e194374be29c6578a520fa4481a7b25c1ea26 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 10:56:46 +0000 Subject: [PATCH 03/25] added debug log --- src/github/teams.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/github/teams.ts b/src/github/teams.ts index 49d0596..5955a29 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -34,6 +34,8 @@ export class GitHubTeamsApi implements TeamApi { if (this.teamsCache.has(teamName)) { return this.teamsCache.get(teamName) as string[]; } + this.logger.debug(`Fetching team '${teamName}'`); + // TODO: cache the result const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); return data.map((d) => d.login); } From 6d4ed8e33cfd9aa5fff240c4f3e78493f4937e90 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:01:24 +0000 Subject: [PATCH 04/25] added verbosity before file fetch --- src/github/pullRequest.ts | 1 + src/github/teams.ts | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 02973b9..f2abafd 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, diff --git a/src/github/teams.ts b/src/github/teams.ts index 5955a29..f6578f8 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -37,6 +37,7 @@ export class GitHubTeamsApi implements TeamApi { this.logger.debug(`Fetching team '${teamName}'`); // TODO: cache the result const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); - return data.map((d) => d.login); + const members = data.map((d) => d.login); + } } From 711d8d28c8874b5925b66c96a12b977a8469953f Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:03:37 +0000 Subject: [PATCH 05/25] added content permissions to action --- .github/workflows/review-bot.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index e095383..168961b 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -2,6 +2,9 @@ name: Review PR on: pull_request: +permissions: + contents: read + jobs: review-approvals: runs-on: ubuntu-latest From 124e7ae286fdd531edf8446c7a9f7f64d4223270 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:34:06 +0000 Subject: [PATCH 06/25] fixed teams null return --- src/github/teams.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/github/teams.ts b/src/github/teams.ts index f6578f8..0b5a954 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -31,13 +31,13 @@ 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}'`); + // TODO: cache the result + 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); } - this.logger.debug(`Fetching team '${teamName}'`); - // TODO: cache the result - const { data } = await this.api.rest.teams.listMembersInOrg({ org: this.org, team_slug: teamName }); - const members = data.map((d) => d.login); - + return this.teamsCache.get(teamName) as string[]; } } From 400e38c9ec41ea86332858eb7de074157ee7b886 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:34:26 +0000 Subject: [PATCH 07/25] assigned action to test branch --- .github/workflows/review-bot.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 168961b..4cd70f9 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -9,7 +9,8 @@ jobs: review-approvals: runs-on: ubuntu-latest steps: - - uses: paritytech/review-bot@main + # TODO: Set this to main + - uses: paritytech/review-bot@self-implementation with: repo-token: ${{ secrets.GITHUB_TOKEN }} team-token: ${{ secrets.TEST_TEAM_TOKEN }} From 7a152206f91fdb9f69790cd790f759a898dd1ca6 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:38:21 +0000 Subject: [PATCH 08/25] UNDO: obtained config file from repo itself --- src/github/pullRequest.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index f2abafd..92e4ad0 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -25,6 +25,8 @@ export class PullRequestApi { owner: this.pr.base.repo.owner.login, repo: this.pr.base.repo.name, path: configFilePath, + // TODO: Remove this + ref: "self-implementation" }); if (!("content" in data)) { From a0a4eb83697e2464f67b848a843a898a8c55f830 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:48:32 +0000 Subject: [PATCH 09/25] fixed default file name --- action.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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' From be6057e37bfd0a65d87bd686f6ccbc3a9bb81e09 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 11:53:54 +0000 Subject: [PATCH 10/25] added more pr events to action --- .github/workflows/review-bot.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/review-bot.yml b/.github/workflows/review-bot.yml index 4cd70f9..279bbae 100644 --- a/.github/workflows/review-bot.yml +++ b/.github/workflows/review-bot.yml @@ -1,6 +1,14 @@ name: Review PR on: pull_request: + types: + - opened + - reopened + - synchronize + - review_requested + - review_request_removed + - ready_for_review + pull_request_review: permissions: contents: read From 4027d6d74ffd4da61ef72e2aa17a44db1ccbc234 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 12:12:58 +0000 Subject: [PATCH 11/25] improved logs --- src/github/teams.ts | 1 - src/runner.ts | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/github/teams.ts b/src/github/teams.ts index 0b5a954..4eb537a 100644 --- a/src/github/teams.ts +++ b/src/github/teams.ts @@ -33,7 +33,6 @@ export class GitHubTeamsApi implements TeamApi { // We first verify that this information hasn't been fetched yet if (!this.teamsCache.has(teamName)) { this.logger.debug(`Fetching team '${teamName}'`); - // TODO: cache the result 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); diff --git a/src/runner.ts b/src/runner.ts index 9959e3e..50df1fb 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -56,11 +56,12 @@ export class ActionRunner { async validatePullRequest({ rules }: ConfigurationFile): Promise { 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; } From a115fdafc1b3e84b94c1797c000bb15c45f3716c Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 12:19:29 +0000 Subject: [PATCH 12/25] added more verbosity --- src/github/pullRequest.ts | 1 + src/runner.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 92e4ad0..bdb4c71 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -59,6 +59,7 @@ export class PullRequestApi { const approvals = reviews.filter((review) => review.state === "approved"); this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); } + this.logger.debug(`PR approvals are ${JSON.stringify(this.usersThatApprovedThePr)}`); return this.usersThatApprovedThePr; } } diff --git a/src/runner.ts b/src/runner.ts index 50df1fb..3f55412 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -89,6 +89,8 @@ export class ActionRunner { * @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 @@ -126,6 +128,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; @@ -138,6 +141,7 @@ export class ActionRunner { // Now we verify if we have any remaining missing review. if (missingReviews > 0) { + 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 [ @@ -150,6 +154,7 @@ export class ActionRunner { }, ]; } else { + this.logger.info("Rule requirements fulfilled"); // If we don't have any missing reviews, we return the succesful case return [true]; } From 99bfe3ac8b834ceca3a77ff6a12f98a59695df88 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 12:44:25 +0000 Subject: [PATCH 13/25] defaulted min_approvals to 1 --- src/file/validator.ts | 16 +++++++++------- src/test/runner/basicRule.test.ts | 22 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/file/validator.ts b/src/file/validator.ts index 551b758..61df560 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,8 @@ 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(), + // TODO: Add test with invalid type + type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug).required(), }); /** General Configuration schema. @@ -38,7 +39,8 @@ 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 }) + // TODO: Add test with negative numbers + .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj }) .or("users", "teams"); /** @@ -54,19 +56,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/test/runner/basicRule.test.ts b/src/test/runner/basicRule.test.ts index 87852e7..96f310c 100644 --- a/src/test/runner/basicRule.test.ts +++ b/src/test/runner/basicRule.test.ts @@ -87,4 +87,26 @@ 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`); + } + }); }); From bc25029427c0aed83c4ff1c3c73dfe5f1dd7510b Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 12:48:21 +0000 Subject: [PATCH 14/25] added more logs --- src/github/pullRequest.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index bdb4c71..baaaf93 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -56,6 +56,7 @@ 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[]; + this.logger.debug(`List of reviews: ${JSON.stringify(reviews)}`); const approvals = reviews.filter((review) => review.state === "approved"); this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); } From 0b456b68a3582207b1028abd2b9bb6f4fe61178e Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 13:33:19 +0000 Subject: [PATCH 15/25] fixed case senstive comparison --- src/github/pullRequest.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index baaaf93..e7513ac 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -26,7 +26,7 @@ export class PullRequestApi { repo: this.pr.base.repo.name, path: configFilePath, // TODO: Remove this - ref: "self-implementation" + ref: "self-implementation", }); if (!("content" in data)) { @@ -57,7 +57,9 @@ export class PullRequestApi { const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number }); const reviews = request.data as PullRequestReview[]; this.logger.debug(`List of reviews: ${JSON.stringify(reviews)}`); - const approvals = reviews.filter((review) => review.state === "approved"); + const approvals = reviews.filter((review) => + review.state.localeCompare("approved", undefined, { sensitivity: "accent" }), + ); this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); } this.logger.debug(`PR approvals are ${JSON.stringify(this.usersThatApprovedThePr)}`); From ce05e6471b80c3742e51077515d47be4de26e373 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 13:46:02 +0000 Subject: [PATCH 16/25] fixed wrong comparisson --- src/github/pullRequest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index e7513ac..0d3a7bf 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -57,8 +57,8 @@ export class PullRequestApi { const request = await this.api.rest.pulls.listReviews({ ...this.repoInfo, pull_number: this.number }); const reviews = request.data as PullRequestReview[]; this.logger.debug(`List of reviews: ${JSON.stringify(reviews)}`); - const approvals = reviews.filter((review) => - review.state.localeCompare("approved", undefined, { sensitivity: "accent" }), + const approvals = reviews.filter( + (review) => review.state.localeCompare("approved", undefined, { sensitivity: "accent" }) === 0, ); this.usersThatApprovedThePr = approvals.map((approval) => approval.user.login); } From f18d2b828c756f13f597c8537e1f8a125083df02 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 13:54:38 +0000 Subject: [PATCH 17/25] added author discrimination Not negative one, just to exclude him from the list of valid reviewers --- src/github/pullRequest.ts | 5 +++++ src/runner.ts | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 0d3a7bf..6efef6c 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -65,4 +65,9 @@ export class PullRequestApi { 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/runner.ts b/src/runner.ts index 3f55412..5dfbf29 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -141,6 +141,7 @@ 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 @@ -148,7 +149,8 @@ export class ActionRunner { 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, }, From 41ea9117e10234262bcc90127a83c2977ca31526 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 14:20:41 +0000 Subject: [PATCH 18/25] added a final report function --- src/runner.ts | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 5dfbf29..0df3206 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -54,6 +54,7 @@ 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}`); @@ -69,7 +70,7 @@ 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) { @@ -77,12 +78,32 @@ export class ActionRunner { 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 From 8ddcc56a841dc34f052d7f77ad7fff484836e974 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 16:20:18 +0000 Subject: [PATCH 19/25] linted project --- src/runner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 0df3206..a235d52 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -80,12 +80,12 @@ export class ActionRunner { } this.logger.info(`Finish validating ${rule.name}`); } - if(errorReports.length > 0) { + 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; } From 838f1fc615e0adae86c1e578d8db1f4244c1766a Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Wed, 26 Jul 2023 16:45:45 +0000 Subject: [PATCH 20/25] imeplemented array concatenation method --- src/runner.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index a235d52..c5d014c 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -118,12 +118,7 @@ export class ActionRunner { 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 } From a3ef620550ec585d2ae1ad0ba4a59c23cf0715f3 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 11:58:01 +0200 Subject: [PATCH 21/25] removed test configuration file --- src/github/pullRequest.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/github/pullRequest.ts b/src/github/pullRequest.ts index 6efef6c..bba1e7b 100644 --- a/src/github/pullRequest.ts +++ b/src/github/pullRequest.ts @@ -25,8 +25,6 @@ export class PullRequestApi { owner: this.pr.base.repo.owner.login, repo: this.pr.base.repo.name, path: configFilePath, - // TODO: Remove this - ref: "self-implementation", }); if (!("content" in data)) { From ffbe2ddb866ccd259dbb9f8b1686f57567714f35 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 12:10:03 +0200 Subject: [PATCH 22/25] beautified logging --- src/runner.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index c5d014c..6ddb81d 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -57,7 +57,7 @@ export class ActionRunner { const errorReports: ReviewReport[] = []; for (const rule of rules) { try { - this.logger.info(`Validating rule ${rule.name}`); + 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 @@ -75,10 +75,10 @@ export class ActionRunner { } } 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}`); + this.logger.info(`Finish validating '${rule.name}'`); } if (errorReports.length > 0) { const finalReport = this.aggregateReports(errorReports); From ce086ab9b3b64bfed18be1d33ad7438388e6c471 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 12:24:57 +0200 Subject: [PATCH 23/25] added test to evaluate negative numbers --- src/file/validator.ts | 1 - src/runner.ts | 2 +- src/test/runner/basicRule.test.ts | 34 +++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/file/validator.ts b/src/file/validator.ts index 61df560..ce57a0b 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -39,7 +39,6 @@ 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() - // TODO: Add test with negative numbers .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj }) .or("users", "teams"); diff --git a/src/runner.ts b/src/runner.ts index 6ddb81d..f0e5c46 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -144,7 +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.logger.info(`Found ${approvals.length} approvals: ${JSON.stringify(approvals)}`); // This is the amount of reviews required. To succeed this should be 0 or lower let missingReviews = rule.min_approvals; diff --git a/src/test/runner/basicRule.test.ts b/src/test/runner/basicRule.test.ts index 96f310c..6ed5221 100644 --- a/src/test/runner/basicRule.test.ts +++ b/src/test/runner/basicRule.test.ts @@ -109,4 +109,38 @@ describe("Basic rule parsing", () => { 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'); + }); }); From 289cb66fb093dfd4311371b2eaa89d7024f8bb5e Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 12:28:00 +0200 Subject: [PATCH 24/25] added tests to validate invalid rule types --- src/file/validator.ts | 1 - src/test/runner/config.test.ts | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/file/validator.ts b/src/file/validator.ts index ce57a0b..2828cf5 100644 --- a/src/file/validator.ts +++ b/src/file/validator.ts @@ -22,7 +22,6 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - // TODO: Add test with invalid type type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug).required(), }); 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 = "(?("; From c2233aaf9d117c4fa725dc08821701b2f1ec0f51 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 1 Aug 2023 12:36:11 +0200 Subject: [PATCH 25/25] removed unnecessary log information It is already logged in the PR API method --- src/runner.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runner.ts b/src/runner.ts index f0e5c46..7e97f60 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -144,7 +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: ${JSON.stringify(approvals)}`); + 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;