ci: validate the message of each new commit as part of the CI linting
This patch adds the gulp command of `validate-commit-messages` which will validate the range of commits messages present in the active branch. This check now runs on CI as part of the linting checks. Allowed commit message types and scopes are controlled via commit-message.json file and documented at https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines This solution is based on old Vojta's code that he wrote for angular/angular.js, that was later adjusted by @matsko in #13815. Ideally we should switch over to something like https://www.npmjs.com/package/commitplease as suggested in #9953 but that package currently doesn't support strict scope checking, which is one of the primarily goal of this PR. Note that this PR removes support for "chore" which was previously overused by everyone on the team. Closes #13815 Fixes #3337
This commit is contained in:
Igor Minar
Alex Rickabaugh
parent
ba52b2e08c
commit
dff6ee3272
|
|
@ -191,21 +191,46 @@ If the commit reverts a previous commit, it should begin with `revert: `, follow
|
|||
### Type
|
||||
Must be one of the following:
|
||||
|
||||
* **feat**: A new feature
|
||||
* **fix**: A bug fix
|
||||
* **docs**: Documentation only changes
|
||||
* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing
|
||||
semi-colons, etc)
|
||||
* **refactor**: A code change that neither fixes a bug nor adds a feature
|
||||
* **perf**: A code change that improves performance
|
||||
* **test**: Adding missing tests or correcting existing tests
|
||||
* **build**: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
|
||||
* **ci**: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
|
||||
* **chore**: Other changes that don't modify `src` or `test` files
|
||||
* **docs**: Documentation only changes
|
||||
* **feat**: A new feature
|
||||
* **fix**: A bug fix
|
||||
* **perf**: A code change that improves performance
|
||||
* **refactor**: A code change that neither fixes a bug nor adds a feature
|
||||
* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing
|
||||
semi-colons, etc)
|
||||
* **test**: Adding missing tests or correcting existing tests
|
||||
|
||||
### Scope
|
||||
The scope could be anything specifying place of the commit change. For example
|
||||
`Compiler`, `ElementInjector`, etc.
|
||||
The scope should be the name of the npm package affected (as perceived by person reading changelog generated from commit messages.
|
||||
|
||||
The following is the list of supported scopes:
|
||||
|
||||
* **common**
|
||||
* **compiler**
|
||||
* **compiler-cli**
|
||||
* **core**
|
||||
* **forms**
|
||||
* **http**
|
||||
* **language-service**
|
||||
* **platform-browser**
|
||||
* **platform-browser-dynamic**
|
||||
* **platform-server**
|
||||
* **platform-webworker**
|
||||
* **platform-webworker-dynamic**
|
||||
* **router**
|
||||
* **upgrade**
|
||||
* **tsc-wrapped**
|
||||
|
||||
There is currently few exception to the "use package name" rule:
|
||||
|
||||
* **packaging**: used for changes that change the npm package layout in all of our packages, e.g. public path changes, package.json changes done to all packages, d.ts file/format changes, changes to bundles, etc.
|
||||
* **changelog**: used for updating the release notes in CHANGELOG.md
|
||||
* none/empty string: useful for `style`, `test` and `refactor` changes that are done across all packages (e.g. `style: add missing semicolons`)
|
||||
|
||||
|
||||
Packaging
|
||||
|
||||
### Subject
|
||||
The subject contains succinct description of the change:
|
||||
|
|
|
|||
36
gulpfile.js
36
gulpfile.js
|
|
@ -126,7 +126,7 @@ gulp.task('public-api:update', ['build.sh'], (done) => {
|
|||
});
|
||||
|
||||
// Check the coding standards and programming errors
|
||||
gulp.task('lint', ['format:enforce', 'tools:build'], () => {
|
||||
gulp.task('lint', ['format:enforce', 'tools:build', 'validate-commit-messages'], () => {
|
||||
const tslint = require('gulp-tslint');
|
||||
// Built-in rules are at
|
||||
// https://palantir.github.io/tslint/rules/
|
||||
|
|
@ -155,6 +155,40 @@ gulp.task('lint', ['format:enforce', 'tools:build'], () => {
|
|||
.pipe(tslint.report({emitError: true}));
|
||||
});
|
||||
|
||||
gulp.task('validate-commit-messages', () => {
|
||||
const validateCommitMessage = require('./tools/validate-commit-message');
|
||||
const childProcess = require('child_process');
|
||||
|
||||
// We need to fetch origin explicitly because it might be stale.
|
||||
// I couldn't find a reliable way to do this without fetch.
|
||||
childProcess.exec(
|
||||
'git fetch origin master && git log --reverse --format=%s HEAD ^origin/master',
|
||||
(error, stdout, stderr) => {
|
||||
if (error) {
|
||||
console.log(stderr);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
let someCommitsInvalid = false;
|
||||
let commitsByLine = stdout.trim().split(/\n/);
|
||||
|
||||
console.log(`Examining ${commitsByLine.length} commits between HEAD and master`);
|
||||
|
||||
if (commitsByLine.length == 0) {
|
||||
console.log('There are zero new commits between this HEAD and master');
|
||||
}
|
||||
|
||||
someCommitsInvalid = !commitsByLine.every(validateCommitMessage);
|
||||
|
||||
if (someCommitsInvalid) {
|
||||
console.log('Please fix the failing commit messages before continuing...');
|
||||
console.log(
|
||||
'Commit message guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines');
|
||||
process.exit(1);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
gulp.task('tools:build', (done) => { tsc('tools/', done); });
|
||||
|
||||
// Check for circular dependency in the source code
|
||||
|
|
|
|||
|
|
@ -20,6 +20,10 @@ echo 'travis_fold:start:test.unit.tools'
|
|||
# Run unit tests in tools
|
||||
node ./dist/tools/tsc-watch/ tools runCmdsOnly
|
||||
|
||||
cd tools/validate-commit-message
|
||||
$(npm bin)/jasmine
|
||||
cd -
|
||||
|
||||
echo 'travis_fold:end:test.unit.tools'
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,35 @@
|
|||
{
|
||||
"maxLength": 100,
|
||||
"types": [
|
||||
"build",
|
||||
"ci",
|
||||
"docs",
|
||||
"feat",
|
||||
"fix",
|
||||
"perf",
|
||||
"refactor",
|
||||
"style",
|
||||
"test"
|
||||
],
|
||||
"scopes": [
|
||||
"benchpress",
|
||||
"common",
|
||||
"compiler",
|
||||
"compiler-cli",
|
||||
"core",
|
||||
"forms",
|
||||
"http",
|
||||
"language-service",
|
||||
"platform-browser",
|
||||
"platform-browser-dynamic",
|
||||
"platform-server",
|
||||
"platform-webworker",
|
||||
"platform-webworker-dynamic",
|
||||
"router",
|
||||
"upgrade",
|
||||
"tsc-wrapped",
|
||||
|
||||
"packaging",
|
||||
"changelog"
|
||||
]
|
||||
}
|
||||
|
|
@ -0,0 +1 @@
|
|||
module.exports = require('./validate-commit-message');
|
||||
|
|
@ -0,0 +1,11 @@
|
|||
{
|
||||
"spec_dir": "",
|
||||
"spec_files": [
|
||||
"**/*[sS]pec.js"
|
||||
],
|
||||
"helpers": [
|
||||
"helpers/**/*.js"
|
||||
],
|
||||
"stopSpecOnExpectationFailure": false,
|
||||
"random": false
|
||||
}
|
||||
|
|
@ -0,0 +1,52 @@
|
|||
#!/usr/bin/env node
|
||||
|
||||
/**
|
||||
* GIT commit message format enforcement
|
||||
*
|
||||
* Note: this script was originally written by Vojta for AngularJS :-)
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const configPath = path.resolve(__dirname, './commit-message.json');
|
||||
const config = JSON.parse(fs.readFileSync(configPath, 'utf8'));
|
||||
const PATTERN = /^(revert\: )?(\w+)(?:\(([^)]+)\))?\: (.+)$/;
|
||||
|
||||
module.exports = function(commitSubject) {
|
||||
if (commitSubject.length > config['maxLength']) {
|
||||
error(`The commit message is longer than ${config['maxLength']} characters`, commitSubject);
|
||||
return false;
|
||||
}
|
||||
|
||||
const match = PATTERN.exec(commitSubject);
|
||||
if (!match || match[2] === 'revert') {
|
||||
error(
|
||||
`The commit message does not match the format of "<type>(<scope>): <subject> OR revert: type(<scope>): <subject>"`,
|
||||
commitSubject);
|
||||
return false;
|
||||
}
|
||||
|
||||
const type = match[2];
|
||||
if (config['types'].indexOf(type) === -1) {
|
||||
error(
|
||||
`${type} is not an allowed type.\n => TYPES: ${config['types'].join(', ')}`, commitSubject);
|
||||
return false;
|
||||
}
|
||||
|
||||
const scope = match[3];
|
||||
|
||||
if (scope && !config['scopes'].includes(scope)) {
|
||||
error(
|
||||
`"${scope}" is not an allowed scope.\n => SCOPES: ${config['scopes'].join(', ')}`,
|
||||
commitSubject);
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
};
|
||||
|
||||
function error(errorMessage, commitMessage) {
|
||||
console.error(`INVALID COMMIT MSG: "${commitMessage}"\n => ERROR: ${errorMessage}`);
|
||||
}
|
||||
|
|
@ -0,0 +1,119 @@
|
|||
'use strict';
|
||||
|
||||
describe('validate-commit-message.js', function() {
|
||||
var validateMessage = require('./validate-commit-message');
|
||||
var errors = [];
|
||||
var logs = [];
|
||||
|
||||
var VALID = true;
|
||||
var INVALID = false;
|
||||
|
||||
beforeEach(function() {
|
||||
errors.length = 0;
|
||||
logs.length = 0;
|
||||
|
||||
spyOn(console, 'error').and.callFake(function(msg) {
|
||||
errors.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor
|
||||
});
|
||||
|
||||
spyOn(console, 'log').and.callFake(function(msg) {
|
||||
logs.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor
|
||||
});
|
||||
});
|
||||
|
||||
describe('validateMessage', function() {
|
||||
|
||||
it('should be valid', function() {
|
||||
expect(validateMessage('fix(core): something')).toBe(VALID);
|
||||
expect(validateMessage('feat(common): something')).toBe(VALID);
|
||||
expect(validateMessage('docs(compiler): something')).toBe(VALID);
|
||||
expect(validateMessage('style(http): something')).toBe(VALID);
|
||||
expect(validateMessage('refactor(platform-webworker): something')).toBe(VALID);
|
||||
expect(validateMessage('test(language-service): something')).toBe(VALID);
|
||||
expect(validateMessage('test(packaging): something')).toBe(VALID);
|
||||
expect(errors).toEqual([]);
|
||||
});
|
||||
|
||||
|
||||
it('should fail when scope is invalid', function() {
|
||||
expect(validateMessage('fix(Compiler): something')).toBe(INVALID);
|
||||
expect(validateMessage('feat(bah): something')).toBe(INVALID);
|
||||
expect(validateMessage('docs(animations): something')).toBe(INVALID);
|
||||
expect(validateMessage('style(webworker): something')).toBe(INVALID);
|
||||
expect(validateMessage('refactor(security): something')).toBe(INVALID);
|
||||
expect(validateMessage('refactor(docs): something')).toBe(INVALID);
|
||||
['INVALID COMMIT MSG: "fix(Compiler): something"\n' +
|
||||
' => ERROR: "Compiler" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog',
|
||||
'INVALID COMMIT MSG: "feat(bah): something"\n' +
|
||||
' => ERROR: "bah" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog',
|
||||
'INVALID COMMIT MSG: "docs(animations): something"\n' +
|
||||
' => ERROR: "animations" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog',
|
||||
'INVALID COMMIT MSG: "style(webworker): something"\n' +
|
||||
' => ERROR: "webworker" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog',
|
||||
'INVALID COMMIT MSG: "refactor(security): something"\n' +
|
||||
' => ERROR: "security" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog',
|
||||
'INVALID COMMIT MSG: "refactor(docs): something"\n' +
|
||||
' => ERROR: "docs" is not an allowed scope.\n' +
|
||||
' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog']
|
||||
.forEach((expectedErrorMessage, index) => {
|
||||
expect(expectedErrorMessage).toEqual(errors[index]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
it('should validate 100 characters length', function() {
|
||||
var msg =
|
||||
'fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... ';
|
||||
|
||||
expect(validateMessage(msg)).toBe(INVALID);
|
||||
expect(errors).toEqual([
|
||||
'INVALID COMMIT MSG: "fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... "\n => ERROR: The commit message is longer than 100 characters'
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
it('should validate "<type>(<scope>): <subject>" format', function() {
|
||||
var msg = 'not correct format';
|
||||
|
||||
expect(validateMessage(msg)).toBe(INVALID);
|
||||
expect(errors).toEqual([
|
||||
'INVALID COMMIT MSG: "not correct format"\n => ERROR: The commit message does not match the format of "<type>(<scope>): <subject> OR revert: type(<scope>): <subject>"'
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
it('should support "revert: type(scope):" syntax and reject "revert(scope):" syntax', function() {
|
||||
let correctMsg = 'revert: fix(compiler): reduce generated code payload size by 65%';
|
||||
expect(validateMessage(correctMsg)).toBe(VALID);
|
||||
|
||||
let incorretMsg = 'revert(compiler): reduce generated code payload size by 65%';
|
||||
expect(validateMessage(incorretMsg)).toBe(INVALID);
|
||||
expect(errors).toEqual([
|
||||
'INVALID COMMIT MSG: "revert(compiler): reduce generated code payload size by 65%"\n => ERROR: The commit message does not match the format of "<type>(<scope>): <subject> OR revert: type(<scope>): <subject>"'
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
it('should validate type', function() {
|
||||
expect(validateMessage('weird($filter): something')).toBe(INVALID);
|
||||
expect(errors).toEqual(
|
||||
['INVALID COMMIT MSG: "weird($filter): something"\n' +
|
||||
' => ERROR: weird is not an allowed type.\n' +
|
||||
' => TYPES: build, ci, docs, feat, fix, perf, refactor, style, test']);
|
||||
});
|
||||
|
||||
|
||||
it('should allow empty scope',
|
||||
function() { expect(validateMessage('fix: blablabla')).toBe(VALID); });
|
||||
|
||||
// we don't want to allow WIP. it's ok to fail the PR build in this case to show that there is
|
||||
// work still to be done.
|
||||
it('should not ignore msg prefixed with "WIP: "',
|
||||
function() { expect(validateMessage('WIP: bullshit')).toBe(INVALID); });
|
||||
});
|
||||
});
|
||||
Loading…
Reference in New Issue