From c7d86bca210516d990c4dbce710eb7a4c2b805b3 Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Tue, 6 Apr 2021 08:18:44 -0700 Subject: [PATCH] feat(dev-infra): add support for `targetLabelExemptScopes` for merging (#41459) Add a property, `targetLabelExemptScopes`, to the merge configuration allowing certain scopes to be exempted from requirements for features and breaking changes only included in PRs targetting certain labels. PR Close #41459 --- dev-infra/ng-dev.js | 49 +++++++++++++++++++----------- dev-infra/pr/merge/config.ts | 5 +++ dev-infra/pr/merge/failures.ts | 4 +-- dev-infra/pr/merge/pull-request.ts | 43 +++++++++++++++++--------- 4 files changed, 67 insertions(+), 34 deletions(-) diff --git a/dev-infra/ng-dev.js b/dev-infra/ng-dev.js index 69807d1308..fe7ff2bf05 100755 --- a/dev-infra/ng-dev.js +++ b/dev-infra/ng-dev.js @@ -3355,13 +3355,13 @@ var PullRequestFailure = /** @class */ (function () { }; PullRequestFailure.hasBreakingChanges = function (label) { var message = "Cannot merge into branch for \"" + label.pattern + "\" as the pull request has " + - "breaking changes. Breaking changes can only be merged with the \"target: major\" label."; + "breaking changes. Breaking changes can only be merged with the \"target: major\" label."; return new this(message); }; PullRequestFailure.hasFeatureCommits = function (label) { var message = "Cannot merge into branch for \"" + label.pattern + "\" as the pull request has " + - 'commits with the "feat" type. New features can only be merged with the "target: minor" or ' + - '"target: major" label.'; + 'commits with the "feat" type. New features can only be merged with the "target: minor" ' + + 'or "target: major" label.'; return new this(message); }; return PullRequestFailure; @@ -3398,7 +3398,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, state, githubTargetBranch, requiredBaseSha, needsCommitMessageFixup, hasCaretakerNote, targetBranches, error_1; + var prData, labels, targetLabel, commitMessages, 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,7 +3424,8 @@ function loadAndValidatePullRequest(_a, prNumber, ignoreNonFatalFailures) { throw error; } try { - assertCorrectTargetForChanges(prData.commits.nodes.map(function (n) { return n.commit.message; }), targetLabel); + commitMessages = prData.commits.nodes.map(function (n) { return n.commit.message; }); + assertChangesAllowForTargetLabel(commitMessages, targetLabel, config); } catch (error) { return [2 /*return*/, error]; @@ -3465,17 +3466,20 @@ function loadAndValidatePullRequest(_a, prNumber, ignoreNonFatalFailures) { hasCaretakerNote: hasCaretakerNote, targetBranches: targetBranches, title: prData.title, - commitCount: prData.commits.nodes.length, + commitCount: prData.commits.totalCount, }]; } }); }); } -/* GraphQL schema for the response body the requested PR. */ +/* GraphQL schema for the response body the requested pull request. */ var PR_SCHEMA$2 = { url: typedGraphqlify.types.string, number: typedGraphqlify.types.number, - commits: typedGraphqlify.params({ first: 100 }, { + // Only the last 100 commits from a pull request are obtained as we likely will never see a pull + // requests with more than 100 commits. + commits: typedGraphqlify.params({ last: 100 }, { + totalCount: typedGraphqlify.types.number, nodes: [{ commit: { status: { @@ -3496,13 +3500,15 @@ var PR_SCHEMA$2 = { /** Fetches a pull request from Github. Returns null if an error occurred. */ function fetchPullRequestFromGithub(git, prNumber) { return tslib.__awaiter(this, void 0, void 0, function () { - var e_1; + var x, e_1; return tslib.__generator(this, function (_a) { switch (_a.label) { case 0: _a.trys.push([0, 2, , 3]); return [4 /*yield*/, getPr(PR_SCHEMA$2, prNumber, git)]; - case 1: return [2 /*return*/, _a.sent()]; + case 1: + x = _a.sent(); + return [2 /*return*/, x]; case 2: e_1 = _a.sent(); // If the pull request could not be found, we want to return `null` so @@ -3524,32 +3530,39 @@ function isPullRequest(v) { * Assert the commits provided are allowed to merge to the provided target label, throwing a * PullRequestFailure otherwise. */ -function assertCorrectTargetForChanges(rawCommits, label) { - /** List of ParsedCommits for all of the commits in the pull request. */ - var commits = rawCommits.map(parseCommitMessage); +function assertChangesAllowForTargetLabel(rawCommits, 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); + }); switch (label.pattern) { case 'target: major': break; case 'target: minor': - // Check if any commits in the PR contains a breaking change. + // Check if any commits in the pull request contains a breaking change. if (commits.some(function (commit) { return commit.breakingChanges.length !== 0; })) { throw PullRequestFailure.hasBreakingChanges(label); } break; case 'target: patch': case 'target: lts': - // Check if any commits in the PR contains a breaking change. + // Check if any commits in the pull request contains a breaking change. if (commits.some(function (commit) { return commit.breakingChanges.length !== 0; })) { throw PullRequestFailure.hasBreakingChanges(label); } - // Check if any commits in the PR contains a commit type of "feat". + // Check if any commits in the pull request contains a commit type of "feat". if (commits.some(function (commit) { return commit.type === 'feat'; })) { throw PullRequestFailure.hasFeatureCommits(label); } break; default: - warn(red('WARNING: Unable to confirm all commits in the PR are eligible to be merged')); - warn(red("into the target branch: " + label.pattern)); + warn(red('WARNING: Unable to confirm all commits in the pull request are eligible to be')); + warn(red("merged into the target branch: " + label.pattern)); break; } } diff --git a/dev-infra/pr/merge/config.ts b/dev-infra/pr/merge/config.ts index ddf4f75dcb..03cea76aaa 100644 --- a/dev-infra/pr/merge/config.ts +++ b/dev-infra/pr/merge/config.ts @@ -71,6 +71,11 @@ export interface MergeConfig { * not support this. */ githubApiMerge: false|GithubApiMergeStrategyConfig; + /** + * 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. + */ + targetLabelExemptScopes?: string[]; } /** diff --git a/dev-infra/pr/merge/failures.ts b/dev-infra/pr/merge/failures.ts index 94b6f378b0..01aa6ae2ee 100644 --- a/dev-infra/pr/merge/failures.ts +++ b/dev-infra/pr/merge/failures.ts @@ -77,13 +77,13 @@ export class PullRequestFailure { static hasBreakingChanges(label: TargetLabel) { const message = `Cannot merge into branch for "${label.pattern}" as the pull request has ` + - `breaking changes. Breaking changes can only be merged with the "target: major" label.`; + `breaking changes. Breaking changes can only be merged with the "target: major" label.`; return new this(message); } static hasFeatureCommits(label: TargetLabel) { const message = `Cannot merge into branch for "${label.pattern}" as the pull request has ` + - 'commits with the "feat" type. New features can only be merged with the "target: minor" ' + + 'commits with the "feat" type. New features can only be merged with the "target: minor" ' + 'or "target: major" label.'; return new this(message); } diff --git a/dev-infra/pr/merge/pull-request.ts b/dev-infra/pr/merge/pull-request.ts index e3ff09fc63..4b80a543c4 100644 --- a/dev-infra/pr/merge/pull-request.ts +++ b/dev-infra/pr/merge/pull-request.ts @@ -12,7 +12,7 @@ import {red, warn} from '../../utils/console'; import {GitClient} from '../../utils/git/index'; import {getPr} from '../../utils/github'; -import {TargetLabel} from './config'; +import {MergeConfig, TargetLabel} from './config'; import {PullRequestFailure} from './failures'; import {matchesPattern} from './string-pattern'; @@ -76,11 +76,14 @@ export async function loadAndValidatePullRequest( } try { - assertCorrectTargetForChanges(prData.commits.nodes.map(n => n.commit.message), targetLabel); + /** Commit message strings for all commits in the pull request. */ + const commitMessages = prData.commits.nodes.map(n => n.commit.message); + assertChangesAllowForTargetLabel(commitMessages, targetLabel, config); } catch (error) { return error; } + /** The combined status of the latest commit in the pull request. */ const state = prData.commits.nodes.slice(-1)[0].commit.status.state; if (state === 'FAILURE' && !ignoreNonFatalFailures) { return PullRequestFailure.failingCiJobs(); @@ -121,15 +124,18 @@ export async function loadAndValidatePullRequest( hasCaretakerNote, targetBranches, title: prData.title, - commitCount: prData.commits.nodes.length, + commitCount: prData.commits.totalCount, }; } -/* GraphQL schema for the response body the requested PR. */ +/* GraphQL schema for the response body the requested pull request. */ const PR_SCHEMA = { url: graphQLTypes.string, number: graphQLTypes.number, - commits: params({first: 100}, { + // Only the last 100 commits from a pull request are obtained as we likely will never see a pull + // requests with more than 100 commits. + commits: params({last: 100}, { + totalCount: graphQLTypes.number, nodes: [{ commit: { status: { @@ -154,7 +160,8 @@ const PR_SCHEMA = { async function fetchPullRequestFromGithub( git: GitClient, prNumber: number): Promise { try { - return await getPr(PR_SCHEMA, prNumber, git); + const x = await getPr(PR_SCHEMA, prNumber, git); + return x; } catch (e) { // If the pull request could not be found, we want to return `null` so // that the error can be handled gracefully. @@ -174,32 +181,40 @@ export function isPullRequest(v: PullRequestFailure|PullRequest): v is PullReque * Assert the commits provided are allowed to merge to the provided target label, throwing a * PullRequestFailure otherwise. */ -function assertCorrectTargetForChanges(rawCommits: string[], label: TargetLabel) { - /** List of ParsedCommits for all of the commits in the pull request. */ - const commits = rawCommits.map(parseCommitMessage); +function assertChangesAllowForTargetLabel( + rawCommits: string[], 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); + }); switch (label.pattern) { case 'target: major': break; case 'target: minor': - // Check if any commits in the PR contains a breaking change. + // Check if any commits in the pull request contains a breaking change. if (commits.some(commit => commit.breakingChanges.length !== 0)) { throw PullRequestFailure.hasBreakingChanges(label); } break; case 'target: patch': case 'target: lts': - // Check if any commits in the PR contains a breaking change. + // Check if any commits in the pull request contains a breaking change. if (commits.some(commit => commit.breakingChanges.length !== 0)) { throw PullRequestFailure.hasBreakingChanges(label); } - // Check if any commits in the PR contains a commit type of "feat". + // Check if any commits in the pull request contains a commit type of "feat". if (commits.some(commit => commit.type === 'feat')) { throw PullRequestFailure.hasFeatureCommits(label); } break; default: - warn(red('WARNING: Unable to confirm all commits in the PR are eligible to be merged')); - warn(red(`into the target branch: ${label.pattern}`)); + warn(red('WARNING: Unable to confirm all commits in the pull request are eligible to be')); + warn(red(`merged into the target branch: ${label.pattern}`)); break; } }