From 6f0f0d3ea2e44f9884a4a0c8acdf4dda52c09b6c Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 24 Jul 2020 18:05:51 +0200 Subject: [PATCH] feat(dev-infra): support user-failures when computing branches for target label (#38223) The merge tool provides a way for configurations to determine the branches for a label lazily. This is supported because it allows labels to respect the currently selected base branch through the Github UI. e.g. if `target: label` is applied on a PR and the PR is based on the patch branch, then the change could only go into the selected target branch, while if it would be based on `master`, the change would be cherry-picked to `master` too. This allows for convenient back-porting of changes if they did not apply cleanly to the primary development branch (`master`). We want to expand this function so that it is possible to report failures if an invalid target label is appplied (e.g. `target: major` not allowed in some situations), or if the Github base branch is not valid for the given target label (e.g. if `target: lts` is used, but it's not based on a LTS branch). PR Close #38223 --- dev-infra/pr/merge/config.ts | 3 +++ dev-infra/pr/merge/pull-request.ts | 18 ++++++++++++++++-- dev-infra/pr/merge/target-label.ts | 30 ++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/dev-infra/pr/merge/config.ts b/dev-infra/pr/merge/config.ts index 5ac8cfaee8..ddf4f75dcb 100644 --- a/dev-infra/pr/merge/config.ts +++ b/dev-infra/pr/merge/config.ts @@ -31,6 +31,9 @@ export interface TargetLabel { * List of branches a pull request with this target label should be merged into. * Can also be wrapped in a function that accepts the target branch specified in the * Github Web UI. This is useful for supporting labels like `target: development-branch`. + * + * @throws {InvalidTargetLabelError} Invalid label has been applied to pull request. + * @throws {InvalidTargetBranchError} Invalid Github target branch has been selected. */ branches: TargetLabelBranchResult|((githubTargetBranch: string) => TargetLabelBranchResult); } diff --git a/dev-infra/pr/merge/pull-request.ts b/dev-infra/pr/merge/pull-request.ts index 406105c5a4..40e1d646a9 100644 --- a/dev-infra/pr/merge/pull-request.ts +++ b/dev-infra/pr/merge/pull-request.ts @@ -12,7 +12,7 @@ import {GitClient} from '../../utils/git'; import {PullRequestFailure} from './failures'; import {matchesPattern} from './string-pattern'; -import {getBranchesFromTargetLabel, getTargetLabelFromPullRequest} from './target-label'; +import {getBranchesFromTargetLabel, getTargetLabelFromPullRequest, InvalidTargetBranchError, InvalidTargetLabelError} from './target-label'; import {PullRequestMergeTask} from './task'; /** Interface that describes a pull request. */ @@ -83,6 +83,20 @@ export async function loadAndValidatePullRequest( labels.some(name => matchesPattern(name, config.commitMessageFixupLabel)); const hasCaretakerNote = !!config.caretakerNoteLabel && labels.some(name => matchesPattern(name, config.caretakerNoteLabel!)); + let targetBranches: string[]; + + // If branches are determined for a given target label, capture errors that are + // thrown as part of branch computation. This is expected because a merge configuration + // can lazily compute branches for a target label and throw. e.g. if an invalid target + // label is applied, we want to exit the script gracefully with an error message. + try { + targetBranches = await getBranchesFromTargetLabel(targetLabel, githubTargetBranch); + } catch (error) { + if (error instanceof InvalidTargetBranchError || error instanceof InvalidTargetLabelError) { + return new PullRequestFailure(error.failureMessage); + } + throw error; + } return { url: prData.html_url, @@ -92,8 +106,8 @@ export async function loadAndValidatePullRequest( githubTargetBranch, needsCommitMessageFixup, hasCaretakerNote, + targetBranches, title: prData.title, - targetBranches: getBranchesFromTargetLabel(targetLabel, githubTargetBranch), commitCount: prData.commits, }; } diff --git a/dev-infra/pr/merge/target-label.ts b/dev-infra/pr/merge/target-label.ts index dec769b1ee..ba43cecc1f 100644 --- a/dev-infra/pr/merge/target-label.ts +++ b/dev-infra/pr/merge/target-label.ts @@ -9,6 +9,22 @@ import {MergeConfig, TargetLabel} from './config'; import {matchesPattern} from './string-pattern'; +/** + * Unique error that can be thrown in the merge configuration if an + * invalid branch is targeted. + */ +export class InvalidTargetBranchError { + constructor(public failureMessage: string) {} +} + +/** + * Unique error that can be thrown in the merge configuration if an + * invalid label has been applied to a pull request. + */ +export class InvalidTargetLabelError { + constructor(public failureMessage: string) {} +} + /** Gets the target label from the specified pull request labels. */ export function getTargetLabelFromPullRequest(config: MergeConfig, labels: string[]): TargetLabel| null { @@ -21,8 +37,14 @@ export function getTargetLabelFromPullRequest(config: MergeConfig, labels: strin return null; } -/** Gets the branches from the specified target label. */ -export function getBranchesFromTargetLabel( - label: TargetLabel, githubTargetBranch: string): string[] { - return typeof label.branches === 'function' ? label.branches(githubTargetBranch) : label.branches; +/** + * Gets the branches from the specified target label. + * + * @throws {InvalidTargetLabelError} Invalid label has been applied to pull request. + * @throws {InvalidTargetBranchError} Invalid Github target branch has been selected. + */ +export async function getBranchesFromTargetLabel( + label: TargetLabel, githubTargetBranch: string): Promise { + return typeof label.branches === 'function' ? await label.branches(githubTargetBranch) : + await label.branches; }