Added excludeAuthors feature (paritytech#69)

This adds the feature to exclude rules from being executed when the
author belongs to a particular team or list.
This resolves paritytech#66.

README.md

@@ -238,6 +238,11 @@ rules:
       - user-1
       - user-2
     countAuthor: true
+    allowedToSkipRule:
+      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`
+- **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.
 #### 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

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[] };
+  allowedToSkipRule?: Omit<Reviewers, "min_approvals">;
   countAuthor?: boolean;
 }
 

src/rules/validator.ts

@@ -24,6 +24,7 @@ const ruleSchema = Joi.object<Rule & { type: string }>().keys({
     include: Joi.array().items(Joi.string()).required(),
     exclude: Joi.array().items(Joi.string()).optional().allow(null),
   }),
+  allowedToSkipRule: Joi.object<Omit<Reviewers, "min_approvals">>().keys(reviewersObj).optional().or("users", "teams"),
   type: Joi.string().valid(RuleTypes.Basic, RuleTypes.And, RuleTypes.Or, RuleTypes.AndDistinct).required(),
 });
 

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.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.`);
+            continue;
+          }
         }
         if (rule.type === "basic") {
           const [result, missingData] = await this.evaluateCondition(rule, rule.countAuthor);
@@ -368,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.fetchAllUsers(rule);
+
     if (requiredUsers.length === 0) {
       throw new Error("No users have been found in the required reviewers");
     }
@@ -455,6 +452,30 @@ 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 fetchAllUsers(reviewers: Omit<Reviewers, "min_approvals">): Promise<string[]> {
+    const users: Set<string> = new Set<string>();
+    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

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
+    allowedToSkipRule:
+      teams:
+        - core-devs
+    teams:
+      - srlabs
+
   - name: Core developers
     countAuthor: true
     condition:

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({

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("allowedToSkipRule", () => {
+      test("should get teams", async () => {
+        api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Default review
+            condition:
+              include: 
+                  - '.*'
+              exclude: 
+                  - 'example'
+            type: basic
+            allowedToSkipRule:
+              teams:
+                - team-example
+            teams:
+              - team-example
+          `);
+        const config = await runner.getConfigFile("");
+        const { allowedToSkipRule } = config.rules[0];
+        expect(allowedToSkipRule?.teams).toEqual(["team-example"]);
+        expect(allowedToSkipRule?.users).toBeUndefined();
+      });
+
+      test("should get users", async () => {
+        api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Default review
+            condition:
+              include: 
+                  - '.*'
+              exclude: 
+                  - 'example'
+            type: basic
+            allowedToSkipRule:
+              users:
+                - user-example
+            teams:
+              - team-example
+          `);
+        const config = await runner.getConfigFile("");
+        const { allowedToSkipRule } = config.rules[0];
+        expect(allowedToSkipRule?.users).toEqual(["user-example"]);
+        expect(allowedToSkipRule?.teams).toBeUndefined();
+      });
+
+      test("should get teams and users", async () => {
+        api.getConfigFile.mockResolvedValue(`
+        rules:
+          - name: Default review
+            condition:
+              include: 
+                  - '.*'
+              exclude: 
+                  - 'example'
+            type: basic
+            allowedToSkipRule:
+              teams:
+                - team-example
+              users:
+                - user-example
+            teams:
+              - team-example
+          `);
+        const config = await runner.getConfigFile("");
+        const { allowedToSkipRule } = config.rules[0];
+        expect(allowedToSkipRule?.teams).toEqual(["team-example"]);
+        expect(allowedToSkipRule?.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].allowedToSkipRule).toBeUndefined();
+      });
+    });
   });
 });

src/test/runner/runner.test.ts

@@ -32,6 +32,34 @@ describe("Shared validations", () => {
     expect(evaluation).toBeTruthy();
   });
 
+  test("validatePullRequest should return true if author belongs to allowedToSkipRule", async () => {
+    const config: ConfigurationFile = {
+      rules: [
+        {
+          name: "Rule allowedToSkipRule",
+          type: RuleTypes.Basic,
+          condition: { include: ["src"] },
+          min_approvals: 1,
+          allowedToSkipRule: { teams: ["abc"] },
+        },
+      ],
+    };
+    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 allowedToSkipRule as author belong to greenlight rule.",
+    );
+  });
+
+  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"] } };