From 56a897062f2bc34163a7ba964609c6afd2e80948 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 16:41:48 +0200 Subject: [PATCH 1/8] added type for "or" rule --- src/rules/types.ts | 15 +++++++++------ src/rules/validator.ts | 9 +++++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/rules/types.ts b/src/rules/types.ts index a6d7083..44b6f7c 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -2,9 +2,10 @@ export enum RuleTypes { Basic = "basic", Debug = "debug", And = "and", + Or = "or", } -export type Reviewers = { users?: string[]; teams?: string[] }; +export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: number }; export interface Rule { name: string; @@ -19,21 +20,23 @@ export interface DebugRule extends Rule { export interface BasicRule extends Rule, Reviewers { type: RuleTypes.Basic; - min_approvals: number; } export interface AndRule extends Rule { type: RuleTypes.And; - reviewers: ({ - min_approvals: number; - } & Reviewers)[]; + reviewers: Reviewers[]; +} + +export interface OrRule extends Rule { + type: RuleTypes.Or; + reviewers: Reviewers[]; } export interface ConfigurationFile { /** Based on the `type` parameter, Typescript converts the object to the correct type * @see {@link Rules} */ - rules: (BasicRule | DebugRule | AndRule)[]; + rules: (BasicRule | DebugRule | AndRule | OrRule)[]; preventReviewRequests?: { teams?: string[]; users?: string[]; diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 286df55..ec7eee1 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -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().valid(RuleTypes.Basic, RuleTypes.Debug, RuleTypes.And).required(), + type: Joi.string().valid(RuleTypes.Basic, RuleTypes.Debug, RuleTypes.And, RuleTypes.Or).required(), }); /** General Configuration schema. @@ -41,7 +41,8 @@ export const basicRuleSchema = Joi.object() .keys({ min_approvals: Joi.number().min(1).default(1), ...reviewersObj }) .or("users", "teams"); -export const andRuleSchema = Joi.object().keys({ +/** 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(basicRuleSchema).min(2).required(), }); @@ -66,8 +67,8 @@ export const validateConfig = (config: ConfigurationFile): ConfigurationFile | n validatedConfig.rules[i] = validate(rule, basicRuleSchema, { message }); } else if (type === "debug") { validatedConfig.rules[i] = validate(rule, ruleSchema, { message }); - } else if (type === "and") { - validatedConfig.rules[i] = validate(rule, andRuleSchema, { message }); + } else if (type === "and" || type === "or") { + validatedConfig.rules[i] = validate(rule, otherRulesSchema, { message }); } else { // eslint-disable-next-line @typescript-eslint/restrict-template-expressions throw new Error(`Rule ${name} has an invalid type: ${type}`); From 67d9bf92a5a0ec5b2f715061b39ecad9005d4ffa Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 16:52:57 +0200 Subject: [PATCH 2/8] added evaluation logic for OR rule --- src/runner.ts | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 039ce9d..b06b350 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -59,7 +59,7 @@ export class ActionRunner { */ async validatePullRequest({ rules }: ConfigurationFile): Promise { const errorReports: RuleReport[] = []; - for (const rule of rules) { + ruleCheck: 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 @@ -87,16 +87,30 @@ export class ActionRunner { } } if (reports.length > 0) { - const finalReport: RuleReport = { - missingReviews: reports.reduce((a, b) => a + b.missingReviews, 0), - missingUsers: [...new Set(reports.flatMap((r) => r.missingUsers))], - teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))], - usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))], - name: rule.name, - }; + const finalReport = unifyReport(reports, rule.name); this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); errorReports.push(finalReport); } + } else if (rule.type === "or") { + const reports: ReviewReport[] = []; + for (const reviewer of rule.reviewers) { + const [result, missingData] = await this.evaluateCondition(reviewer); + if (result) { + // This is a OR condition, so with ONE positive result + // we can continue the loop to check the following rule + continue ruleCheck; + } + // But, until we get a positive case we add all the failed cases + reports.push(missingData); + } + + // If the loop was not skipped it means that we have errors + if (reports.length > 0) { + const finalReport = unifyReport(reports, rule.name); + this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); + // We unify the reports and push them for handling + errorReports.push(finalReport); + } } } catch (error: unknown) { // We only throw if there was an unexpected error, not if the check fails @@ -291,3 +305,13 @@ export class ActionRunner { return checkRunData; } } + +const unifyReport = (reports: ReviewReport[], name: string): RuleReport => { + return { + missingReviews: reports.reduce((a, b) => a + b.missingReviews, 0), + missingUsers: [...new Set(reports.flatMap((r) => r.missingUsers))], + teamsToRequest: [...new Set(reports.flatMap((r) => r.teamsToRequest ?? []))], + usersToRequest: [...new Set(reports.flatMap((r) => r.usersToRequest ?? []))], + name, + }; +}; From 1131ee4c132b5a38cfe0f7154001320656053297 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 17:00:54 +0200 Subject: [PATCH 3/8] added or tests --- src/test/rules/config.test.ts | 5 +- src/test/rules/or.test.ts | 200 ++++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 src/test/rules/or.test.ts diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index 47425a0..208fda3 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -5,6 +5,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { PullRequestApi } from "../../github/pullRequest"; import { TeamApi } from "../../github/teams"; +import { RuleTypes } from "../../rules/types"; import { ActionRunner } from "../../runner"; import { TestLogger } from "../logger"; @@ -71,8 +72,10 @@ describe("Config Parsing", () => { teams: - team-example `); + // prettifies string to [basic, debug, and, or...] + const types = JSON.stringify(Object.values(RuleTypes)).replace(/"/g, "").replace(/,/g, ", "); await expect(runner.getConfigFile("")).rejects.toThrowError( - 'Configuration file is invalid: "rules[0].type" must be one of [basic, debug, and]', + 'Configuration file is invalid: "rules[0].type" must be one of ' + types, ); }); diff --git a/src/test/rules/or.test.ts b/src/test/rules/or.test.ts new file mode 100644 index 0000000..273dd51 --- /dev/null +++ b/src/test/rules/or.test.ts @@ -0,0 +1,200 @@ +/* eslint-disable @typescript-eslint/unbound-method */ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ +/* eslint-disable @typescript-eslint/no-unsafe-call */ +import { mock, MockProxy } from "jest-mock-extended"; + +import { PullRequestApi } from "../../github/pullRequest"; +import { TeamApi } from "../../github/teams"; +import { OrRule } from "../../rules/types"; +import { ActionRunner } from "../../runner"; +import { TestLogger } from "../logger"; + +describe("'Or' rule parsing", () => { + let api: MockProxy; + let runner: ActionRunner; + let teamsApi: MockProxy; + let logger: TestLogger; + beforeEach(() => { + logger = new TestLogger(); + api = mock(); + runner = new ActionRunner(api, teamsApi, logger); + }); + test("should get minimal config", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + reviewers: + - teams: + - team-example + - teams: + - team-abc + `); + const config = await runner.getConfigFile(""); + expect(config.rules[0].name).toEqual("Test review"); + expect(config.rules[0].type).toEqual("or"); + }); + + describe("reviewers", () => { + test("should require teams", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + reviewers: + - teams: + - team-example + - teams: + - team-abc + `); + const config = await runner.getConfigFile(""); + const rule = config.rules[0] as OrRule; + expect(rule.reviewers).toHaveLength(2); + expect(rule.reviewers[0].teams).toContainEqual("team-example"); + expect(rule.reviewers[0].users).toBeUndefined(); + expect(rule.reviewers[1].teams).toContainEqual("team-abc"); + expect(rule.reviewers[1].users).toBeUndefined(); + }); + test("should require users", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + reviewers: + - users: + - user-example + - users: + - user-special + `); + const config = await runner.getConfigFile(""); + const rule = config.rules[0] as OrRule; + expect(rule.reviewers[0].users).toContainEqual("user-example"); + expect(rule.reviewers[0].teams).toBeUndefined(); + expect(rule.reviewers[1].users).toContainEqual("user-special"); + expect(rule.reviewers[1].teams).toBeUndefined(); + }); + + test("should fail without reviewers", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + `); + await expect(runner.getConfigFile("")).rejects.toThrowError('"reviewers" is required'); + }); + + test("should fill the reviewers array", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + reviewers: + - teams: + - team-example + - min_approvals: 2 + users: + - abc + teams: + - xyz + `); + const config = await runner.getConfigFile(""); + const rule = config.rules[0] as OrRule; + expect(rule.reviewers).toHaveLength(2); + expect(rule.reviewers[0].teams).toContainEqual("team-example"); + expect(rule.reviewers[0].users).toBeUndefined(); + expect(rule.reviewers[1].min_approvals).toEqual(2); + expect(rule.reviewers[1].users).toContainEqual("abc"); + expect(rule.reviewers[1].teams).toContainEqual("xyz"); + }); + }); + + test("should default min_approvals to 1", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Test review + condition: + include: + - '.*' + exclude: + - 'example' + type: or + reviewers: + - users: + - user-example + - teams: + - team-example + `); + const config = await runner.getConfigFile(""); + const [rule] = config.rules; + if (rule.type === "or") { + expect(rule.reviewers[0].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: or + reviewers: + - min_approvals: -99 + users: + - user-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError( + '"reviewers[0].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: or + reviewers: + - min_approvals: 0 + users: + - user-example + `); + await expect(runner.getConfigFile("")).rejects.toThrowError( + '"reviewers[0].min_approvals" must be greater than or equal to 1', + ); + }); +}); From 72ebba1f6114a68c4f636119b36df7c3f4353bdf Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 17:46:40 +0200 Subject: [PATCH 4/8] fixed the amount of missing reviews --- src/runner.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/runner.ts b/src/runner.ts index b06b350..dba989e 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -106,7 +106,15 @@ export class ActionRunner { // If the loop was not skipped it means that we have errors if (reports.length > 0) { + // We get the lowest amount of reviews needed to fulfill one of the reviews + const lowerAmountOfReviewsNeeded = reports.reduce( + (a, b) => (a.missingReviews < b.missingReviews ? a : b), + 10, + ).missingReviews; + // We unify the reports const finalReport = unifyReport(reports, rule.name); + // We set the value to the minimum neccesary + finalReport.missingReviews = lowerAmountOfReviewsNeeded; this.logger.error(`Missing the reviews from ${JSON.stringify(finalReport.missingUsers)}`); // We unify the reports and push them for handling errorReports.push(finalReport); From ab10d84ebf063d21c00fe1bbf2dfa9f53124118d Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 17:46:47 +0200 Subject: [PATCH 5/8] added logic tests --- src/test/runner/validation/or.test.ts | 127 ++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) create mode 100644 src/test/runner/validation/or.test.ts diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts new file mode 100644 index 0000000..501c5a1 --- /dev/null +++ b/src/test/runner/validation/or.test.ts @@ -0,0 +1,127 @@ +import { mock, MockProxy } from "jest-mock-extended"; + +import { PullRequestApi } from "../../../github/pullRequest"; +import { TeamApi } from "../../../github/teams"; +import { ConfigurationFile, RuleTypes } from "../../../rules/types"; +import { ActionRunner } from "../../../runner"; +import { TestLogger } from "../../logger"; + +describe("'And' rule validation", () => { + let api: MockProxy; + let teamsApi: MockProxy; + let runner: ActionRunner; + let logger: TestLogger; + const users = ["user-1", "user-2", "user-3"]; + beforeEach(() => { + logger = new TestLogger(); + api = mock(); + teamsApi = mock(); + teamsApi.getTeamMembers.calledWith("abc").mockResolvedValue(users); + api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml"]); + api.listApprovedReviewsAuthors.mockResolvedValue([]); + runner = new ActionRunner(api, teamsApi, logger); + }); + + describe("approvals", () => { + test("should not report errors if the reviewers reviewed", async () => { + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [{ teams: ["abc"], min_approvals: 1 }], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); + const evaluation = await runner.validatePullRequest(config); + expect(evaluation).toHaveLength(0); + }); + + test("should not report errors if the reviewer belong to both conditions", async () => { + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { teams: ["abc"], min_approvals: 1 }, + { users: [users[0]], min_approvals: 1 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([users[0]]); + const evaluation = await runner.validatePullRequest(config); + expect(evaluation).toHaveLength(0); + }); + + test("should not report errors if the reviewer belong to one of the conditions", async () => { + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { teams: ["abc"], min_approvals: 1 }, + { users: [users[0]], min_approvals: 1 }, + { users: [users[1]], min_approvals: 1 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([users[2]]); + const evaluation = await runner.validatePullRequest(config); + expect(evaluation).toHaveLength(0); + }); + }); + describe("errors", () => { + test("should report all missing individual users if all of the rules have not been met", async () => { + const individualUsers = [users[0], users[1]]; + + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { teams: ["abc"], min_approvals: 1 }, + { users: individualUsers, min_approvals: 2 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const [result] = await runner.validatePullRequest(config); + expect(result.missingReviews).toEqual(1); + expect(result.missingUsers).toEqual(users); + expect(result.teamsToRequest).toContainEqual("abc"); + expect(result.usersToRequest).toEqual(individualUsers); + }); + + test("should show the lowest amount of reviews needed to fulfill the rule", async () => { + const config: ConfigurationFile = { + rules: [ + { + name: "Or rule", + type: RuleTypes.Or, + condition: { include: ["review-bot.yml"] }, + reviewers: [ + { users: ["abc"], min_approvals: 1 }, + { users: ["bcd", "cef"], min_approvals: 2 }, + { users: ["bds", "cj9", "dij", "ehu"], min_approvals: 4 }, + { users: ["bob", "cat", "dpo", "eio", "fgy"], min_approvals: 5 }, + ], + }, + ], + }; + api.listApprovedReviewsAuthors.mockResolvedValue([]); + const [result] = await runner.validatePullRequest(config); + expect(result.missingReviews).toEqual(1); + }); + }); +}); From c95b60e5b2460d63a351c37441ff617b90bd82e2 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 21:52:31 +0200 Subject: [PATCH 6/8] fixed reduce method I forgot to push a file --- src/runner.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index dba989e..f236193 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -107,10 +107,9 @@ export class ActionRunner { // If the loop was not skipped it means that we have errors if (reports.length > 0) { // We get the lowest amount of reviews needed to fulfill one of the reviews - const lowerAmountOfReviewsNeeded = reports.reduce( - (a, b) => (a.missingReviews < b.missingReviews ? a : b), - 10, - ).missingReviews; + const lowerAmountOfReviewsNeeded = reports + .map((r) => r.missingReviews) + .reduce((a, b) => (a < b ? a : b), 999); // We unify the reports const finalReport = unifyReport(reports, rule.name); // We set the value to the minimum neccesary From 615ac17d2fac7cf35ab20e434b06517106cd8756 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Fri, 11 Aug 2023 21:53:27 +0200 Subject: [PATCH 7/8] moved check event to action --- .github/workflows/publish.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 2d17d1a..e5ef769 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -17,13 +17,13 @@ jobs: run: docker build . --file Dockerfile validate-action: runs-on: ubuntu-latest + if: github.event_name == 'pull_request' steps: - uses: actions/checkout@v3.3.0 # This checks that .github/workflows/review-bot.yml is pointing towards the main branch # as, during development, we change this to use the code from the test branch and # we may forget to set it back to main - name: Validate that action points to main branch - if: github.event_name == 'pull_request' run: | BRANCH=$(yq '.jobs.review-approvals.steps[1].uses' $FILE_NAME | cut -d "@" -f2) # If the branch is not the main branch From 73fb957e812d7949cd28db407bfe0c70236793ce Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Mon, 14 Aug 2023 10:44:48 +0200 Subject: [PATCH 8/8] Fixed wrong test name Co-authored-by: Przemek Rzad --- src/test/runner/validation/or.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/runner/validation/or.test.ts b/src/test/runner/validation/or.test.ts index 501c5a1..da46ca4 100644 --- a/src/test/runner/validation/or.test.ts +++ b/src/test/runner/validation/or.test.ts @@ -6,7 +6,7 @@ import { ConfigurationFile, RuleTypes } from "../../../rules/types"; import { ActionRunner } from "../../../runner"; import { TestLogger } from "../../logger"; -describe("'And' rule validation", () => { +describe("'Or' rule validation", () => { let api: MockProxy; let teamsApi: MockProxy; let runner: ActionRunner;