From 6fc85073d2e49164a52077fda0df828f29b668c8 Mon Sep 17 00:00:00 2001
From: Joey Perrott <josephperrott@gmail.com>
Date: Tue, 10 Mar 2020 10:29:44 -0700
Subject: [PATCH] feat(dev-infra): create commit-message validation
script/tooling (#36117)
PR Close #36117
---
.dev-infra.json | 47 ++++
dev-infra/BUILD.bazel | 3 +
dev-infra/cli.ts | 10 +
dev-infra/commit-message/BUILD.bazel | 39 ++++
dev-infra/commit-message/config.ts | 13 ++
dev-infra/commit-message/validate.spec.ts | 248 ++++++++++++++++++++++
dev-infra/commit-message/validate.ts | 134 ++++++++++++
dev-infra/utils/BUILD.bazel | 1 +
dev-infra/utils/config.ts | 14 +-
9 files changed, 503 insertions(+), 6 deletions(-)
create mode 100644 .dev-infra.json
create mode 100644 dev-infra/commit-message/BUILD.bazel
create mode 100644 dev-infra/commit-message/config.ts
create mode 100644 dev-infra/commit-message/validate.spec.ts
create mode 100644 dev-infra/commit-message/validate.ts
diff --git a/.dev-infra.json b/.dev-infra.json
new file mode 100644
index 0000000000..0a0997a8e9
--- /dev/null
+++ b/.dev-infra.json
@@ -0,0 +1,47 @@
+{
+ "commitMessage": {
+ "maxLength": 120,
+ "minBodyLength": 0,
+ "types": [
+ "build",
+ "ci",
+ "docs",
+ "feat",
+ "fix",
+ "perf",
+ "refactor",
+ "release",
+ "style",
+ "test"
+ ],
+ "scopes": [
+ "animations",
+ "bazel",
+ "benchpress",
+ "changelog",
+ "common",
+ "compiler",
+ "compiler-cli",
+ "core",
+ "dev-infra",
+ "docs-infra",
+ "elements",
+ "forms",
+ "http",
+ "language-service",
+ "localize",
+ "ngcc",
+ "packaging",
+ "platform-browser",
+ "platform-browser-dynamic",
+ "platform-server",
+ "platform-webworker",
+ "platform-webworker-dynamic",
+ "router",
+ "service-worker",
+ "upgrade",
+ "ve",
+ "zone.js"
+ ]
+ }
+}
\ No newline at end of file
diff --git a/dev-infra/BUILD.bazel b/dev-infra/BUILD.bazel
index 2d70417db3..348009afd6 100644
--- a/dev-infra/BUILD.bazel
+++ b/dev-infra/BUILD.bazel
@@ -8,7 +8,9 @@ ts_library(
],
module_name = "@angular/dev-infra-private",
deps = [
+ "//dev-infra/commit-message",
"//dev-infra/pullapprove",
+ "//dev-infra/utils:config",
"@npm//@types/node",
],
)
@@ -33,6 +35,7 @@ pkg_npm(
deps = [
":cli",
":package-json",
+ "//dev-infra/commit-message",
"//dev-infra/ts-circular-dependencies",
],
)
diff --git a/dev-infra/cli.ts b/dev-infra/cli.ts
index b6285c768d..3aa6d2e733 100644
--- a/dev-infra/cli.ts
+++ b/dev-infra/cli.ts
@@ -6,7 +6,11 @@
* 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
*/
+import {readFileSync} from 'fs';
+import {join} from 'path';
import {verify} from './pullapprove/verify';
+import {validateCommitMessage} from './commit-message/validate';
+import {getRepoBaseDir} from './utils/config';
const args = process.argv.slice(2);
@@ -16,6 +20,12 @@ switch (args[0]) {
case 'pullapprove:verify':
verify();
break;
+ case 'commit-message:pre-commit-validate':
+ const commitMessage = readFileSync(join(getRepoBaseDir(), '.git/COMMIT_EDITMSG'), 'utf8');
+ if (validateCommitMessage(commitMessage)) {
+ console.info('√ Valid commit message');
+ }
+ break;
default:
console.info('No commands were matched');
}
diff --git a/dev-infra/commit-message/BUILD.bazel b/dev-infra/commit-message/BUILD.bazel
new file mode 100644
index 0000000000..e6029084c3
--- /dev/null
+++ b/dev-infra/commit-message/BUILD.bazel
@@ -0,0 +1,39 @@
+load("//tools:defaults.bzl", "jasmine_node_test")
+load("@npm_bazel_typescript//:index.bzl", "ts_library")
+
+ts_library(
+ name = "commit-message",
+ srcs = [
+ "config.ts",
+ "validate.ts",
+ ],
+ module_name = "@angular/dev-infra-private/commit-message",
+ visibility = ["//dev-infra:__subpackages__"],
+ deps = [
+ "//dev-infra/utils:config",
+ "@npm//@types/node",
+ "@npm//tslib",
+ ],
+)
+
+ts_library(
+ name = "validate-test",
+ testonly = True,
+ srcs = ["validate.spec.ts"],
+ deps = [
+ ":commit-message",
+ "//dev-infra/utils:config",
+ "@npm//@types/events",
+ "@npm//@types/jasmine",
+ "@npm//@types/node",
+ ],
+)
+
+jasmine_node_test(
+ name = "test",
+ bootstrap = ["//tools/testing:node_no_angular_es5"],
+ deps = [
+ ":commit-message",
+ ":validate-test",
+ ],
+)
diff --git a/dev-infra/commit-message/config.ts b/dev-infra/commit-message/config.ts
new file mode 100644
index 0000000000..8b01bf3053
--- /dev/null
+++ b/dev-infra/commit-message/config.ts
@@ -0,0 +1,13 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+export interface CommitMessageConfig {
+ maxLineLength: number;
+ minBodyLength: number;
+ types: string[];
+ scopes: string[];
+}
\ No newline at end of file
diff --git a/dev-infra/commit-message/validate.spec.ts b/dev-infra/commit-message/validate.spec.ts
new file mode 100644
index 0000000000..f9f3166f06
--- /dev/null
+++ b/dev-infra/commit-message/validate.spec.ts
@@ -0,0 +1,248 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+
+// Imports
+import * as utilConfig from '../utils/config';
+
+import {validateCommitMessage} from './validate';
+
+
+// Constants
+const config = {
+ 'commitMessage': {
+ 'maxLineLength': 120,
+ 'minBodyLength': 0,
+ 'types': [
+ 'feat',
+ 'fix',
+ 'refactor',
+ 'release',
+ 'style',
+ ],
+ 'scopes': [
+ 'common',
+ 'compiler',
+ 'core',
+ 'packaging',
+ ]
+ }
+};
+const TYPES = config.commitMessage.types.join(', ');
+const SCOPES = config.commitMessage.scopes.join(', ');
+const INVALID = false;
+const VALID = true;
+
+// TODO(josephperrott): Clean up tests to test script rather than for
+// specific commit messages we want to use.
+describe('validate-commit-message.js', () => {
+ let lastError: string = '';
+
+ beforeEach(() => {
+ lastError = '';
+
+ spyOn(console, 'error').and.callFake((msg: string) => lastError = msg);
+ spyOn(utilConfig, 'getAngularDevConfig').and.returnValue(config);
+ });
+
+ describe('validateMessage()', () => {
+
+ it('should be valid', () => {
+ expect(validateCommitMessage('feat(packaging): something')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('release(packaging): something')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('fixup! release(packaging): something')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('squash! release(packaging): something')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('Revert: "release(packaging): something"')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ });
+
+ it('should validate max length', () => {
+ const msg =
+ 'fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer and longer and longer and longer...';
+
+ expect(validateCommitMessage(msg)).toBe(INVALID);
+ expect(lastError).toContain(
+ `The commit message header is longer than ${config.commitMessage.maxLineLength} characters`);
+ });
+
+ it('should validate "<type>(<scope>): <subject>" format', () => {
+ const msg = 'not correct format';
+
+ expect(validateCommitMessage(msg)).toBe(INVALID);
+ expect(lastError).toContain(`The commit message header does not match the expected format.`);
+ });
+
+ it('should fail when type is invalid', () => {
+ const msg = 'weird(core): something';
+
+ expect(validateCommitMessage(msg)).toBe(INVALID);
+ expect(lastError).toContain(`'weird' is not an allowed type.\n => TYPES: ${TYPES}`);
+ });
+
+ it('should fail when scope is invalid', () => {
+ const errorMessageFor = (scope: string, header: string) =>
+ `'${scope}' is not an allowed scope.\n => SCOPES: ${SCOPES}`;
+
+ expect(validateCommitMessage('fix(Compiler): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('Compiler', 'fix(Compiler): something'));
+
+ expect(validateCommitMessage('feat(bah): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('bah', 'feat(bah): something'));
+
+ expect(validateCommitMessage('style(webworker): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('webworker', 'style(webworker): something'));
+
+ expect(validateCommitMessage('refactor(security): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('security', 'refactor(security): something'));
+
+ expect(validateCommitMessage('refactor(docs): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('docs', 'refactor(docs): something'));
+
+ expect(validateCommitMessage('release(angular): something')).toBe(INVALID);
+ expect(lastError).toContain(errorMessageFor('angular', 'release(angular): something'));
+ });
+
+ it('should allow empty scope', () => {
+ expect(validateCommitMessage('fix: blablabla')).toBe(VALID);
+ expect(lastError).toBe('');
+ });
+
+ // We do not want to allow WIP. It is OK to fail the PR build in this case to show that there is
+ // work still to be done (i.e. fixing the commit message).
+ it('should not allow "WIP: ..." syntax', () => {
+ const msg = 'WIP: fix: something';
+
+ expect(validateCommitMessage(msg)).toBe(INVALID);
+ expect(lastError).toContain(`'WIP' is not an allowed type.\n => TYPES: ${TYPES}`);
+ });
+
+ describe('(revert)', () => {
+
+ it('should allow valid "revert: ..." syntaxes', () => {
+ expect(validateCommitMessage('revert: anything')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('Revert: "anything"')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('revert anything')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ expect(validateCommitMessage('rEvErT anything')).toBe(VALID);
+ expect(lastError).toBe('');
+
+ });
+
+ it('should not allow "revert(scope): ..." syntax', () => {
+ const msg = 'revert(compiler): reduce generated code payload size by 65%';
+
+ expect(validateCommitMessage(msg)).toBe(INVALID);
+ expect(lastError).toContain(`'revert' is not an allowed type.\n => TYPES: ${TYPES}`);
+ });
+
+ // https://github.com/angular/angular/issues/23479
+ it('should allow typical Angular messages generated by git', () => {
+ const msg =
+ 'Revert "fix(compiler): Pretty print object instead of [Object object] (#22689)" (#23442)';
+
+ expect(validateCommitMessage(msg)).toBe(VALID);
+ expect(lastError).toBe('');
+ });
+ });
+
+ describe('(squash)', () => {
+
+ it('should strip the `squash! ` prefix and validate the rest', () => {
+ const errorMessage = `The commit message header does not match the expected format.`;
+
+ // Valid messages.
+ expect(validateCommitMessage('squash! feat(core): add feature')).toBe(VALID);
+ expect(validateCommitMessage('squash! fix: a bug', false)).toBe(VALID);
+
+ // Invalid messages.
+ expect(validateCommitMessage('squash! fix a typo', false)).toBe(INVALID);
+ expect(lastError).toContain('squash! fix a typo');
+ expect(lastError).toContain(errorMessage);
+
+ expect(validateCommitMessage('squash! squash! fix: a bug')).toBe(INVALID);
+ expect(lastError).toContain('squash! squash! fix: a bug');
+ expect(lastError).toContain(errorMessage);
+
+
+ });
+
+ describe('with `disallowSquash`', () => {
+
+ it('should fail', () => {
+ expect(validateCommitMessage('fix(core): something', true)).toBe(VALID);
+ expect(validateCommitMessage('squash! fix(core): something', true)).toBe(INVALID);
+ expect(lastError).toContain(
+ 'The commit must be manually squashed into the target commit');
+ });
+ });
+ });
+
+ describe('(fixup)', () => {
+
+ describe('without `nonFixupCommitHeaders`', () => {
+
+ it('should strip the `fixup! ` prefix and validate the rest', () => {
+ const errorMessage = `The commit message header does not match the expected format.`;
+
+ // Valid messages.
+ expect(validateCommitMessage('fixup! feat(core): add feature')).toBe(VALID);
+ expect(validateCommitMessage('fixup! fix: a bug')).toBe(VALID);
+
+ // Invalid messages.
+ expect(validateCommitMessage('fixup! fix a typo')).toBe(INVALID);
+ expect(lastError).toContain('fixup! fix a typo');
+ expect(lastError).toContain(errorMessage);
+
+ expect(validateCommitMessage('fixup! fixup! fix: a bug')).toBe(INVALID);
+ expect(lastError).toContain('fixup! fixup! fix: a bug');
+ expect(lastError).toContain(errorMessage);
+
+
+ });
+ });
+
+ describe('with `nonFixupCommitHeaders`', () => {
+
+ it('should check that the fixup commit matches a non-fixup one', () => {
+ const msg = 'fixup! foo';
+
+ expect(validateCommitMessage(msg, false, ['foo', 'bar', 'baz'])).toBe(VALID);
+ expect(validateCommitMessage(msg, false, ['bar', 'baz', 'foo'])).toBe(VALID);
+ expect(validateCommitMessage(msg, false, ['baz', 'foo', 'bar'])).toBe(VALID);
+
+ expect(validateCommitMessage(msg, false, ['qux', 'quux', 'quuux'])).toBe(INVALID);
+ expect(lastError).toContain(
+ 'Unable to find match for fixup commit among prior commits: \n' +
+ ' qux\n' +
+ ' quux\n' +
+ ' quuux');
+ });
+
+ it('should fail if `nonFixupCommitHeaders` is empty', () => {
+ expect(validateCommitMessage('refactor(core): make reactive', false, [])).toBe(VALID);
+ expect(validateCommitMessage('fixup! foo', false, [])).toBe(INVALID);
+ expect(lastError).toContain(
+ `Unable to find match for fixup commit among prior commits: -`);
+ });
+ });
+ });
+ });
+});
diff --git a/dev-infra/commit-message/validate.ts b/dev-infra/commit-message/validate.ts
new file mode 100644
index 0000000000..1213cd416c
--- /dev/null
+++ b/dev-infra/commit-message/validate.ts
@@ -0,0 +1,134 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+import {getAngularDevConfig} from '../utils/config';
+import {CommitMessageConfig} from './config';
+
+const FIXUP_PREFIX_RE = /^fixup! /i;
+const GITHUB_LINKING_RE = /((closed?s?)|(fix(es)?(ed)?)|(resolved?s?))\s\#(\d+)/ig;
+const SQUASH_PREFIX_RE = /^squash! /i;
+const REVERT_PREFIX_RE = /^revert:? /i;
+const TYPE_SCOPE_RE = /^(\w+)(?:\(([^)]+)\))?\:\s(.+)$/;
+const COMMIT_HEADER_RE = /^(.*)/i;
+const COMMIT_BODY_RE = /^.*\n\n(.*)/i;
+
+/** Parse a full commit message into its composite parts. */
+export function parseCommitMessage(commitMsg: string) {
+ let header = '';
+ let body = '';
+ let bodyWithoutLinking = '';
+ let type = '';
+ let scope = '';
+ let subject = '';
+
+ if (COMMIT_HEADER_RE.test(commitMsg)) {
+ header = COMMIT_HEADER_RE.exec(commitMsg) ![1]
+ .replace(FIXUP_PREFIX_RE, '')
+ .replace(SQUASH_PREFIX_RE, '');
+ }
+ if (COMMIT_BODY_RE.test(commitMsg)) {
+ body = COMMIT_BODY_RE.exec(commitMsg) ![1];
+ bodyWithoutLinking = body.replace(GITHUB_LINKING_RE, '');
+ }
+
+ if (TYPE_SCOPE_RE.test(header)) {
+ const parsedCommitHeader = TYPE_SCOPE_RE.exec(header) !;
+ type = parsedCommitHeader[1];
+ scope = parsedCommitHeader[2];
+ subject = parsedCommitHeader[3];
+ }
+ return {
+ header,
+ body,
+ bodyWithoutLinking,
+ type,
+ scope,
+ subject,
+ isFixup: FIXUP_PREFIX_RE.test(commitMsg),
+ isSquash: SQUASH_PREFIX_RE.test(commitMsg),
+ isRevert: REVERT_PREFIX_RE.test(commitMsg),
+ };
+}
+
+
+/** Validate a commit message against using the local repo's config. */
+export function validateCommitMessage(
+ commitMsg: string, disallowSquash: boolean = false, nonFixupCommitHeaders?: string[]) {
+ function error(errorMessage: string) {
+ console.error(
+ `INVALID COMMIT MSG: \n` +
+ `${'─'.repeat(40)}\n` +
+ `${commitMsg}\n` +
+ `${'─'.repeat(40)}\n` +
+ `ERROR: \n` +
+ ` ${errorMessage}` +
+ `\n\n` +
+ `The expected format for a commit is: \n` +
+ `<type>(<scope>): <subject>\n\n<body>`);
+ }
+
+ const config = getAngularDevConfig<'commitMessage', CommitMessageConfig>().commitMessage;
+ const commit = parseCommitMessage(commitMsg);
+
+ if (commit.isRevert) {
+ return true;
+ }
+
+ if (commit.isSquash && disallowSquash) {
+ error('The commit must be manually squashed into the target commit');
+ return false;
+ }
+
+ // If it is a fixup commit and `nonFixupCommitHeaders` is not empty, we only care to check whether
+ // there is a corresponding non-fixup commit (i.e. a commit whose header is identical to this
+ // commit's header after stripping the `fixup! ` prefix).
+ if (commit.isFixup && nonFixupCommitHeaders) {
+ if (!nonFixupCommitHeaders.includes(commit.header)) {
+ error(
+ 'Unable to find match for fixup commit among prior commits: ' +
+ (nonFixupCommitHeaders.map(x => `\n ${x}`).join('') || '-'));
+ return false;
+ }
+
+ return true;
+ }
+
+ if (commit.header.length > config.maxLineLength) {
+ error(`The commit message header is longer than ${config.maxLineLength} characters`);
+ return false;
+ }
+
+ if (!commit.type) {
+ error(`The commit message header does not match the expected format.`);
+ return false;
+ }
+
+ if (!config.types.includes(commit.type)) {
+ error(`'${commit.type}' is not an allowed type.\n => TYPES: ${config.types.join(', ')}`);
+ return false;
+ }
+
+ if (commit.scope && !config.scopes.includes(commit.scope)) {
+ error(`'${commit.scope}' is not an allowed scope.\n => SCOPES: ${config.scopes.join(', ')}`);
+ return false;
+ }
+
+ if (commit.bodyWithoutLinking.trim().length < config.minBodyLength) {
+ error(
+ `The commit message body does not meet the minimum length of ${config.minBodyLength} characters`);
+ return false;
+ }
+
+ const bodyByLine = commit.body.split('\n');
+ if (bodyByLine.some(line => line.length > config.maxLineLength)) {
+ error(
+ `The commit messsage body contains lines greater than ${config.maxLineLength} characters`);
+ return false;
+ }
+
+ return true;
+}
diff --git a/dev-infra/utils/BUILD.bazel b/dev-infra/utils/BUILD.bazel
index 44785fa1b8..35637ade55 100644
--- a/dev-infra/utils/BUILD.bazel
+++ b/dev-infra/utils/BUILD.bazel
@@ -5,6 +5,7 @@ ts_library(
srcs = [
"config.ts",
],
+ module_name = "@angular/dev-infra-private/utils",
visibility = ["//dev-infra:__subpackages__"],
deps = [
"@npm//@types/json5",
diff --git a/dev-infra/utils/config.ts b/dev-infra/utils/config.ts
index ad92b8946f..78b165d223 100644
--- a/dev-infra/utils/config.ts
+++ b/dev-infra/utils/config.ts
@@ -5,16 +5,15 @@
* 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
*/
-import {parse} from 'json5';
import {readFileSync} from 'fs';
+import {parse} from 'json5';
import {join} from 'path';
import {exec} from 'shelljs';
-
/**
* Gets the path of the directory for the repository base.
*/
-function getRepoBaseDir() {
+export function getRepoBaseDir() {
const baseRepoDir = exec(`git rev-parse --show-toplevel`, {silent: true});
if (baseRepoDir.code) {
throw Error(
@@ -28,7 +27,7 @@ function getRepoBaseDir() {
/**
* Retrieve the configuration from the .dev-infra.json file.
*/
-export function getAngularDevConfig(): DevInfraConfig {
+export function getAngularDevConfig<K, T>(): DevInfraConfig<K, T> {
const configPath = join(getRepoBaseDir(), '.dev-infra.json');
let rawConfig = '';
try {
@@ -41,5 +40,8 @@ export function getAngularDevConfig(): DevInfraConfig {
return parse(rawConfig);
}
-// Interface exressing the expected structure of the DevInfraConfig.
-export interface DevInfraConfig {}
\ No newline at end of file
+/**
+ * Interface exressing the expected structure of the DevInfraConfig.
+ * Allows for providing a typing for a part of the config to read.
+ */
+export interface DevInfraConfig<K, T> { [K: string]: T; }