From 3bde908e8846521ba867eeda8cc65fda8e1b51fc Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 11:20:46 -0300 Subject: [PATCH 01/14] created greenlight code base --- src/rules/types.ts | 1 + src/rules/validator.ts | 1 + src/runner.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/src/rules/types.ts b/src/rules/types.ts index ec70b9f..3dba71a 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -10,6 +10,7 @@ export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: num export interface Rule { name: string; condition: { include: string[]; exclude?: string[] }; + greenlightFrom?: Omit; countAuthor?: boolean; } diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 7cc010d..a09a732 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -24,6 +24,7 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), + greenlightFrom:Joi.object>().keys(reviewersObj).optional().or("users", "teams"), type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), }); diff --git a/src/runner.ts b/src/runner.ts index 241a1c1..bd0d1a9 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -81,6 +81,13 @@ export class ActionRunner { this.logger.info(`Skipping rule ${rule.name} as no condition matched`); // If there are no matches, we simply skip the check continue; + } else if (rule.greenlightFrom) { + const members = await this.fetchAllMembers(rule.greenlightFrom); + const author = this.prApi.getAuthor(); + if (members.indexOf(author) > -1) { + this.logger.info(`Skipping rule ${rule.name} as author belong to greenlight rule.`); + continue; + } } if (rule.type === "basic") { const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor); @@ -455,6 +462,25 @@ export class ActionRunner { return matches; } + async fetchAllMembers(reviewers: Omit): Promise { + const users: Set = new Set(); + if (reviewers.teams) { + for (const team of reviewers.teams) { + const members = await this.teamApi.getTeamMembers(team); + for (const member of members) { + users.add(member); + } + } + } + if (reviewers.users) { + for (const user of reviewers.users) { + users.add(user); + } + } + + return Array.from(users); + } + /** Core runner of the app. * 1. It fetches the config * 2. It validates all the pull request requirements based on the config file From 64b9ab4b34186e7637250d038964ed4e1e55a7e1 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 11:22:40 -0300 Subject: [PATCH 02/14] applied utility method --- src/runner.ts | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index bd0d1a9..b9522d0 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -375,18 +375,8 @@ export class ActionRunner { 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); - 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 - } - if (rule.users) { - requiredUsers = concatArraysUniquely(requiredUsers, rule.users); - } + const requiredUsers: string[] = await this.fetchAllMembers(rule); + if (requiredUsers.length === 0) { throw new Error("No users have been found in the required reviewers"); } @@ -462,6 +452,11 @@ export class ActionRunner { return matches; } + /** + * Fetch all the members of a team and/or list and removes duplicates + * @param reviewers Object with users or teams to fetch members + * @returns an array with all the users + */ async fetchAllMembers(reviewers: Omit): Promise { const users: Set = new Set(); if (reviewers.teams) { From d54ef93f41276980a4fff2b8e2121d4d0b6b6a3b Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 11:26:08 -0300 Subject: [PATCH 03/14] added tests to new method --- src/runner.ts | 6 +++--- src/test/runner/runner.test.ts | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index b9522d0..466d404 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -82,7 +82,7 @@ export class ActionRunner { // If there are no matches, we simply skip the check continue; } else if (rule.greenlightFrom) { - const members = await this.fetchAllMembers(rule.greenlightFrom); + const members = await this.fetchAllUsers(rule.greenlightFrom); const author = this.prApi.getAuthor(); if (members.indexOf(author) > -1) { this.logger.info(`Skipping rule ${rule.name} as author belong to greenlight rule.`); @@ -375,7 +375,7 @@ export class ActionRunner { this.logger.debug(JSON.stringify(rule)); // This is a list of all the users that need to approve a PR - const requiredUsers: string[] = await this.fetchAllMembers(rule); + const requiredUsers: string[] = await this.fetchAllUsers(rule); if (requiredUsers.length === 0) { throw new Error("No users have been found in the required reviewers"); @@ -457,7 +457,7 @@ export class ActionRunner { * @param reviewers Object with users or teams to fetch members * @returns an array with all the users */ - async fetchAllMembers(reviewers: Omit): Promise { + async fetchAllUsers(reviewers: Omit): Promise { const users: Set = new Set(); if (reviewers.teams) { for (const team of reviewers.teams) { diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 35619e2..80e8b63 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -32,6 +32,24 @@ describe("Shared validations", () => { expect(evaluation).toBeTruthy(); }); + test("validatePullRequest should return true if no rule matches any files", async () => { + const config: ConfigurationFile = { + rules: [ + { name: "Rule 1", type: RuleTypes.Basic, condition: { include: ["src"] }, min_approvals: 1 }, + { name: "Rule 2", type: RuleTypes.Basic, condition: { include: ["README.md"] }, min_approvals: 99 }, + ], + }; + api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml", "LICENSE"]); + const evaluation = await runner.validatePullRequest(config); + expect(evaluation).toBeTruthy(); + }); + + test("fetchAllUsers should not return duplicates", async () => { + teamsApi.getTeamMembers.mockResolvedValue(["user-1", "user-2", "user-3"]); + const users = await runner.fetchAllUsers({ teams: ["abc"], users: ["user-1", "user-2", "user-4"] }); + expect(users).toStrictEqual(["user-1", "user-2", "user-3", "user-4"]); + }); + describe("listFilesThatMatchRuleCondition tests", () => { test("should get values that match the condition", () => { const mockRule = { condition: { include: ["src"] } }; From c66fda7de10b8765fc98bda91a5a062723c4bf63 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 12:21:24 -0300 Subject: [PATCH 04/14] updated reference --- src/rules/types.ts | 2 +- src/rules/validator.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rules/types.ts b/src/rules/types.ts index 3dba71a..6521a70 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -10,7 +10,7 @@ export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: num export interface Rule { name: string; condition: { include: string[]; exclude?: string[] }; - greenlightFrom?: Omit; + greenlightAuthorsFrom?: Omit; countAuthor?: boolean; } diff --git a/src/rules/validator.ts b/src/rules/validator.ts index a09a732..da9a61f 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -24,7 +24,7 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - greenlightFrom:Joi.object>().keys(reviewersObj).optional().or("users", "teams"), + greenlightAuthorsFrom:Joi.object>().keys(reviewersObj).optional().or("users", "teams"), type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), }); From e62f63fa545d20b994710d7b4d58215f8001375c Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 12:21:49 -0300 Subject: [PATCH 05/14] and name --- src/runner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 466d404..79128c4 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -81,8 +81,8 @@ export class ActionRunner { this.logger.info(`Skipping rule ${rule.name} as no condition matched`); // If there are no matches, we simply skip the check continue; - } else if (rule.greenlightFrom) { - const members = await this.fetchAllUsers(rule.greenlightFrom); + } else if (rule.greenlightAuthorsFrom) { + const members = await this.fetchAllUsers(rule.greenlightAuthorsFrom); const author = this.prApi.getAuthor(); if (members.indexOf(author) > -1) { this.logger.info(`Skipping rule ${rule.name} as author belong to greenlight rule.`); From a3a83f8f7743696fcff8453783a6362e30386c25 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 12:45:09 -0300 Subject: [PATCH 06/14] applied yarn fix --- src/rules/validator.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/rules/validator.ts b/src/rules/validator.ts index da9a61f..66e1bd3 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -24,7 +24,10 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - greenlightAuthorsFrom:Joi.object>().keys(reviewersObj).optional().or("users", "teams"), + greenlightAuthorsFrom: Joi.object>() + .keys(reviewersObj) + .optional() + .or("users", "teams"), type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), }); From 22a562f59d075e19d922a3cb7f762066bcc4260f Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 12:58:10 -0300 Subject: [PATCH 07/14] aadded audit rules --- src/test/config.yml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/test/config.yml b/src/test/config.yml index 54e0328..2aa0c93 100644 --- a/src/test/config.yml +++ b/src/test/config.yml @@ -16,6 +16,24 @@ rules: - ci - release-engineering + - name: Audit rules + type: basic + condition: + include: + - ^polkadot/runtime\/(kusama|polkadot|common)\/.* + - ^polkadot/primitives/src\/.+\.rs$ + - ^substrate/primitives/.* + - ^substrate/frame/.* + exclude: + - ^polkadot/runtime\/(kusama|polkadot)\/src\/weights\/.+\.rs$ + - ^substrate\/frame\/.+\.md$ + min_approvals: 2 + greenlightAuthorsFrom: + teams: + - core-devs + teams: + - srlabs + - name: Core developers countAuthor: true condition: From 8320ba4f4ddf515038fd2e0924847ab2f38cae19 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 13:09:25 -0300 Subject: [PATCH 08/14] added integration test cases --- src/test/index.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/test/index.test.ts b/src/test/index.test.ts index 1e8dffb..ee3c922 100644 --- a/src/test/index.test.ts +++ b/src/test/index.test.ts @@ -13,6 +13,7 @@ import { ActionRunner, RuleReport } from "../runner"; type ReportName = | "CI files" | "Core developers" + | "Audit rules" | "Runtime files cumulus" | "Bridges subtree files" | "FRAME coders substrate"; @@ -39,6 +40,7 @@ describe("Integration testing", () => { ["polkadot-review", ["gavofyork", "bkchr", "pr-1", "pr-2"]], ["bridges-core", ["bridge-1", "bridge-2", "bridge-3"]], ["frame-coders", ["frame-1", "frame-2", "frame-3"]], + ["srlabs", ["audit-1", "audit-2", "audit-3"]], ]; let api: PullRequestApi; @@ -225,6 +227,18 @@ describe("Integration testing", () => { expect(report.missingReviews).toBe(2); }); + test("should skip audit rule if author belongs to greenlighted team", async () => { + // @ts-ignore + client.rest.pulls.listFiles.mockResolvedValue({ data: [{ filename: "polkadot/primitives/src/transfer.rs" }] }); + const result = await runner.runAction({ configLocation: "abc" }); + expect(result.reports).toHaveLength(2); + expect(result.reports.map((r) => r.name)).toContainEqual("Audit rules"); + pr.user.login = "gavofyork"; + const newResult = await runner.runAction({ configLocation: "abc" }); + expect(newResult.reports).toHaveLength(1); + expect(newResult.reports.map((r) => r.name)).not.toContainEqual("Audit rules"); + }); + describe("Combinations", () => { test("should use same reviewer for separate rules", async () => { client.rest.pulls.listFiles.mockResolvedValue({ From aa765085972f45b69c049883cf1beb175c118395 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 16:41:41 -0300 Subject: [PATCH 09/14] renamed config to excludeAuthors --- src/rules/types.ts | 2 +- src/rules/validator.ts | 5 +---- src/runner.ts | 4 ++-- src/test/config.yml | 2 +- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/rules/types.ts b/src/rules/types.ts index 6521a70..7df3307 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -10,7 +10,7 @@ export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: num export interface Rule { name: string; condition: { include: string[]; exclude?: string[] }; - greenlightAuthorsFrom?: Omit; + excludeAuthors?: Omit; countAuthor?: boolean; } diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 66e1bd3..601e5cb 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -24,10 +24,7 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - greenlightAuthorsFrom: Joi.object>() - .keys(reviewersObj) - .optional() - .or("users", "teams"), + excludeAuthors: Joi.object>().keys(reviewersObj).optional().or("users", "teams"), type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), }); diff --git a/src/runner.ts b/src/runner.ts index 79128c4..d966a13 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -81,8 +81,8 @@ export class ActionRunner { this.logger.info(`Skipping rule ${rule.name} as no condition matched`); // If there are no matches, we simply skip the check continue; - } else if (rule.greenlightAuthorsFrom) { - const members = await this.fetchAllUsers(rule.greenlightAuthorsFrom); + } else if (rule.excludeAuthors) { + const members = await this.fetchAllUsers(rule.excludeAuthors); const author = this.prApi.getAuthor(); if (members.indexOf(author) > -1) { this.logger.info(`Skipping rule ${rule.name} as author belong to greenlight rule.`); diff --git a/src/test/config.yml b/src/test/config.yml index 2aa0c93..a5d85eb 100644 --- a/src/test/config.yml +++ b/src/test/config.yml @@ -28,7 +28,7 @@ rules: - ^polkadot/runtime\/(kusama|polkadot)\/src\/weights\/.+\.rs$ - ^substrate\/frame\/.+\.md$ min_approvals: 2 - greenlightAuthorsFrom: + excludeAuthors: teams: - core-devs teams: From 90b2dd3ddf3ce6931fc3a837ca25f690c4e07851 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 16:50:36 -0300 Subject: [PATCH 10/14] added tests --- src/test/rules/config.test.ts | 87 +++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index 3255491..fcb24ac 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -271,5 +271,92 @@ describe("Config Parsing", () => { const config = await runner.getConfigFile(""); expect(config.rules[0].condition.exclude).toBeUndefined(); }); + + describe("excludeAuthor", () => { + test("should get teams", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + excludeAuthors: + teams: + - team-example + teams: + - team-example + `); + const config = await runner.getConfigFile(""); + const { excludeAuthors } = config.rules[0]; + expect(excludeAuthors?.teams).toEqual(["team-example"]); + expect(excludeAuthors?.users).toBeUndefined(); + }); + + test("should get users", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + excludeAuthors: + users: + - user-example + teams: + - team-example + `); + const config = await runner.getConfigFile(""); + const { excludeAuthors } = config.rules[0]; + expect(excludeAuthors?.users).toEqual(["user-example"]); + expect(excludeAuthors?.teams).toBeUndefined(); + }); + + test("should get teams and users", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + excludeAuthors: + teams: + - team-example + users: + - user-example + teams: + - team-example + `); + const config = await runner.getConfigFile(""); + const { excludeAuthors } = config.rules[0]; + expect(excludeAuthors?.teams).toEqual(["team-example"]); + expect(excludeAuthors?.users).toEqual(["user-example"]); + }); + + test("should be null by default", async () => { + api.getConfigFile.mockResolvedValue(` + rules: + - name: Default review + condition: + include: + - '.*' + exclude: + - 'example' + type: basic + teams: + - team-example + `); + const config = await runner.getConfigFile(""); + expect(config.rules[0].excludeAuthors).toBeUndefined(); + }); + }); }); }); From 4c6a2661e0aeaf6a9c6512850be75f5046646ea7 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 17:02:57 -0300 Subject: [PATCH 11/14] updated readme --- README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/README.md b/README.md index e9e2226..b61e5be 100644 --- a/README.md +++ b/README.md @@ -238,6 +238,11 @@ rules: - user-1 - user-2 countAuthor: true + excludeAuthors: + teams: + - team-1 + users: + - user-1 ``` It has the same parameters as a normal rule: - **name**: Name of the rule. This value must be unique per rule. @@ -257,6 +262,10 @@ It has the same parameters as a normal rule: - **countAuthor**: If the pull request author should be considered as an approval. - If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total). - ** Optional**: Defaults to `false` +- **excludeAuthor**: If the author belong to one of the teams and/or users in the list, the rule should be skipped. + - **Optional**. + - This is useful for cases where we want to make sure that some eyes look into a PR, but for we don’t need to ensure that much security on internal teams. + - For example, if someone modifies a CI file, we want to make sure they didn’t break anything. Unless it’s someone from the CI team. They *should know* what they are doing. #### Other rules The other three rules (**or**, **and** and **and-distinct**) have the same configuration, so let’s summarize that here and then move into how they work. ```yaml From 96787b85904239f0056cd54fd4c08d53f4608bb6 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 17:08:47 -0300 Subject: [PATCH 12/14] added rule to verify if it skips --- src/test/runner/runner.test.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 80e8b63..404ee14 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -32,16 +32,24 @@ describe("Shared validations", () => { expect(evaluation).toBeTruthy(); }); - test("validatePullRequest should return true if no rule matches any files", async () => { + test("validatePullRequest should return true if author belongs to excludeAuthors", async () => { const config: ConfigurationFile = { rules: [ - { name: "Rule 1", type: RuleTypes.Basic, condition: { include: ["src"] }, min_approvals: 1 }, - { name: "Rule 2", type: RuleTypes.Basic, condition: { include: ["README.md"] }, min_approvals: 99 }, + { + name: "Rule excludeAuthors", + type: RuleTypes.Basic, + condition: { include: ["src"] }, + min_approvals: 1, + excludeAuthors: { teams: ["abc"] }, + }, ], }; - api.listModifiedFiles.mockResolvedValue([".github/workflows/review-bot.yml", "LICENSE"]); + api.listModifiedFiles.mockResolvedValue(["src/polkadot/init.rs", "LICENSE"]); + teamsApi.getTeamMembers.mockResolvedValue(["user-1", "user-2", "user-3"]); + api.getAuthor.mockReturnValue("user-1"); const evaluation = await runner.validatePullRequest(config); expect(evaluation).toBeTruthy(); + expect(logger.info).toHaveBeenCalledWith("Skipping rule Rule excludeAuthors as author belong to greenlight rule."); }); test("fetchAllUsers should not return duplicates", async () => { From 6b02415ae7e64498212cde07292365da45841814 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 19:15:29 -0300 Subject: [PATCH 13/14] renamed excludeAuthors to allowedToSkipRule --- README.md | 2 +- src/rules/types.ts | 2 +- src/rules/validator.ts | 2 +- src/runner.ts | 4 ++-- src/test/config.yml | 2 +- src/test/rules/config.test.ts | 26 +++++++++++++------------- src/test/runner/runner.test.ts | 10 ++++++---- 7 files changed, 25 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index b61e5be..4fd5b2a 100644 --- a/README.md +++ b/README.md @@ -238,7 +238,7 @@ rules: - user-1 - user-2 countAuthor: true - excludeAuthors: + allowedToSkipRule: teams: - team-1 users: diff --git a/src/rules/types.ts b/src/rules/types.ts index 7df3307..3a7e0c5 100644 --- a/src/rules/types.ts +++ b/src/rules/types.ts @@ -10,7 +10,7 @@ export type Reviewers = { users?: string[]; teams?: string[]; min_approvals: num export interface Rule { name: string; condition: { include: string[]; exclude?: string[] }; - excludeAuthors?: Omit; + allowedToSkipRule?: Omit; countAuthor?: boolean; } diff --git a/src/rules/validator.ts b/src/rules/validator.ts index 601e5cb..c7b2c0b 100644 --- a/src/rules/validator.ts +++ b/src/rules/validator.ts @@ -24,7 +24,7 @@ const ruleSchema = Joi.object().keys({ include: Joi.array().items(Joi.string()).required(), exclude: Joi.array().items(Joi.string()).optional().allow(null), }), - excludeAuthors: Joi.object>().keys(reviewersObj).optional().or("users", "teams"), + allowedToSkipRule: Joi.object>().keys(reviewersObj).optional().or("users", "teams"), type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(), }); diff --git a/src/runner.ts b/src/runner.ts index d966a13..bb96ed5 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -81,8 +81,8 @@ export class ActionRunner { this.logger.info(`Skipping rule ${rule.name} as no condition matched`); // If there are no matches, we simply skip the check continue; - } else if (rule.excludeAuthors) { - const members = await this.fetchAllUsers(rule.excludeAuthors); + } else if (rule.allowedToSkipRule) { + const members = await this.fetchAllUsers(rule.allowedToSkipRule); const author = this.prApi.getAuthor(); if (members.indexOf(author) > -1) { this.logger.info(`Skipping rule ${rule.name} as author belong to greenlight rule.`); diff --git a/src/test/config.yml b/src/test/config.yml index a5d85eb..8e7d8ab 100644 --- a/src/test/config.yml +++ b/src/test/config.yml @@ -28,7 +28,7 @@ rules: - ^polkadot/runtime\/(kusama|polkadot)\/src\/weights\/.+\.rs$ - ^substrate\/frame\/.+\.md$ min_approvals: 2 - excludeAuthors: + allowedToSkipRule: teams: - core-devs teams: diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index fcb24ac..91e6cb0 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -283,16 +283,16 @@ describe("Config Parsing", () => { exclude: - 'example' type: basic - excludeAuthors: + allowedToSkipRule: teams: - team-example teams: - team-example `); const config = await runner.getConfigFile(""); - const { excludeAuthors } = config.rules[0]; - expect(excludeAuthors?.teams).toEqual(["team-example"]); - expect(excludeAuthors?.users).toBeUndefined(); + const { allowedToSkipRule } = config.rules[0]; + expect(allowedToSkipRule?.teams).toEqual(["team-example"]); + expect(allowedToSkipRule?.users).toBeUndefined(); }); test("should get users", async () => { @@ -305,16 +305,16 @@ describe("Config Parsing", () => { exclude: - 'example' type: basic - excludeAuthors: + allowedToSkipRule: users: - user-example teams: - team-example `); const config = await runner.getConfigFile(""); - const { excludeAuthors } = config.rules[0]; - expect(excludeAuthors?.users).toEqual(["user-example"]); - expect(excludeAuthors?.teams).toBeUndefined(); + const { allowedToSkipRule } = config.rules[0]; + expect(allowedToSkipRule?.users).toEqual(["user-example"]); + expect(allowedToSkipRule?.teams).toBeUndefined(); }); test("should get teams and users", async () => { @@ -327,7 +327,7 @@ describe("Config Parsing", () => { exclude: - 'example' type: basic - excludeAuthors: + allowedToSkipRule: teams: - team-example users: @@ -336,9 +336,9 @@ describe("Config Parsing", () => { - team-example `); const config = await runner.getConfigFile(""); - const { excludeAuthors } = config.rules[0]; - expect(excludeAuthors?.teams).toEqual(["team-example"]); - expect(excludeAuthors?.users).toEqual(["user-example"]); + const { allowedToSkipRule } = config.rules[0]; + expect(allowedToSkipRule?.teams).toEqual(["team-example"]); + expect(allowedToSkipRule?.users).toEqual(["user-example"]); }); test("should be null by default", async () => { @@ -355,7 +355,7 @@ describe("Config Parsing", () => { - team-example `); const config = await runner.getConfigFile(""); - expect(config.rules[0].excludeAuthors).toBeUndefined(); + expect(config.rules[0].allowedToSkipRule).toBeUndefined(); }); }); }); diff --git a/src/test/runner/runner.test.ts b/src/test/runner/runner.test.ts index 404ee14..bd22293 100644 --- a/src/test/runner/runner.test.ts +++ b/src/test/runner/runner.test.ts @@ -32,15 +32,15 @@ describe("Shared validations", () => { expect(evaluation).toBeTruthy(); }); - test("validatePullRequest should return true if author belongs to excludeAuthors", async () => { + test("validatePullRequest should return true if author belongs to allowedToSkipRule", async () => { const config: ConfigurationFile = { rules: [ { - name: "Rule excludeAuthors", + name: "Rule allowedToSkipRule", type: RuleTypes.Basic, condition: { include: ["src"] }, min_approvals: 1, - excludeAuthors: { teams: ["abc"] }, + allowedToSkipRule: { teams: ["abc"] }, }, ], }; @@ -49,7 +49,9 @@ describe("Shared validations", () => { api.getAuthor.mockReturnValue("user-1"); const evaluation = await runner.validatePullRequest(config); expect(evaluation).toBeTruthy(); - expect(logger.info).toHaveBeenCalledWith("Skipping rule Rule excludeAuthors as author belong to greenlight rule."); + expect(logger.info).toHaveBeenCalledWith( + "Skipping rule Rule allowedToSkipRule as author belong to greenlight rule.", + ); }); test("fetchAllUsers should not return duplicates", async () => { From 7faf0282501d1d07f4e3196f3a716923ecbbe000 Mon Sep 17 00:00:00 2001 From: Javier Bullrich Date: Tue, 5 Sep 2023 19:21:40 -0300 Subject: [PATCH 14/14] fixed missing replacements --- README.md | 2 +- src/test/rules/config.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4fd5b2a..585ab21 100644 --- a/README.md +++ b/README.md @@ -262,7 +262,7 @@ It has the same parameters as a normal rule: - **countAuthor**: If the pull request author should be considered as an approval. - If the author belongs to the list of approved users (either by team or by users) his approval will be counted (requiring one less approvals in total). - ** Optional**: Defaults to `false` -- **excludeAuthor**: If the author belong to one of the teams and/or users in the list, the rule should be skipped. +- **allowedToSkipRule**: If the author belong to one of the teams and/or users in the list, the rule should be skipped. - **Optional**. - This is useful for cases where we want to make sure that some eyes look into a PR, but for we don’t need to ensure that much security on internal teams. - For example, if someone modifies a CI file, we want to make sure they didn’t break anything. Unless it’s someone from the CI team. They *should know* what they are doing. diff --git a/src/test/rules/config.test.ts b/src/test/rules/config.test.ts index 91e6cb0..0e63a60 100644 --- a/src/test/rules/config.test.ts +++ b/src/test/rules/config.test.ts @@ -272,7 +272,7 @@ describe("Config Parsing", () => { expect(config.rules[0].condition.exclude).toBeUndefined(); }); - describe("excludeAuthor", () => { + describe("allowedToSkipRule", () => { test("should get teams", async () => { api.getConfigFile.mockResolvedValue(` rules: