Skip to content

Commit dff6ee3

Browse files
IgorMinaralxhub
IgorMinar
authored and
alxhub
committed
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
1 parent ba52b2e commit dff6ee3

8 files changed

Lines changed: 290 additions & 9 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,21 +191,46 @@ If the commit reverts a previous commit, it should begin with `revert: `, follow
191191
### Type
192192
Must be one of the following:
193193

194+
* **build**: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
195+
* **ci**: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
196+
* **docs**: Documentation only changes
194197
* **feat**: A new feature
195198
* **fix**: A bug fix
196-
* **docs**: Documentation only changes
199+
* **perf**: A code change that improves performance
200+
* **refactor**: A code change that neither fixes a bug nor adds a feature
197201
* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing
198202
semi-colons, etc)
199-
* **refactor**: A code change that neither fixes a bug nor adds a feature
200-
* **perf**: A code change that improves performance
201203
* **test**: Adding missing tests or correcting existing tests
202-
* **build**: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
203-
* **ci**: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
204-
* **chore**: Other changes that don't modify `src` or `test` files
205204

206205
### Scope
207-
The scope could be anything specifying place of the commit change. For example
208-
`Compiler`, `ElementInjector`, etc.
206+
The scope should be the name of the npm package affected (as perceived by person reading changelog generated from commit messages.
207+
208+
The following is the list of supported scopes:
209+
210+
* **common**
211+
* **compiler**
212+
* **compiler-cli**
213+
* **core**
214+
* **forms**
215+
* **http**
216+
* **language-service**
217+
* **platform-browser**
218+
* **platform-browser-dynamic**
219+
* **platform-server**
220+
* **platform-webworker**
221+
* **platform-webworker-dynamic**
222+
* **router**
223+
* **upgrade**
224+
* **tsc-wrapped**
225+
226+
There is currently few exception to the "use package name" rule:
227+
228+
* **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.
229+
* **changelog**: used for updating the release notes in CHANGELOG.md
230+
* none/empty string: useful for `style`, `test` and `refactor` changes that are done across all packages (e.g. `style: add missing semicolons`)
231+
232+
233+
Packaging
209234

210235
### Subject
211236
The subject contains succinct description of the change:

‎gulpfile.js‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ gulp.task('public-api:update', ['build.sh'], (done) => {
126126
});
127127

128128
// Check the coding standards and programming errors
129-
gulp.task('lint', ['format:enforce', 'tools:build'], () => {
129+
gulp.task('lint', ['format:enforce', 'tools:build', 'validate-commit-messages'], () => {
130130
const tslint = require('gulp-tslint');
131131
// Built-in rules are at
132132
// https://palantir.github.io/tslint/rules/
@@ -155,6 +155,40 @@ gulp.task('lint', ['format:enforce', 'tools:build'], () => {
155155
.pipe(tslint.report({emitError: true}));
156156
});
157157

158+
gulp.task('validate-commit-messages', () => {
159+
const validateCommitMessage = require('./tools/validate-commit-message');
160+
const childProcess = require('child_process');
161+
162+
// We need to fetch origin explicitly because it might be stale.
163+
// I couldn't find a reliable way to do this without fetch.
164+
childProcess.exec(
165+
'git fetch origin master && git log --reverse --format=%s HEAD ^origin/master',
166+
(error, stdout, stderr) => {
167+
if (error) {
168+
console.log(stderr);
169+
process.exit(1);
170+
}
171+
172+
let someCommitsInvalid = false;
173+
let commitsByLine = stdout.trim().split(/\n/);
174+
175+
console.log(`Examining ${commitsByLine.length} commits between HEAD and master`);
176+
177+
if (commitsByLine.length == 0) {
178+
console.log('There are zero new commits between this HEAD and master');
179+
}
180+
181+
someCommitsInvalid = !commitsByLine.every(validateCommitMessage);
182+
183+
if (someCommitsInvalid) {
184+
console.log('Please fix the failing commit messages before continuing...');
185+
console.log(
186+
'Commit message guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines');
187+
process.exit(1);
188+
}
189+
});
190+
});
191+
158192
gulp.task('tools:build', (done) => { tsc('tools/', done); });
159193

160194
// Check for circular dependency in the source code

‎scripts/ci-lite/test_js.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ echo 'travis_fold:start:test.unit.tools'
2020
# Run unit tests in tools
2121
node ./dist/tools/tsc-watch/ tools runCmdsOnly
2222

23+
cd tools/validate-commit-message
24+
$(npm bin)/jasmine
25+
cd -
26+
2327
echo 'travis_fold:end:test.unit.tools'
2428

2529

‎tools/validate-commit-message/commit-message.json‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
{
2+
"maxLength": 100,
3+
"types": [
4+
"build",
5+
"ci",
6+
"docs",
7+
"feat",
8+
"fix",
9+
"perf",
10+
"refactor",
11+
"style",
12+
"test"
13+
],
14+
"scopes": [
15+
"benchpress",
16+
"common",
17+
"compiler",
18+
"compiler-cli",
19+
"core",
20+
"forms",
21+
"http",
22+
"language-service",
23+
"platform-browser",
24+
"platform-browser-dynamic",
25+
"platform-server",
26+
"platform-webworker",
27+
"platform-webworker-dynamic",
28+
"router",
29+
"upgrade",
30+
"tsc-wrapped",
31+
32+
"packaging",
33+
"changelog"
34+
]
35+
}

‎tools/validate-commit-message/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
module.exports = require('./validate-commit-message');

‎tools/validate-commit-message/spec/support/jasmine.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"spec_dir": "",
3+
"spec_files": [
4+
"**/*[sS]pec.js"
5+
],
6+
"helpers": [
7+
"helpers/**/*.js"
8+
],
9+
"stopSpecOnExpectationFailure": false,
10+
"random": false
11+
}

‎tools/validate-commit-message/validate-commit-message.js‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
#!/usr/bin/env node
2+
3+
/**
4+
* GIT commit message format enforcement
5+
*
6+
* Note: this script was originally written by Vojta for AngularJS :-)
7+
*/
8+
9+
'use strict';
10+
11+
const fs = require('fs');
12+
const path = require('path');
13+
const configPath = path.resolve(__dirname, './commit-message.json');
14+
const config = JSON.parse(fs.readFileSync(configPath, 'utf8'));
15+
const PATTERN = /^(revert\: )?(\w+)(?:\(([^)]+)\))?\: (.+)$/;
16+
17+
module.exports = function(commitSubject) {
18+
if (commitSubject.length > config['maxLength']) {
19+
error(`The commit message is longer than ${config['maxLength']} characters`, commitSubject);
20+
return false;
21+
}
22+
23+
const match = PATTERN.exec(commitSubject);
24+
if (!match || match[2] === 'revert') {
25+
error(
26+
`The commit message does not match the format of "<type>(<scope>): <subject> OR revert: type(<scope>): <subject>"`,
27+
commitSubject);
28+
return false;
29+
}
30+
31+
const type = match[2];
32+
if (config['types'].indexOf(type) === -1) {
33+
error(
34+
`${type} is not an allowed type.\n => TYPES: ${config['types'].join(', ')}`, commitSubject);
35+
return false;
36+
}
37+
38+
const scope = match[3];
39+
40+
if (scope && !config['scopes'].includes(scope)) {
41+
error(
42+
`"${scope}" is not an allowed scope.\n => SCOPES: ${config['scopes'].join(', ')}`,
43+
commitSubject);
44+
return false;
45+
}
46+
47+
return true;
48+
};
49+
50+
function error(errorMessage, commitMessage) {
51+
console.error(`INVALID COMMIT MSG: "${commitMessage}"\n => ERROR: ${errorMessage}`);
52+
}

‎tools/validate-commit-message/validate-commit-message.spec.js‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
'use strict';
2+
3+
describe('validate-commit-message.js', function() {
4+
var validateMessage = require('./validate-commit-message');
5+
var errors = [];
6+
var logs = [];
7+
8+
var VALID = true;
9+
var INVALID = false;
10+
11+
beforeEach(function() {
12+
errors.length = 0;
13+
logs.length = 0;
14+
15+
spyOn(console, 'error').and.callFake(function(msg) {
16+
errors.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor
17+
});
18+
19+
spyOn(console, 'log').and.callFake(function(msg) {
20+
logs.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor
21+
});
22+
});
23+
24+
describe('validateMessage', function() {
25+
26+
it('should be valid', function() {
27+
expect(validateMessage('fix(core): something')).toBe(VALID);
28+
expect(validateMessage('feat(common): something')).toBe(VALID);
29+
expect(validateMessage('docs(compiler): something')).toBe(VALID);
30+
expect(validateMessage('style(http): something')).toBe(VALID);
31+
expect(validateMessage('refactor(platform-webworker): something')).toBe(VALID);
32+
expect(validateMessage('test(language-service): something')).toBe(VALID);
33+
expect(validateMessage('test(packaging): something')).toBe(VALID);
34+
expect(errors).toEqual([]);
35+
});
36+
37+
38+
it('should fail when scope is invalid', function() {
39+
expect(validateMessage('fix(Compiler): something')).toBe(INVALID);
40+
expect(validateMessage('feat(bah): something')).toBe(INVALID);
41+
expect(validateMessage('docs(animations): something')).toBe(INVALID);
42+
expect(validateMessage('style(webworker): something')).toBe(INVALID);
43+
expect(validateMessage('refactor(security): something')).toBe(INVALID);
44+
expect(validateMessage('refactor(docs): something')).toBe(INVALID);
45+
['INVALID COMMIT MSG: "fix(Compiler): something"\n' +
46+
' => ERROR: "Compiler" is not an allowed scope.\n' +
47+
' => 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',
48+
'INVALID COMMIT MSG: "feat(bah): something"\n' +
49+
' => ERROR: "bah" is not an allowed scope.\n' +
50+
' => 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',
51+
'INVALID COMMIT MSG: "docs(animations): something"\n' +
52+
' => ERROR: "animations" is not an allowed scope.\n' +
53+
' => 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',
54+
'INVALID COMMIT MSG: "style(webworker): something"\n' +
55+
' => ERROR: "webworker" is not an allowed scope.\n' +
56+
' => 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',
57+
'INVALID COMMIT MSG: "refactor(security): something"\n' +
58+
' => ERROR: "security" is not an allowed scope.\n' +
59+
' => 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',
60+
'INVALID COMMIT MSG: "refactor(docs): something"\n' +
61+
' => ERROR: "docs" is not an allowed scope.\n' +
62+
' => 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']
63+
.forEach((expectedErrorMessage, index) => {
64+
expect(expectedErrorMessage).toEqual(errors[index]);
65+
});
66+
});
67+
68+
69+
it('should validate 100 characters length', function() {
70+
var msg =
71+
'fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... ';
72+
73+
expect(validateMessage(msg)).toBe(INVALID);
74+
expect(errors).toEqual([
75+
'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'
76+
]);
77+
});
78+
79+
80+
it('should validate "<type>(<scope>): <subject>" format', function() {
81+
var msg = 'not correct format';
82+
83+
expect(validateMessage(msg)).toBe(INVALID);
84+
expect(errors).toEqual([
85+
'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>"'
86+
]);
87+
});
88+
89+
90+
it('should support "revert: type(scope):" syntax and reject "revert(scope):" syntax', function() {
91+
let correctMsg = 'revert: fix(compiler): reduce generated code payload size by 65%';
92+
expect(validateMessage(correctMsg)).toBe(VALID);
93+
94+
let incorretMsg = 'revert(compiler): reduce generated code payload size by 65%';
95+
expect(validateMessage(incorretMsg)).toBe(INVALID);
96+
expect(errors).toEqual([
97+
'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>"'
98+
]);
99+
});
100+
101+
102+
it('should validate type', function() {
103+
expect(validateMessage('weird($filter): something')).toBe(INVALID);
104+
expect(errors).toEqual(
105+
['INVALID COMMIT MSG: "weird($filter): something"\n' +
106+
' => ERROR: weird is not an allowed type.\n' +
107+
' => TYPES: build, ci, docs, feat, fix, perf, refactor, style, test']);
108+
});
109+
110+
111+
it('should allow empty scope',
112+
function() { expect(validateMessage('fix: blablabla')).toBe(VALID); });
113+
114+
// we don't want to allow WIP. it's ok to fail the PR build in this case to show that there is
115+
// work still to be done.
116+
it('should not ignore msg prefixed with "WIP: "',
117+
function() { expect(validateMessage('WIP: bullshit')).toBe(INVALID); });
118+
});
119+
});

0 commit comments

Comments
 (0)