feat: make custom branch patterns more useful (#271)

docs/usage.md

@@ -45,4 +45,6 @@ and update the labels appropriately.
 
 * `BOT_USER_NAME` - the username if your bot (e.g `trop[bot]`)
 * `SKIP_CHECK_LABEL` - see [skipping manual backports](./manual-backports.md#skipping-backport-checks)
-* `NUM_SUPPORTED_VERSIONS` - trop assumes numeric branch prefixes (e.g `8-x-y`, 9-x-y) and can automatically backport to the 4 most recent branches by default.
+* `NUM_SUPPORTED_VERSIONS` - automatic backports to stable branches further than this many back will be skipped. Defaults to 3.
+* `NO_EOL_SUPPORT` - if set to `1`, manual backports to stable branches older than `NUM_SUPPORTED_VERSIONS` will also be disallowed.
+* `SUPPORTED_BRANCH_PATTERN` - regex to define what a "stable branch" is. Defaults to `^(\d+)-(?:(\d+)-x|x-y)$`, which matches branches like `8-x-y` or `5-1-x`. Regex groups will be used for sorting, to determine which branch is "older" than another. Numeric groups (matching `/^\d+$/`) will be compared numerically, otherwise groups will be compared lexically. The first group is treated as the major version. There must be at least one group.

spec/branch-util.spec.ts

@@ -1,4 +1,4 @@
-import { getBackportPattern } from '../src/utils/branch-util';
+import { BranchMatcher, getBackportPattern } from '../src/utils/branch-util';
 
 describe('getBackportPattern', () => {
   it('matches backport patterns correctly', () => {
@@ -18,3 +18,45 @@ describe('getBackportPattern', () => {
     }
   });
 });
+
+describe('BranchMatcher', () => {
+  it('matches supported branches', () => {
+    const bm = new BranchMatcher(/^(\d+)-x-y$/, 3);
+    expect(bm.isBranchSupported('3-x-y')).toBeTruthy();
+    expect(bm.isBranchSupported('192-x-y')).toBeTruthy();
+    expect(bm.isBranchSupported('z-x-y')).toBeFalsy();
+    expect(bm.isBranchSupported('foo')).toBeFalsy();
+    expect(bm.isBranchSupported('3-x-y-z')).toBeFalsy();
+    expect(bm.isBranchSupported('x3-x-y')).toBeFalsy();
+    expect(bm.isBranchSupported('')).toBeFalsy();
+  });
+
+  it('sorts and filters release branches', () => {
+    const bm = new BranchMatcher(/^(\d+)-x-y$/, 2);
+    expect(
+      bm.getSupportedBranches([
+        '3-x-y',
+        '6-x-y',
+        '5-x-y',
+        'unrelated',
+        '4-x-y',
+      ]),
+    ).toStrictEqual(['5-x-y', '6-x-y']);
+  });
+
+  it('when one group is undefined, the branch with fewer groups wins', () => {
+    const bm = new BranchMatcher(/^(\d+)-(?:(\d+)-x|x-y)$/, 2);
+    expect(bm.getSupportedBranches(['6-x-y', '5-1-x', '5-x-y'])).toStrictEqual([
+      '5-x-y',
+      '6-x-y',
+    ]);
+  });
+
+  it('can sort non-numeric groups', () => {
+    const bm = new BranchMatcher(/^0\.([A-Z])$/, 2);
+    expect(bm.getSupportedBranches(['0.F', '0.H', '0.G'])).toStrictEqual([
+      '0.G',
+      '0.H',
+    ]);
+  });
+});

spec/index.spec.ts

@@ -250,7 +250,7 @@ describe('trop', () => {
       expect(updatePayload).toMatchObject({
         title: 'Conflicting Backport Information',
         summary:
-          'The PR has a "no-backport" and at least one "target/x-y-z" label. Impossible to determine backport action.',
+          'The PR has a "no-backport" and at least one "target/<branch>" label. Impossible to determine backport action.',
         conclusion: CheckRunStatus.FAILURE,
       });
     });

src/constants.ts

@@ -2,7 +2,10 @@ export const CHECK_PREFIX = 'Backportable? - ';
 
 export const BACKPORT_INFORMATION_CHECK = 'Backport Labels Added';
 
-export const NUM_SUPPORTED_VERSIONS = process.env.NUM_SUPPORTED_VERSIONS || 3;
+export const NUM_SUPPORTED_VERSIONS = parseInt(
+  process.env.NUM_SUPPORTED_VERSIONS || '3',
+  10,
+);
 
 export const BACKPORT_LABEL = 'backport';
 

src/index.ts

@@ -20,7 +20,7 @@ import {
   backportToBranch,
 } from './operations/backport-to-location';
 import { updateManualBackport } from './operations/update-manual-backport';
-import { getSupportedBranches } from './utils/branch-util';
+import { getSupportedBranches, isBranchSupported } from './utils/branch-util';
 import {
   getBackportInformationCheck,
   queueBackportInformationCheck,
@@ -59,10 +59,7 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
     change: PRChange,
   ) => {
     for (const label of pr.labels) {
-      const targetBranch = label.name.match(
-        /^(\d)+-(?:(?:[0-9]+-x$)|(?:x+-y$))$/,
-      );
-      if (targetBranch && targetBranch[0]) {
+      if (isBranchSupported(label.name)) {
         await labelClosedPR(context, pr, label.name, change);
       }
     }
@@ -243,10 +240,7 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
               label.name.startsWith(status),
             )
           ) {
-            const targetBranch = label!.name.match(
-              /^(\d)+-(?:(?:[0-9]+-x$)|(?:x+-y$))$/,
-            );
-            if (targetBranch?.[0] && targetBranch?.[0] === pr.base.ref) {
+            if (isBranchSupported(label!.name) && label!.name === pr.base.ref) {
               robot.log(
                 `#${pr.number} is trying to backport to itself - this is not allowed`,
               );
@@ -427,7 +421,7 @@ const probotHandler: ApplicationFunction = async (robot, { getRouter }) => {
         await updateBackportInformationCheck(context, backportCheck, {
           title: 'Conflicting Backport Information',
           summary:
-            'The PR has a "no-backport" and at least one "target/x-y-z" label. Impossible to determine backport action.',
+            'The PR has a "no-backport" and at least one "target/<branch>" label. Impossible to determine backport action.',
           conclusion: CheckRunStatus.FAILURE,
         });
 

src/utils/branch-util.ts

@@ -5,6 +5,64 @@ import { log } from './log-util';
 import { LogLevel } from '../enums';
 import { WebHookRepoContext } from '../types';
 
+const SUPPORTED_BRANCH_PATTERN = new RegExp(
+  getEnvVar('SUPPORTED_BRANCH_PATTERN', '^(\\d+)-(?:(\\d+)-x|x-y)$'),
+);
+
+export class BranchMatcher {
+  branchPattern: RegExp;
+  numSupportedVersions: number;
+
+  constructor(branchPattern: RegExp, numSupportedVersions: number) {
+    this.branchPattern = branchPattern;
+    this.numSupportedVersions = numSupportedVersions;
+  }
+
+  isBranchSupported(branchName: string): boolean {
+    return this.branchPattern.test(branchName);
+  }
+
+  getSupportedBranches(allBranches: string[]): string[] {
+    const releaseBranches = allBranches.filter((branch) =>
+      this.isBranchSupported(branch),
+    );
+    console.log(allBranches, releaseBranches);
+    const filtered: Record<string, string> = {};
+    releaseBranches.sort((a, b) => {
+      const [, ...aParts] = this.branchPattern.exec(a)!;
+      const [, ...bParts] = this.branchPattern.exec(b)!;
+      for (let i = 0; i < aParts.length; i += 1) {
+        if (aParts[i] === bParts[i]) continue;
+        return comparePart(aParts[i], bParts[i]);
+      }
+      return 0;
+    });
+    for (const branch of releaseBranches)
+      filtered[this.branchPattern.exec(branch)![1]] = branch;
+
+    const values = Object.values(filtered);
+    return values.sort(comparePart).slice(-this.numSupportedVersions);
+  }
+}
+
+const branchMatcher = new BranchMatcher(
+  SUPPORTED_BRANCH_PATTERN,
+  NUM_SUPPORTED_VERSIONS,
+);
+
+export const isBranchSupported =
+  branchMatcher.isBranchSupported.bind(branchMatcher);
+
+function comparePart(a: string, b: string): number {
+  if (a == null && b != null) return 1;
+  if (b == null) return -1;
+  if (/^\d+$/.test(a)) {
+    return parseInt(a, 10) - parseInt(b, 10);
+  } else {
+    return a.localeCompare(b);
+  }
+}
+
 /**
  * Fetches an array of the currently supported branches for a repository.
  *
@@ -20,45 +78,17 @@ export async function getSupportedBranches(
     'Fetching supported branches for this repository',
   );
 
-  const SUPPORTED_BRANCH_ENV_PATTERN = getEnvVar(
-    'SUPPORTED_BRANCH_PATTERN',
-    '^(d)+-(?:(?:[0-9]+-x$)|(?:x+-y$))$',
-  );
-  const SUPPORTED_BRANCH_PATTERN = new RegExp(SUPPORTED_BRANCH_ENV_PATTERN);
-
   const { data: branches } = await context.octokit.repos.listBranches(
     context.repo({
       protected: true,
     }),
   );
 
-  const releaseBranches = branches.filter((branch) =>
-    branch.name.match(SUPPORTED_BRANCH_PATTERN),
-  );
-  const filtered: Record<string, string> = {};
-  releaseBranches
-    .sort((a, b) => {
-      const aParts = a.name.split('-');
-      const bParts = b.name.split('-');
-      for (let i = 0; i < aParts.length; i += 1) {
-        if (aParts[i] === bParts[i]) continue;
-        return parseInt(aParts[i], 10) - parseInt(bParts[i], 10);
-      }
-      return 0;
-    })
-    .forEach((branch) => {
-      return (filtered[branch.name.split('-')[0]] = branch.name);
-    });
-
-  const values = Object.values(filtered);
-  return values
-    .sort((a, b) => parseInt(a, 10) - parseInt(b, 10))
-    .slice(-NUM_SUPPORTED_VERSIONS);
+  return branchMatcher.getSupportedBranches(branches.map((b) => b.name));
 }
 
 /**
  * @returns A scoped Regex matching the backport pattern present in PR bodies.
  */
-export const getBackportPattern = () => {
-  return /(?:^|\n)(?:manual |manually )?backport (?:of )?(?:#(\d+)|https:\/\/github.com\/.*\/pull\/(\d+))/gim;
-};
+export const getBackportPattern = () =>
+  /(?:^|\n)(?:manual |manually )?backport (?:of )?(?:#(\d+)|https:\/\/github.com\/.*\/pull\/(\d+))/gim;

src/utils/checks-util.ts

@@ -83,7 +83,7 @@ export async function queueBackportInformationCheck(context: WebHookPRContext) {
       output: {
         title: 'Needs Backport Information',
         summary:
-          'This PR requires backport information. It should have a "no-backport" or a "target/x-y-z" label.',
+          'This PR requires backport information. It should have a "no-backport" or a "target/<branch>" label.',
       },
     }),
   );