Improve protected branch plugin

plugins/protected-branch/__tests__/protected-branch.test.ts

@@ -1,39 +1,45 @@
 import * as Auto from "@auto-it/core";
+import Git from "@auto-it/core/dist/git";
 import { dummyLog } from "@auto-it/core/dist/utils/logger";
 import { makeHooks } from "@auto-it/core/dist/utils/make-hooks";
+
 import ProtectedBranchPlugin from "../src";
 
-const execPromise = jest.fn();
+const exec = jest.fn();
 jest.mock(
-    "../../../packages/core/dist/utils/exec-promise",
-    () => (...args: any[]) => execPromise(...args),
+    "@auto-it/core/dist/utils/exec-promise",
+    () =>
+        (...args: unknown[]) =>
+            exec(...args)
 );
 
+jest.mock("@auto-it/core/dist/git");
+const mockAutoGit = Git as unknown as jest.SpyInstance;
+
 describe("Protected-Branch Plugin", () => {
   const mockGetSha = jest.fn();
   const mockCreateCheck = jest.fn();
   const mockCreatePr = jest.fn();
   const mockApprovePr = jest.fn();
 
   function setupProtectedBranchPlugin(
-    checkEnv?: jest.SpyInstance,
-    withoutGit = false
+      checkEnv?: jest.SpyInstance,
+      withoutGit = false
   ): { plugin: ProtectedBranchPlugin; hooks: Auto.IAutoHooks } {
-    const plugin = new ProtectedBranchPlugin({ reviewerToken: "token" });
+    const plugin = new ProtectedBranchPlugin({ reviewerToken: "reviewerToken" });
     const hooks = makeHooks();
 
     plugin.apply(({
       hooks,
       checkEnv,
       git: withoutGit
-        ? undefined
-        : {
+          ? undefined
+          : {
             getSha: mockGetSha,
             github: {
               checks: { create: mockCreateCheck },
               pulls: {
                 create: mockCreatePr,
-                createReview: mockApprovePr,
               },
             },
             options: {
@@ -50,10 +56,11 @@ describe("Protected-Branch Plugin", () => {
   }
 
   beforeEach(() => {
-    execPromise.mockReset();
+    exec.mockReset();
     mockGetSha.mockReset().mockResolvedValueOnce("sha");
     mockCreateCheck.mockReset();
     mockCreatePr.mockReset().mockResolvedValueOnce({ data: { number: 42 } });
+    mockAutoGit.mockReset();
     mockApprovePr.mockReset();
   });
 
@@ -68,16 +75,10 @@ describe("Protected-Branch Plugin", () => {
   describe("validateConfig", () => {
     test("should validate the configuration", async () => {
       const { hooks, plugin } = setupProtectedBranchPlugin();
-      await expect(
-        hooks.validateConfig.promise("not-me", {})
-      ).resolves.toBeUndefined();
-      await expect(
-        hooks.validateConfig.promise(plugin.name, {})
-      ).resolves.toStrictEqual([]);
-
-      const res = await hooks.validateConfig.promise(plugin.name, {
-        invalidKey: "value",
-      });
+      await expect(hooks.validateConfig.promise("not-me", {})).resolves.toBeUndefined();
+      await expect(hooks.validateConfig.promise(plugin.name, {})).resolves.toStrictEqual([]);
+
+      const res = await hooks.validateConfig.promise(plugin.name, { invalidKey: "value" });
       expect(res).toHaveLength(1);
       // eslint-disable-next-line @typescript-eslint/prefer-optional-chain
       expect(res && res[0]).toContain(plugin.name);
@@ -94,18 +95,17 @@ describe("Protected-Branch Plugin", () => {
       const { hooks } = setupProtectedBranchPlugin(checkEnv);
       await hooks.beforeRun.promise({
         plugins: [["protected-branch", {}]],
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
       } as any);
-      expect(checkEnv).toHaveBeenCalledWith(
-        "protected-branch",
-        "PROTECTED_BRANCH_REVIEWER_TOKEN"
-      );
+      expect(checkEnv).toHaveBeenCalledWith("protected-branch", "PROTECTED_BRANCH_REVIEWER_TOKEN");
     });
 
     test("shouldn't check env with image", async () => {
       const checkEnv = jest.fn();
       const { hooks } = setupProtectedBranchPlugin(checkEnv);
       await hooks.beforeRun.promise({
         plugins: [["protected-branch", { reviewerToken: "token" }]],
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
       } as any);
       expect(checkEnv).not.toHaveBeenCalled();
     });
@@ -118,8 +118,16 @@ describe("Protected-Branch Plugin", () => {
       repo: "my-repo",
     };
 
+    function doMockPrApproval() {
+      mockAutoGit.mockReturnValueOnce({
+        github: {
+          pulls: { createReview: mockApprovePr },
+        },
+      });
+    }
+
     function expectCreateRemoteBranch(): void {
-      expect(execPromise).toHaveBeenNthCalledWith(1, "git", [
+      expect(exec).toHaveBeenNthCalledWith(1, "git", [
         "push",
         "--set-upstream",
         "remote",
@@ -146,49 +154,53 @@ describe("Protected-Branch Plugin", () => {
         head: "automatic-release-sha",
         title: "Automatic release",
       });
-      expect(execPromise).toHaveBeenNthCalledWith(2, "gh", [
-        "api",
-        "/repos/TheOwner/my-repo/pulls/42/reviews",
-        "-X",
-        "POST",
-        "-F",
-        "commit_id=sha",
-        "-F",
-        `event=APPROVE`,
-      ]);
+      expect(mockAutoGit).toHaveBeenCalledWith(
+          expect.objectContaining({
+            token: "reviewerToken",
+          }),
+          expect.anything()
+      );
+      expect(mockApprovePr).toHaveBeenCalledWith({
+        ...commonGitArgs,
+        pull_number: 42,
+        commit_id: "sha",
+        event: "APPROVE",
+      });
     }
 
     test("should do nothing without git", async () => {
       const { hooks } = setupProtectedBranchPlugin(undefined, true);
 
       await expect(hooks.publish.promise(options)).resolves.toBeUndefined();
 
-      expect(execPromise).not.toHaveBeenCalled();
+      expect(exec).not.toHaveBeenCalled();
       expect(mockGetSha).not.toHaveBeenCalled();
       expect(mockCreateCheck).not.toHaveBeenCalled();
       expect(mockCreatePr).not.toHaveBeenCalled();
-      expect(mockApprovePr).not.toHaveBeenCalled();
+      expect(mockAutoGit).not.toHaveBeenCalled();
     });
 
     test("should do nothing without reviewerToken", async () => {
       const { hooks, plugin } = setupProtectedBranchPlugin();
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
       (plugin as any).options.reviewerToken = undefined;
 
       await expect(hooks.publish.promise(options)).resolves.toBeUndefined();
 
-      expect(execPromise).not.toHaveBeenCalled();
+      expect(exec).not.toHaveBeenCalled();
       expect(mockGetSha).not.toHaveBeenCalled();
       expect(mockCreateCheck).not.toHaveBeenCalled();
       expect(mockCreatePr).not.toHaveBeenCalled();
-      expect(mockApprovePr).not.toHaveBeenCalled();
+      expect(mockAutoGit).not.toHaveBeenCalled();
     });
 
     test("should handle all branch protections", async () => {
       const { hooks } = setupProtectedBranchPlugin();
+      doMockPrApproval();
 
       await expect(hooks.publish.promise(options)).resolves.toBeUndefined();
 
-      expect(execPromise).toHaveBeenCalledTimes(2);
+      expect(exec).toHaveBeenCalledTimes(1);
       expectCreateRemoteBranch();
       expectHandleBranchProtections([]);
     });
@@ -197,25 +209,30 @@ describe("Protected-Branch Plugin", () => {
       const ciChecks = ["ci", "release"];
 
       const { hooks, plugin } = setupProtectedBranchPlugin();
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
       (plugin as any).options.requiredStatusChecks = ciChecks;
 
+      doMockPrApproval();
+
       await expect(hooks.publish.promise(options)).resolves.toBeUndefined();
 
-      expect(execPromise).toHaveBeenCalledTimes(2);
+      expect(exec).toHaveBeenCalledTimes(1);
       expectCreateRemoteBranch();
       expectHandleBranchProtections(ciChecks);
     });
 
     test("should silently cleanup remote stuff", async () => {
       const { hooks } = setupProtectedBranchPlugin();
-      execPromise
-        .mockResolvedValueOnce("")
-        .mockResolvedValueOnce("")
-        .mockRejectedValueOnce(new Error("couldn't delete remote branch"));
+      exec
+          .mockResolvedValueOnce("")
+          .mockResolvedValueOnce("")
+          .mockRejectedValueOnce(new Error("couldn't delete remote branch"));
+
+      doMockPrApproval();
 
       await expect(hooks.publish.promise(options)).resolves.toBeUndefined();
 
-      expect(execPromise).toHaveBeenCalledTimes(2);
+      expect(exec).toHaveBeenCalledTimes(1);
       expectCreateRemoteBranch();
       expectHandleBranchProtections([]);
     });

plugins/protected-branch/src/GitOperator.ts

@@ -71,28 +71,25 @@ export class GitOperator {
     pull_number: number,
     sha: string
   ): Promise<void> {
-    const oldToken = process.env.GITHUB_TOKEN;
-    try {
-      this.logger.verbose.info("Approving PullRequest using:\n", {
-        pull_number,
-        sha,
-      });
-
-      process.env.GITHUB_TOKEN = token;
-      await execPromise("gh", [
-        "api",
-        `/repos/${this.git.options.owner}/${this.git.options.repo}/pulls/${pull_number}/reviews`,
-        "-X",
-        "POST",
-        "-F",
-        `commit_id=${sha}`,
-        "-F",
-        `event=APPROVE`,
-      ]);
-    } finally {
-      process.env.GITHUB_TOKEN = oldToken;
-    }
-
-    this.logger.verbose.info("Approve Pull Request on GitHub.");
+    const approverGit = new Git(
+        {
+          ...this.git.options,
+          token,
+        },
+        this.logger
+    );
+
+    this.logger.verbose.info('Approving PullRequest using:\n', { pull_number, sha });
+    const params: RestEndpointMethodTypes['pulls']['createReview']['parameters'] = {
+      owner: this.git.options.owner,
+      repo: this.git.options.repo,
+      pull_number,
+      commit_id: sha,
+      event: 'APPROVE',
+    };
+
+    await approverGit.github.pulls.createReview(params);
+
+    this.logger.verbose.info('Approve PullRequest on GitHub.');
   }
 }

plugins/protected-branch/src/index.ts

@@ -111,11 +111,18 @@ export default class ProtectedBranchPlugin implements IPlugin {
 
       auto.logger.log.info("Delete release branch 🗑️ ");
 
-      await auto.git.github.git.deleteRef({
-        owner: auto.git.options.owner,
-        repo: auto.git.options.repo,
-        ref: `heads/${headBranch}`,
-      });
+      try {
+        await auto.git.github.git.deleteRef({
+          owner: auto.git.options.owner,
+          repo: auto.git.options.repo,
+          ref: `heads/${headBranch}`,
+        });
+      } catch (e) {
+        // Silently ignore reference not found error since the ref might already be deleted
+        if (!e || e.status !== 422 || e.message !== "Reference does not exist") {
+          throw e;
+        }
+      }
     });
   }
 }