From 4a69a7b95244a6d01143b6b8da11816c268c68da Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Tue, 6 Apr 2021 08:57:32 -0700 Subject: [PATCH] feat(dev-infra): verify breaking changes are properly labeled before merging (#41546) During merging with `ng-dev pr merge` tooling will ensure that pull requests are properly labeled for breaking changes. Pull requests with commits noting breaking changes must also be labeled as such, additionally pull requests with breaking change labels must contain commits noting breaking changes. Fixes #38776 PR Close #41546 --- dev-infra/ng-dev.js | 43 ++++++++++++++++++++++++------ dev-infra/pr/merge/config.ts | 2 ++ dev-infra/pr/merge/failures.ts | 12 +++++++++ dev-infra/pr/merge/pull-request.ts | 41 +++++++++++++++++++++------- 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index 0ceb6c45bb..5d86f09a9f 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -3365,6 +3365,16 @@ var PullRequestFailure = /** @class */ (function () { 'or "target: major" label.'; return new this(message); }; + PullRequestFailure.missingBreakingChangeLabel = function () { + var message = 'Pull Request has at least one commit containing a breaking change note, but ' + + 'does not have a breaking change label.'; + return new this(message); + }; + PullRequestFailure.missingBreakingChangeCommit = function () { + var message = 'Pull Request has a breaking change label, but does not contain any commits ' + + 'with breaking change notes.'; + return new this(message); + }; return PullRequestFailure; }()); @@ -3391,6 +3401,8 @@ function getTargettedBranchesConfirmationPromptMessage(pullRequest) { * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +/** The default label for labeling pull requests containing a breaking change. */ +var BreakingChangeLabel = 'breaking changes'; /** * Loads and validates the specified pull request against the given configuration. * If the pull requests fails, a pull request failure is returned. @@ -3399,7 +3411,7 @@ function loadAndValidatePullRequest(_a, prNumber, ignoreNonFatalFailures) { var git = _a.git, config = _a.config; if (ignoreNonFatalFailures === void 0) { ignoreNonFatalFailures = false; } return tslib.__awaiter(this, void 0, void 0, function () { - var prData, labels, targetLabel, commitMessages, state, githubTargetBranch, requiredBaseSha, needsCommitMessageFixup, hasCaretakerNote, targetBranches, error_1; + var prData, labels, targetLabel, commitsInPr, state, githubTargetBranch, requiredBaseSha, needsCommitMessageFixup, hasCaretakerNote, targetBranches, error_1; return tslib.__generator(this, function (_b) { switch (_b.label) { case 0: return [4 /*yield*/, fetchPullRequestFromGithub(git, prNumber)]; @@ -3424,9 +3436,10 @@ function loadAndValidatePullRequest(_a, prNumber, ignoreNonFatalFailures) { } throw error; } + commitsInPr = prData.commits.nodes.map(function (n) { return parseCommitMessage(n.commit.message); }); try { - commitMessages = prData.commits.nodes.map(function (n) { return n.commit.message; }); - assertChangesAllowForTargetLabel(commitMessages, targetLabel, config); + assertChangesAllowForTargetLabel(commitsInPr, targetLabel, config); + assertCorrectBreakingChangeLabeling(commitsInPr, labels, config); } catch (error) { return [2 /*return*/, error]; @@ -3531,16 +3544,14 @@ function isPullRequest(v) { * Assert the commits provided are allowed to merge to the provided target label, throwing a * PullRequestFailure otherwise. */ -function assertChangesAllowForTargetLabel(rawCommits, label, config) { +function assertChangesAllowForTargetLabel(commits, label, config) { /** * List of commit scopes which are exempted from target label content requirements. i.e. no `feat` * scopes in patch branches, no breaking changes in minor or patch changes. */ var exemptedScopes = config.targetLabelExemptScopes || []; - /** List of parsed commits which are subject to content requirements for the target label. */ - var commits = rawCommits.map(parseCommitMessage).filter(function (commit) { - return !exemptedScopes.includes(commit.scope); - }); + /** List of commits which are subject to content requirements for the target label. */ + commits = commits.filter(function (commit) { return !exemptedScopes.includes(commit.scope); }); switch (label.pattern) { case 'target: major': break; @@ -3567,6 +3578,22 @@ function assertChangesAllowForTargetLabel(rawCommits, label, config) { break; } } +/** + * Assert the pull request has the proper label for breaking changes if there are breaking change + * commits, and only has the label if there are breaking change commits. + */ +function assertCorrectBreakingChangeLabeling(commits, labels, config) { + /** Whether the PR has a label noting a breaking change. */ + var hasLabel = labels.includes(config.breakingChangeLabel || BreakingChangeLabel); + //** Whether the PR has at least one commit which notes a breaking change. */ + var hasCommit = commits.some(function (commit) { return commit.breakingChanges.length !== 0; }); + if (!hasLabel && hasCommit) { + throw PullRequestFailure.missingBreakingChangeLabel(); + } + if (hasLabel && !hasCommit) { + throw PullRequestFailure.missingBreakingChangeCommit(); + } +} /** * @license diff --git a/dev-infra/pr/merge/config.ts b/dev-infra/pr/merge/config.ts index 03cea76aaa..b34df2ed24 100644 --- a/dev-infra/pr/merge/config.ts +++ b/dev-infra/pr/merge/config.ts @@ -64,6 +64,8 @@ export interface MergeConfig { caretakerNoteLabel?: string|RegExp; /** Label which can be applied to fixup commit messages in the merge script. */ commitMessageFixupLabel: string|RegExp; + /** Label that is applied when a breaking change is made in the pull request. */ + breakingChangeLabel?: string; /** * Whether pull requests should be merged using the Github API. This can be enabled * if projects want to have their pull requests show up as `Merged` in the Github UI. diff --git a/dev-infra/pr/merge/failures.ts b/dev-infra/pr/merge/failures.ts index 01aa6ae2ee..5675eb5ad0 100644 --- a/dev-infra/pr/merge/failures.ts +++ b/dev-infra/pr/merge/failures.ts @@ -87,4 +87,16 @@ export class PullRequestFailure { 'or "target: major" label.'; return new this(message); } + + static missingBreakingChangeLabel() { + const message = 'Pull Request has at least one commit containing a breaking change note, but ' + + 'does not have a breaking change label.'; + return new this(message); + } + + static missingBreakingChangeCommit() { + const message = 'Pull Request has a breaking change label, but does not contain any commits ' + + 'with breaking change notes.'; + return new this(message); + } } diff --git a/dev-infra/pr/merge/pull-request.ts b/dev-infra/pr/merge/pull-request.ts index c79435103d..07988287b9 100644 --- a/dev-infra/pr/merge/pull-request.ts +++ b/dev-infra/pr/merge/pull-request.ts @@ -7,7 +7,7 @@ */ import {params, types as graphQLTypes} from 'typed-graphqlify'; -import {parseCommitMessage} from '../../commit-message/parse'; +import {Commit, parseCommitMessage} from '../../commit-message/parse'; import {red, warn} from '../../utils/console'; import {GitClient} from '../../utils/git/index'; @@ -19,6 +19,9 @@ import {matchesPattern} from './string-pattern'; import {getBranchesFromTargetLabel, getTargetLabelFromPullRequest, InvalidTargetBranchError, InvalidTargetLabelError} from './target-label'; import {PullRequestMergeTask} from './task'; +/** The default label for labeling pull requests containing a breaking change. */ +const BreakingChangeLabel = 'breaking changes'; + /** Interface that describes a pull request. */ export interface PullRequest { /** URL to the pull request. */ @@ -75,10 +78,12 @@ export async function loadAndValidatePullRequest( throw error; } + /** List of parsed commits for all of the commits in the pull request. */ + const commitsInPr = prData.commits.nodes.map(n => parseCommitMessage(n.commit.message)); + try { - /** Commit message strings for all commits in the pull request. */ - const commitMessages = prData.commits.nodes.map(n => n.commit.message); - assertChangesAllowForTargetLabel(commitMessages, targetLabel, config); + assertChangesAllowForTargetLabel(commitsInPr, targetLabel, config); + assertCorrectBreakingChangeLabeling(commitsInPr, labels, config); } catch (error) { return error; } @@ -182,16 +187,14 @@ export function isPullRequest(v: PullRequestFailure|PullRequest): v is PullReque * PullRequestFailure otherwise. */ function assertChangesAllowForTargetLabel( - rawCommits: string[], label: TargetLabel, config: MergeConfig) { + commits: Commit[], label: TargetLabel, config: MergeConfig) { /** * List of commit scopes which are exempted from target label content requirements. i.e. no `feat` * scopes in patch branches, no breaking changes in minor or patch changes. */ const exemptedScopes = config.targetLabelExemptScopes || []; - /** List of parsed commits which are subject to content requirements for the target label. */ - let commits = rawCommits.map(parseCommitMessage).filter(commit => { - return !exemptedScopes.includes(commit.scope); - }); + /** List of commits which are subject to content requirements for the target label. */ + commits = commits.filter(commit => !exemptedScopes.includes(commit.scope)); switch (label.pattern) { case 'target: major': break; @@ -218,3 +221,23 @@ function assertChangesAllowForTargetLabel( break; } } + +/** + * Assert the pull request has the proper label for breaking changes if there are breaking change + * commits, and only has the label if there are breaking change commits. + */ +function assertCorrectBreakingChangeLabeling( + commits: Commit[], labels: string[], config: MergeConfig) { + /** Whether the PR has a label noting a breaking change. */ + const hasLabel = labels.includes(config.breakingChangeLabel || BreakingChangeLabel); + //** Whether the PR has at least one commit which notes a breaking change. */ + const hasCommit = commits.some(commit => commit.breakingChanges.length !== 0); + + if (!hasLabel && hasCommit) { + throw PullRequestFailure.missingBreakingChangeLabel(); + } + + if (hasLabel && !hasCommit) { + throw PullRequestFailure.missingBreakingChangeCommit(); + } +}