Merge pull request #301 from riley-kohler/fix_create_mirror_error_han…

…dling_overwriting_initial_error fix: create mirror error handling overwriting initial error
github-community-projects · Jan 21, 2025 · bed6b25 · bed6b25
2 parents 0e133c5 + a91851e
commit bed6b25
Show file tree

Hide file tree

Showing 2 changed files with 108 additions and 83 deletions.
diff --git a/src/server/repos/controller.ts b/src/server/repos/controller.ts
@@ -28,6 +28,10 @@ export const createMirrorHandler = async ({
 }: {
   input: CreateMirrorSchema
 }) => {
+  // Use to track mirror creation for cleanup in case of errors
+  let privateOctokit
+  let newRepo
+
   try {
     reposApiLogger.info('createMirror', { input: input })
 
@@ -41,7 +45,7 @@ export const createMirrorHandler = async ({
     const contributionOctokit = octokitData.contribution.octokit
     const contributionAccessToken = octokitData.contribution.accessToken
 
-    const privateOctokit = octokitData.private.octokit
+    privateOctokit = octokitData.private.octokit
     const privateInstallationId = octokitData.private.installationId
     const privateAccessToken = octokitData.private.accessToken
 
@@ -79,95 +83,85 @@ export const createMirrorHandler = async ({
         }
       })
 
-    try {
-      const forkData = await contributionOctokit.rest.repos.get({
-        owner: input.forkRepoOwner,
-        repo: input.forkRepoName,
-      })
+    const forkData = await contributionOctokit.rest.repos.get({
+      owner: input.forkRepoOwner,
+      repo: input.forkRepoName,
+    })
 
-      // Now create a temporary directory to clone the repo into
-      const tempDir = temporaryDirectory()
-
-      const options: Partial<SimpleGitOptions> = {
-        config: [
-          `user.name=pma[bot]`,
-          // We want to use the private installation ID as the email so that we can push to the private repo
-          `user.email=${privateInstallationId}+pma[bot]@users.noreply.github.com`,
-          // Disable any global git hooks to prevent potential interference when running the app locally
-          'core.hooksPath=/dev/null',
-        ],
-      }
-      const git = simpleGit(tempDir, options)
-      const remote = generateAuthUrl(
-        contributionAccessToken,
-        input.forkRepoOwner,
-        input.forkRepoName,
-      )
+    // Now create a temporary directory to clone the repo into
+    const tempDir = temporaryDirectory()
+
+    const options: Partial<SimpleGitOptions> = {
+      config: [
+        `user.name=pma[bot]`,
+        // We want to use the private installation ID as the email so that we can push to the private repo
+        `user.email=${privateInstallationId}+pma[bot]@users.noreply.github.com`,
+        // Disable any global git hooks to prevent potential interference when running the app locally
+        'core.hooksPath=/dev/null',
+      ],
+    }
+    const git = simpleGit(tempDir, options)
+    const remote = generateAuthUrl(
+      contributionAccessToken,
+      input.forkRepoOwner,
+      input.forkRepoName,
+    )
 
-      await git.clone(remote, tempDir)
+    await git.clone(remote, tempDir)
 
-      // Get the organization custom properties
-      const orgCustomProps =
-        await privateOctokit.rest.orgs.getAllCustomProperties({
-          org: privateOrg,
-        })
-
-      // Creates custom property fork in the org if it doesn't exist
-      if (
-        !orgCustomProps.data.some(
-          (prop: { property_name: string }) => prop.property_name === 'fork',
-        )
-      ) {
-        await privateOctokit.rest.orgs.createOrUpdateCustomProperty({
-          org: privateOrg,
-          custom_property_name: 'fork',
-          value_type: 'string',
-        })
-      }
+    // Get the organization custom properties
+    const orgCustomProps =
+      await privateOctokit.rest.orgs.getAllCustomProperties({
+        org: privateOrg,
+      })
 
-      // This repo needs to be created in the private org
-      const newRepo = await privateOctokit.rest.repos.createInOrg({
-        name: input.newRepoName,
+    // Creates custom property fork in the org if it doesn't exist
+    if (
+      !orgCustomProps.data.some(
+        (prop: { property_name: string }) => prop.property_name === 'fork',
+      )
+    ) {
+      await privateOctokit.rest.orgs.createOrUpdateCustomProperty({
         org: privateOrg,
-        private: true,
-        description: `Mirror of ${input.forkRepoOwner}/${input.forkRepoName}`,
-        custom_properties: {
-          fork: `${input.forkRepoOwner}/${input.forkRepoName}`,
-        },
+        custom_property_name: 'fork',
+        value_type: 'string',
       })
+    }
 
-      const defaultBranch = forkData.data.default_branch
+    // This repo needs to be created in the private org
+    newRepo = await privateOctokit.rest.repos.createInOrg({
+      name: input.newRepoName,
+      org: privateOrg,
+      private: true,
+      description: `Mirror of ${input.forkRepoOwner}/${input.forkRepoName}`,
+      custom_properties: {
+        fork: `${input.forkRepoOwner}/${input.forkRepoName}`,
+      },
+    })
 
-      // Add the mirror remote
-      const upstreamRemote = generateAuthUrl(
-        privateAccessToken,
-        newRepo.data.owner.login,
-        newRepo.data.name,
-      )
-      await git.addRemote('upstream', upstreamRemote)
-      await git.push('upstream', defaultBranch)
+    const defaultBranch = forkData.data.default_branch
 
-      // Create a new branch on both
-      await git.checkoutBranch(input.newBranchName, defaultBranch)
-      await git.push('origin', input.newBranchName)
+    // Add the mirror remote
+    const upstreamRemote = generateAuthUrl(
+      privateAccessToken,
+      newRepo.data.owner.login,
+      newRepo.data.name,
+    )
+    await git.addRemote('upstream', upstreamRemote)
+    await git.push('upstream', defaultBranch)
 
-      reposApiLogger.info('Mirror created', {
-        org: newRepo.data.owner.login,
-        name: newRepo.data.name,
-      })
+    // Create a new branch on both
+    await git.checkoutBranch(input.newBranchName, defaultBranch)
+    await git.push('origin', input.newBranchName)
 
-      return {
-        success: true,
-        data: newRepo.data,
-      }
-    } catch (error) {
-      // Clean up the private mirror repo made
-      await privateOctokit.rest.repos.delete({
-        owner: privateOrg,
-        repo: input.newRepoName,
-      })
+    reposApiLogger.info('Mirror created', {
+      org: newRepo.data.owner.login,
+      name: newRepo.data.name,
+    })
 
-      throw error
+    return {
+      success: true,
+      data: newRepo.data,
     }
   } catch (error) {
     reposApiLogger.error('Error creating mirror', { error })
@@ -178,6 +172,18 @@ export const createMirrorHandler = async ({
       (error as Error)?.message ??
       'An error occurred'
 
+    if (privateOctokit && newRepo) {
+      try {
+        // Clean up the private mirror repo made
+        await privateOctokit.rest.repos.delete({
+          owner: newRepo.data.owner.login,
+          repo: input.newRepoName,
+        })
+      } catch (deleteError) {
+        reposApiLogger.error('Failed to delete mirror', { deleteError })
+      }
+    }
+
     throw new TRPCError({
       code: 'INTERNAL_SERVER_ERROR',
       message,

diff --git a/test/server/repos.test.ts b/test/server/repos.test.ts
@@ -199,9 +199,18 @@ describe('Repos router', () => {
     om.mockFunctions.rest.orgs.get.mockResolvedValue(fakeOrg)
     om.mockFunctions.rest.repos.get.mockResolvedValueOnce(repoNotFound)
     om.mockFunctions.rest.repos.get.mockResolvedValueOnce(fakeMirrorRepo)
-    om.mockFunctions.rest.repos.delete.mockResolvedValue({})
+    stubbedGit.clone.mockResolvedValue({})
+    om.mockFunctions.rest.orgs.getAllCustomProperties.mockResolvedValue({
+      data: [{ fork: 'test' }],
+    })
+    om.mockFunctions.rest.repos.createInOrg.mockResolvedValue({
+      data: { owner: { login: 'github' } },
+    })
 
-    stubbedGit.clone.mockRejectedValue(new Error('clone error'))
+    // error after repo creation so that cleanup can be tested
+    stubbedGit.addRemote.mockRejectedValue(new Error('error adding remote'))
+
+    om.mockFunctions.rest.repos.delete.mockResolvedValue({})
 
     await caller
       .createMirror({
@@ -213,7 +222,7 @@ describe('Repos router', () => {
         newRepoName: 'test',
       })
       .catch((error) => {
-        expect(error.message).toEqual('clone error')
+        expect(error.message).toEqual('error adding remote')
       })
 
     expect(configSpy).toHaveBeenCalledTimes(1)
@@ -239,9 +248,19 @@ describe('Repos router', () => {
     om.mockFunctions.rest.orgs.get.mockResolvedValue(fakeOrg)
     om.mockFunctions.rest.repos.get.mockResolvedValueOnce(repoNotFound)
     om.mockFunctions.rest.repos.get.mockResolvedValueOnce(fakeMirrorRepo)
+    stubbedGit.clone.mockResolvedValue({})
+    om.mockFunctions.rest.orgs.getAllCustomProperties.mockResolvedValue({
+      data: [{ fork: 'test' }],
+    })
+    om.mockFunctions.rest.repos.createInOrg.mockResolvedValue({
+      data: { owner: { login: 'github-test' } },
+    })
     om.mockFunctions.rest.repos.delete.mockResolvedValue({})
 
-    stubbedGit.clone.mockRejectedValue(new Error('clone error'))
+    // error after repo creation so that cleanup can be tested
+    stubbedGit.addRemote.mockRejectedValue(new Error('error adding remote'))
+
+    om.mockFunctions.rest.repos.delete.mockResolvedValue({})
 
     await caller
       .createMirror({
@@ -253,7 +272,7 @@ describe('Repos router', () => {
         newRepoName: 'test',
       })
       .catch((error) => {
-        expect(error.message).toEqual('clone error')
+        expect(error.message).toEqual('error adding remote')
       })
 
     expect(configSpy).toHaveBeenCalledTimes(1)