cargdev/angular
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(dev-infra): use commit message validation from @angular/dev-infra-private (#36172)

Browse Source 
Prior to this change we manage a local version of commit message validation
in addition to the commit message validation tool contained in the ng-dev
tooling.  By adding the ability to validate a range of commit messages
together, the remaining piece of commit message validation that is in the
local version is replicated.

We use both commands provided by the `ng-dev commit-message` tooling:
- pre-commit-validate: Set to automatically run on an git hook to validate
    commits as they are created locally.
- validate-range: Run by CI for every PR, testing that all of the commits
    added by the PR are valid when considered together.  Ensuring that all
    fixups are matched to another commit in the change.

PR Close #36172
This commit is contained in:
Joey Perrott
2020-03-20 12:24:12 -07:00
committed by Kara Erickson
parent 4894220acf
commit 61db817eed
18 changed files with 129 additions and 569 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
dev-infra/commit-message/BUILD.bazel
View File

@ -8,13 +8,16 @@ ts_library(
"config.ts",
"validate.ts",
"validate-file.ts",
"validate-range.ts",
],
module_name = "@angular/dev-infra-private/commit-message",
visibility = ["//dev-infra:__subpackages__"],
deps = [
"//dev-infra/utils:config",
"@npm//@types/node",
"@npm//@types/shelljs",
"@npm//@types/yargs",
"@npm//shelljs",
"@npm//tslib",
"@npm//yargs",
],

33
dev-infra/commit-message/cli.ts
View File

@ -7,13 +7,38 @@
*/
import * as yargs from 'yargs';
import {validateFile} from './validate-file';
import {validateCommitRange} from './validate-range';
/** Build the parser for the commit-message commands. */
export function buildCommitMessageParser(localYargs: yargs.Argv) {
return localYargs.help().strict().command(
'pre-commit-validate', 'Validate the most recent commit message', {}, () => {
validateFile('.git/COMMIT_EDITMSG');
});
return localYargs.help()
.strict()
.command(
'pre-commit-validate', 'Validate the most recent commit message', {},
() => {
validateFile('.git/COMMIT_EDITMSG');
})
.command(
'validate-range', 'Validate a range of commit messages', {
'range': {
description: 'The range of commits to check, e.g. --range abc123..xyz456',
demandOption: ' A range must be provided, e.g. --range abc123..xyz456',
type: 'string',
requiresArg: true,
},
},
argv => {
// If on CI, and not pull request number is provided, assume the branch
// being run on is an upstream branch.
if (process.env['CI'] && process.env['CI_PULL_REQUEST'] === 'false') {
console.info(
`Since valid commit messages are enforced by PR linting on CI, we do not\n` +
`need to validate commit messages on CI runs on upstream branches.\n\n` +
`Skipping check of provided commit range`);
return;
}
validateCommitRange(argv.range);
});
}
if (require.main == module) {

2
dev-infra/commit-message/config.ts
View File

@ -10,4 +10,4 @@ export interface CommitMessageConfig {
minBodyLength: number;
types: string[];
scopes: string[];
}
}

3
dev-infra/commit-message/validate-file.ts
View File

@ -17,5 +17,8 @@ export function validateFile(filePath: string) {
const commitMessage = readFileSync(join(getRepoBaseDir(), filePath), 'utf8');
if (validateCommitMessage(commitMessage)) {
console.info('√ Valid commit message');
return;
}
// If the validation did not return true, exit as a failure.
process.exit(1);
}

45
dev-infra/commit-message/validate-range.ts Normal file
View File

@ -0,0 +1,45 @@
/**
* @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 {exec} from 'shelljs';
import {parseCommitMessage, validateCommitMessage, ValidateCommitMessageOptions} from './validate';
// Whether the provided commit is a fixup commit.
const isNonFixup = (m: string) => !parseCommitMessage(m).isFixup;
/** Validate all commits in a provided git commit range. */
export function validateCommitRange(range: string) {
// A random value is used as a string to allow for a definite split point in the git log result.
const randomValueSeparator = `${Math.random()}`;
// Custom git log format that provides the commit header and body, separated as expected with
// the custom separator as the trailing value.
const gitLogFormat = `%s%n%n%b${randomValueSeparator}`;
// Retrieve the commits in the provided range.
const result = exec(`git log --reverse --format=${gitLogFormat} ${range}`, {silent: true});
if (result.code) {
throw new Error(`Failed to get all commits in the range: \n ${result.stderr}`);
}
// Separate the commits from a single string into individual commits
const commits = result.split(randomValueSeparator).map(l => l.trim()).filter(line => !!line);
console.info(`Examining ${commits.length} commit(s) in the provided range: ${range}`);
// Check each commit in the commit range. Commits are allowed to be fixup commits for other
// commits in the provided commit range.
const allCommitsInRangeValid = commits.every((m, i) => {
const options: ValidateCommitMessageOptions = {
disallowSquash: true,
nonFixupCommitHeaders: isNonFixup(m) ? undefined : commits.slice(0, i).filter(isNonFixup)
};
return validateCommitMessage(m, options);
});
if (allCommitsInRangeValid) {
console.info('√ All commit messages in range valid.');
}
}

52
dev-infra/commit-message/validate.spec.ts
View File

@ -50,7 +50,6 @@ describe('validate-commit-message.js', () => {
});
describe('validateMessage()', () => {
it('should be valid', () => {
expect(validateCommitMessage('feat(packaging): something')).toBe(VALID);
expect(lastError).toBe('');
@ -66,7 +65,6 @@ describe('validate-commit-message.js', () => {
expect(validateCommitMessage('Revert: "release(packaging): something"')).toBe(VALID);
expect(lastError).toBe('');
});
it('should validate max length', () => {
@ -74,8 +72,8 @@ describe('validate-commit-message.js', () => {
'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`);
expect(lastError).toContain(`The commit message header is longer than ${
config.commitMessage.maxLineLength} characters`);
});
it('should validate "<type>(<scope>): <subject>" format', () => {
@ -130,7 +128,6 @@ describe('validate-commit-message.js', () => {
});
describe('(revert)', () => {
it('should allow valid "revert: ..." syntaxes', () => {
expect(validateCommitMessage('revert: anything')).toBe(VALID);
expect(lastError).toBe('');
@ -143,7 +140,6 @@ describe('validate-commit-message.js', () => {
expect(validateCommitMessage('rEvErT anything')).toBe(VALID);
expect(lastError).toBe('');
});
it('should not allow "revert(scope): ..." syntax', () => {
@ -164,31 +160,29 @@ describe('validate-commit-message.js', () => {
});
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);
expect(validateCommitMessage('squash! fix: a bug', {disallowSquash: false})).toBe(VALID);
// Invalid messages.
expect(validateCommitMessage('squash! fix a typo', false)).toBe(INVALID);
expect(validateCommitMessage('squash! fix a typo', {disallowSquash: 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(validateCommitMessage('fix(core): something', {disallowSquash: true})).toBe(VALID);
expect(validateCommitMessage('squash! fix(core): something', {
disallowSquash: true
})).toBe(INVALID);
expect(lastError).toContain(
'The commit must be manually squashed into the target commit');
});
@ -196,9 +190,7 @@ describe('validate-commit-message.js', () => {
});
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.`;
@ -214,21 +206,26 @@ describe('validate-commit-message.js', () => {
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, {disallowSquash: false, nonFixupCommitHeaders: ['foo', 'bar', 'baz']}))
.toBe(VALID);
expect(validateCommitMessage(
msg, {disallowSquash: false, nonFixupCommitHeaders: ['bar', 'baz', 'foo']}))
.toBe(VALID);
expect(validateCommitMessage(
msg, {disallowSquash: false, nonFixupCommitHeaders: ['baz', 'foo', 'bar']}))
.toBe(VALID);
expect(validateCommitMessage(msg, false, ['qux', 'quux', 'quuux'])).toBe(INVALID);
expect(validateCommitMessage(
msg, {disallowSquash: false, nonFixupCommitHeaders: ['qux', 'quux', 'quuux']}))
.toBe(INVALID);
expect(lastError).toContain(
'Unable to find match for fixup commit among prior commits: \n' +
' qux\n' +
@ -237,8 +234,13 @@ describe('validate-commit-message.js', () => {
});
it('should fail if `nonFixupCommitHeaders` is empty', () => {
expect(validateCommitMessage('refactor(core): make reactive', false, [])).toBe(VALID);
expect(validateCommitMessage('fixup! foo', false, [])).toBe(INVALID);
expect(validateCommitMessage('refactor(core): make reactive', {
disallowSquash: false,
nonFixupCommitHeaders: []
})).toBe(VALID);
expect(validateCommitMessage(
'fixup! foo', {disallowSquash: false, nonFixupCommitHeaders: []}))
.toBe(INVALID);
expect(lastError).toContain(
`Unable to find match for fixup commit among prior commits: -`);
});

27
dev-infra/commit-message/validate.ts
View File

@ -8,6 +8,12 @@
import {getAngularDevConfig} from '../utils/config';
import {CommitMessageConfig} from './config';
/** Options for commit message validation. */
export interface ValidateCommitMessageOptions {
disallowSquash?: boolean;
nonFixupCommitHeaders?: string[];
}
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;
@ -26,17 +32,17 @@ export function parseCommitMessage(commitMsg: string) {
let subject = '';
if (COMMIT_HEADER_RE.test(commitMsg)) {
header = COMMIT_HEADER_RE.exec(commitMsg) ![1]
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];
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) !;
const parsedCommitHeader = TYPE_SCOPE_RE.exec(header)!;
type = parsedCommitHeader[1];
scope = parsedCommitHeader[2];
subject = parsedCommitHeader[3];
@ -54,10 +60,9 @@ export function parseCommitMessage(commitMsg: string) {
};
}
/** Validate a commit message against using the local repo's config. */
export function validateCommitMessage(
commitMsg: string, disallowSquash: boolean = false, nonFixupCommitHeaders?: string[]) {
commitMsg: string, options: ValidateCommitMessageOptions = {}) {
function error(errorMessage: string) {
console.error(
`INVALID COMMIT MSG: \n` +
@ -78,7 +83,7 @@ export function validateCommitMessage(
return true;
}
if (commit.isSquash && disallowSquash) {
if (commit.isSquash && options.disallowSquash) {
error('The commit must be manually squashed into the target commit');
return false;
}
@ -86,11 +91,11 @@ export function validateCommitMessage(
// 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)) {
if (commit.isFixup && options.nonFixupCommitHeaders) {
if (!options.nonFixupCommitHeaders.includes(commit.header)) {
error(
'Unable to find match for fixup commit among prior commits: ' +
(nonFixupCommitHeaders.map(x => `\n ${x}`).join('') || '-'));
(options.nonFixupCommitHeaders.map(x => `\n ${x}`).join('') || '-'));
return false;
}
@ -118,8 +123,8 @@ export function validateCommitMessage(
}
if (commit.bodyWithoutLinking.trim().length < config.minBodyLength) {
error(
`The commit message body does not meet the minimum length of ${config.minBodyLength} characters`);
error(`The commit message body does not meet the minimum length of ${
config.minBodyLength} characters`);
return false;
}