Added feature to request reviewers (#97)

Added the feature to request reviewers. For this we are using a github
app with pull request write permission (I need to request our app to be
updated).

I will be making a following PR to merge the keys as we are using the
same key for all the github classes.

The request feature is disabled by default.

Resolves #53

---------

Co-authored-by: Maksym H <1177472+mordamax@users.noreply.github.com>

README.md

@@ -87,6 +87,7 @@ on:
 permissions:
   contents: read
   checks: write
+  pull-requests: write
 
 jobs:
   review-approvals:
@@ -103,6 +104,7 @@ jobs:
           repo-token: ${{ github.token }}
           team-token: ${{ secrets.TEAM_TOKEN }}
           checks-token: ${{ secrets.CHECKS_TOKEN }}
+          request-reviewers: false
           pr-number: ${{ steps.number.outputs.content }}
 ```
 Create a new PR and see if it is working.
@@ -149,6 +151,10 @@ You can find all the inputs in [the action file](./action.yml), but let's walk t
 			- You can use the same GitHub app for `checks-token` and `team-token`.
 - `config-file`: The location of the config file.
 	- **default**: `.github/review-bot.yml`
+- `request-reviewers`: If the system should automatically request the required reviewers.
+	- **default**: false.
+	- If enabled, when there are missing reviews, the system will request the appropriate users and/or team to review. (Note: It won't assign fellowship members as reviewers)
+	- If enabled, and using teams, this requires a GitHub action with `write` permission for `pull request`.
 
 #### Using a GitHub app instead of a PAT
 In some cases, specially in big organizations, it is more organized to use a GitHub app to authenticate, as it allows us to give it permissions per repository, and we can fine-grain them even better. If you wish to do that, you need to create a GitHub app with the following permissions:

action.yml

@@ -21,6 +21,9 @@ inputs:
   pr-number:
     description: 'The number of the pull request to review. Required if event is `workflow_run`'
     required: false
+  request-reviewers:
+    description: If the system should automatically request the required reviewers.
+    required: false
 outputs:
   repo:
     description: 'The name of the repo in owner/repo pattern'

src/github/pullRequest.ts

@@ -1,9 +1,13 @@
 import { PullRequest, PullRequestReview } from "@octokit/webhooks-types";
 
+import { Reviewers } from "../rules/types";
 import { caseInsensitiveEqual } from "../util";
 import { ActionLogger, GitHubClient } from "./types";
 
-/** API class that uses the default token to access the data from the pull request and the repository */
+/** API class that uses the default token to access the data from the pull request and the repository
+ * If we are using the assign reviewers features with teams, it requires a GitHub app
+ * (Action token doesn't have permission to assign teams)
+ */
 export class PullRequestApi {
   private readonly number: number;
   private readonly repoInfo: { repo: string; owner: string };
@@ -107,6 +111,27 @@ export class PullRequestApi {
     return approvals;
   }
 
+  async requestReview({ users, teams }: Pick<Reviewers, "users" | "teams">): Promise<void> {
+    if (users || teams) {
+      const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0;
+      const reviewersLog = [
+        validArray(users) ? `Users: ${JSON.stringify(users)}` : undefined,
+        validArray(teams) ? `Teams: ${JSON.stringify(teams)}` : undefined,
+      ]
+        .filter((e) => !!e)
+        .join(" - ");
+
+      this.logger.info(`Requesting reviews from ${reviewersLog}`);
+
+      await this.api.rest.pulls.requestReviewers({
+        ...this.repoInfo,
+        pull_number: this.number,
+        reviewers: users,
+        team_reviewers: teams,
+      });
+    }
+  }
+
   /** Returns the login of the PR's author */
   getAuthor(): string {
     return this.pr.user.login;

src/index.ts

@@ -15,6 +15,8 @@ export interface Inputs {
   configLocation: string;
   /** GitHub's action default secret */
   repoToken: string;
+  /** Should automatically request missing reviewers */
+  requestReviewers?: boolean;
   /** A custom access token with the read:org access */
   teamApiToken: string;
   /** Number of the PR to analyze. Optional when it is triggered by `pull_request` event */
@@ -38,10 +40,11 @@ const getRepo = (ctx: Context) => {
 const getInputs = (): Inputs => {
   const configLocation = getInput("config-file");
   const repoToken = getInput("repo-token", { required: true });
+  const requestReviewers = !!getInput("request-reviewers", { required: false });
   const teamApiToken = getInput("team-token", { required: true });
   const prNumber = getInput("pr-number");
 
-  return { configLocation, repoToken, teamApiToken, prNumber: prNumber ? parseInt(prNumber) : null };
+  return { configLocation, requestReviewers, repoToken, teamApiToken, prNumber: prNumber ? parseInt(prNumber) : null };
 };
 
 const repo = getRepo(context);

src/runner.ts

@@ -178,7 +178,10 @@ export class ActionRunner {
   }
 
   /** WIP - Class that will assign the requests for review */
-  requestReviewers(reports: RuleReport[], preventReviewRequests: ConfigurationFile["preventReviewRequests"]): void {
+  async requestReviewers(
+    reports: RuleReport[],
+    preventReviewRequests: ConfigurationFile["preventReviewRequests"],
+  ): Promise<void> {
     if (reports.length === 0) {
       return;
     }
@@ -191,6 +194,8 @@ export class ActionRunner {
       finalReport.usersToRequest = concatArraysUniquely(finalReport.usersToRequest, report.usersToRequest);
     }
 
+    this.logger.debug(`Request data: ${JSON.stringify(finalReport)}`);
+
     let { teamsToRequest, usersToRequest } = finalReport;
 
     /**
@@ -214,13 +219,7 @@ export class ActionRunner {
       }
     }
 
-    const validArray = (array: string[] | undefined): boolean => !!array && array.length > 0;
-    const reviewersLog = [
-      validArray(teamsToRequest) ? `Teams: ${JSON.stringify(teamsToRequest)}` : "",
-      validArray(usersToRequest) ? `Users: ${JSON.stringify(usersToRequest)}` : "",
-    ].join(" - ");
-
-    this.logger.info(`Need to request reviews from ${reviewersLog}`);
+    await this.prApi.requestReview({ users: usersToRequest, teams: teamsToRequest });
   }
 
   /** Aggregates all the reports and generate a status report
@@ -556,7 +555,9 @@ export class ActionRunner {
    * 3. It generates a status check in the Pull Request
    * 4. WIP - It assigns the required reviewers to review the PR
    */
-  async runAction(inputs: Pick<Inputs, "configLocation">): Promise<Pick<CheckData, "conclusion"> & PullRequestReport> {
+  async runAction(
+    inputs: Pick<Inputs, "configLocation" | "requestReviewers">,
+  ): Promise<Pick<CheckData, "conclusion"> & PullRequestReport> {
     const config = await this.getConfigFile(inputs.configLocation);
 
     const prValidation = await this.validatePullRequest(config);
@@ -567,7 +568,11 @@ export class ActionRunner {
     const checkRunData = this.generateCheckRunData(reports);
     await this.checks.generateCheckRun(checkRunData);
 
-    this.requestReviewers(reports, config.preventReviewRequests);
+    if (inputs.requestReviewers) {
+      await this.requestReviewers(reports, config.preventReviewRequests);
+    } else {
+      this.logger.info("'request-reviewers' is disabled. Skipping the request.");
+    }
 
     setOutput("report", JSON.stringify(prValidation));
 

src/test/github.test.ts

@@ -147,4 +147,15 @@ describe("Pull Request API Tests", () => {
       expect(client.rest.pulls.listFiles).toHaveBeenCalledTimes(1);
     });
   });
+
+  test("Request reviewers", async () => {
+    await api.requestReview({ users: ["abc"], teams: ["team-abc"] });
+    expect(client.rest.pulls.requestReviewers).toHaveBeenCalledWith({
+      pull_number: 99,
+      owner: "org",
+      repo: pr.base.repo.name,
+      reviewers: ["abc"],
+      team_reviewers: ["team-abc"],
+    });
+  });
 });

src/test/runner/runner.test.ts

@@ -102,30 +102,27 @@ describe("Shared validations", () => {
       usersToRequest: ["user-1"],
     };
 
-    test("should request reviewers if object is not defined", () => {
-      runner.requestReviewers([exampleReport], undefined);
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    test("should request reviewers if object is not defined", async () => {
+      await runner.requestReviewers([exampleReport], undefined);
+      expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] });
     });
 
-    test("should not request user if he is defined", () => {
-      runner.requestReviewers([exampleReport], { users: ["user-1"] });
+    test("should not request user if he is defined", async () => {
+      await runner.requestReviewers([exampleReport], { users: ["user-1"] });
+
       expect(logger.info).toHaveBeenCalledWith("Filtering users to request a review from.");
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
-      expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+      expect(api.requestReview).toHaveBeenCalledWith({ teams: ["team-1"], users: [] });
     });
 
-    test("should not request team if it is defined", () => {
-      runner.requestReviewers([exampleReport], { teams: ["team-1"] });
+    test("should not request team if it is defined", async () => {
+      await runner.requestReviewers([exampleReport], { teams: ["team-1"] });
       expect(logger.info).toHaveBeenCalledWith("Filtering teams to request a review from.");
-      expect(logger.info).not.toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+      expect(api.requestReview).toHaveBeenCalledWith({ teams: [], users: ["user-1"] });
     });
 
-    test("should request reviewers if the team and user are not the same", () => {
-      runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] });
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["team-1"])));
-      expect(logger.info).toHaveBeenCalledWith(expect.stringContaining(JSON.stringify(["user-1"])));
+    test("should request reviewers if the team and user are not the same", async () => {
+      await runner.requestReviewers([exampleReport], { users: ["user-pi"], teams: ["team-alpha"] });
+      expect(api.requestReview).toHaveBeenCalledWith({ users: ["user-1"], teams: ["team-1"] });
     });
   });
 });