From 87d47fd9e2e93ae322af0f4dac3f012c469f34ef Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Fri, 5 Jan 2024 06:53:58 -0800 Subject: [PATCH] feat: make custom branch patterns more useful (#271) --- docs/usage.md | 4 +- spec/branch-util.spec.ts | 44 ++++++++++++++++++- spec/index.spec.ts | 2 +- src/constants.ts | 5 ++- src/index.ts | 14 ++---- src/utils/branch-util.ts | 92 ++++++++++++++++++++++++++-------------- src/utils/checks-util.ts | 2 +- 7 files changed, 117 insertions(+), 46 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 2dd8b29..6a6d9d5 100644 --- a/docs/usage.md +++ b/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. diff --git a/spec/branch-util.spec.ts b/spec/branch-util.spec.ts index 197344e..ec6877b 100644 --- a/spec/branch-util.spec.ts +++ b/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', + ]); + }); +}); diff --git a/spec/index.spec.ts b/spec/index.spec.ts index 2bf430a..1d856b5 100644 --- a/spec/index.spec.ts +++ b/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/" label. Impossible to determine backport action.', conclusion: CheckRunStatus.FAILURE, }); }); diff --git a/src/constants.ts b/src/constants.ts index b74c642..bfc0e6f 100644 --- a/src/constants.ts +++ b/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'; diff --git a/src/index.ts b/src/index.ts index dc3a8ea..1bbfd93 100644 --- a/src/index.ts +++ b/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/" label. Impossible to determine backport action.', conclusion: CheckRunStatus.FAILURE, }); diff --git a/src/utils/branch-util.ts b/src/utils/branch-util.ts index 816fa1a..a753159 100644 --- a/src/utils/branch-util.ts +++ b/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 = {}; + 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 = {}; - 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; diff --git a/src/utils/checks-util.ts b/src/utils/checks-util.ts index 45c49a5..5ce11f9 100644 --- a/src/utils/checks-util.ts +++ b/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/" label.', }, }), );