Improve validation of target branches (#325)

src/backportRun.ts

@@ -5,6 +5,7 @@ import { getCommits } from './lib/getCommits';
 import { getTargetBranches } from './lib/getTargetBranches';
 import { createStatusComment } from './lib/github/v3/createStatusComment';
 import { GithubV4Exception } from './lib/github/v4/apiRequestV4';
+import { validateTargetBranches } from './lib/github/v4/validateTargetBranches';
 import { consoleLog, initLogger } from './lib/logger';
 import { ora } from './lib/ora';
 import { setupRepo } from './lib/setupRepo';
@@ -95,7 +96,10 @@ export async function backportRun({
     const targetBranches = await getTargetBranches(options, commits);
     logger.info('Target branches', targetBranches);
 
-    await setupRepo(options);
+    await Promise.all([
+      setupRepo(options),
+      validateTargetBranches(options, targetBranches),
+    ]);
 
     const results = await runSequentially({
       options,

src/entrypoint.module.private.test.ts

@@ -57,6 +57,31 @@ describe('entrypoint.module', () => {
       });
     });
 
+    describe('when target branch in branchLabelMapping is invalid', () => {
+      let response: BackportFailureResponse;
+      beforeAll(async () => {
+        response = (await backportRun({
+          options: {
+            accessToken,
+            branchLabelMapping: {
+              [`^backport-to-(.+)$`]: '$1',
+            },
+            interactive: false,
+            pullNumber: 1,
+            repoName: 'repo-with-invalid-target-branch-label',
+            repoOwner: 'backport-org',
+          },
+        })) as BackportFailureResponse;
+      });
+
+      it('should correct error code', async () => {
+        expect(response.status).toBe('failure');
+        expect(response.error.message).toBe(
+          'The branch "--foo" does not exist'
+        );
+      });
+    });
+
     describe('when missing branches to backport to', () => {
       let response: BackportFailureResponse;
       beforeAll(async () => {

src/lib/github/v4/validateTargetBranches.private.test.ts

@@ -1,3 +1,4 @@
+import { ValidConfigOptions } from '../../../options/options';
 import { getDevAccessToken } from '../../../test/private/getDevAccessToken';
 import { validateTargetBranches } from './validateTargetBranches';
 
@@ -10,10 +11,12 @@ describe('validateTargetBranches', () => {
         repoOwner: 'backport-org',
         repoName: 'repo-with-target-branches',
         accessToken,
-        targetBranches: [],
-      };
+      } as ValidConfigOptions;
+      const targetBranches: string[] = [];
 
-      expect(await validateTargetBranches(options)).toEqual(undefined);
+      expect(await validateTargetBranches(options, targetBranches)).toEqual(
+        undefined
+      );
     });
   });
 
@@ -23,12 +26,12 @@ describe('validateTargetBranches', () => {
         repoOwner: 'backport-org',
         repoName: 'repo-with-target-branches',
         accessToken,
-        targetBranches: ['production', 'foo'],
-      };
+      } as ValidConfigOptions;
+      const targetBranches = ['production', 'foo'];
 
-      await expect(() => validateTargetBranches(options)).rejects.toThrowError(
-        'The branch "foo" does not exist'
-      );
+      await expect(() =>
+        validateTargetBranches(options, targetBranches)
+      ).rejects.toThrowError('The branch "foo" does not exist');
     });
   });
 
@@ -38,10 +41,12 @@ describe('validateTargetBranches', () => {
         repoOwner: 'backport-org',
         repoName: 'repo-with-target-branches',
         accessToken,
-        targetBranches: ['production', 'staging'],
-      };
+      } as ValidConfigOptions;
+      const targetBranches = ['production', 'staging'];
 
-      expect(await validateTargetBranches(options)).toEqual(undefined);
+      expect(await validateTargetBranches(options, targetBranches)).toEqual(
+        undefined
+      );
     });
   });
 
@@ -51,12 +56,12 @@ describe('validateTargetBranches', () => {
         repoOwner: 'backport-org',
         repoName: 'repo-with-target-branches',
         accessToken,
-        targetBranches: ['foo', 'bar'],
-      };
+      } as ValidConfigOptions;
+      const targetBranches = ['foo', 'bar'];
 
-      await expect(() => validateTargetBranches(options)).rejects.toThrow(
-        /The branch "(foo|bar)" does not exist/
-      );
+      await expect(() =>
+        validateTargetBranches(options, targetBranches)
+      ).rejects.toThrow(/The branch "(foo|bar)" does not exist/);
     });
   });
 });

src/lib/github/v4/validateTargetBranches.ts

@@ -1,4 +1,5 @@
 import gql from 'graphql-tag';
+import { ValidConfigOptions } from '../../../options/options';
 import { BackportError } from '../../BackportError';
 import { apiRequestV4 } from './apiRequestV4';
 
@@ -47,19 +48,10 @@ async function fetchTargetBranch({
   return res.repository.ref;
 }
 
-export async function validateTargetBranches({
-  accessToken,
-  repoName,
-  repoOwner,
-  targetBranches = [],
-  githubApiBaseUrlV4,
-}: {
-  accessToken: string;
-  repoOwner: string;
-  repoName: string;
-  targetBranches?: string[];
-  githubApiBaseUrlV4?: string;
-}) {
+export async function validateTargetBranches(
+  { accessToken, repoName, repoOwner, githubApiBaseUrlV4 }: ValidConfigOptions,
+  targetBranches: string[]
+) {
   await Promise.all(
     targetBranches.map((targetBranch) => {
       return fetchTargetBranch({

src/options/options.ts

@@ -6,7 +6,6 @@ import {
   OptionsFromGithub,
 } from '../lib/github/v4/getOptionsFromGithub/getOptionsFromGithub';
 import { getRepoOwnerAndNameFromGitRemotes } from '../lib/github/v4/getRepoOwnerAndNameFromGitRemotes';
-import { validateTargetBranches } from '../lib/github/v4/validateTargetBranches';
 import { setAccessToken } from '../lib/logger';
 import { ConfigFileOptions, TargetBranchChoiceOrString } from './ConfigOptions';
 import { OptionsFromCliArgs } from './cliArgs';
@@ -116,10 +115,6 @@ export async function getOptions({
 
   throwForRequiredOptions(options);
 
-  if (options.targetBranches.length > 0) {
-    await validateTargetBranches(options);
-  }
-
   return options;
 }