From 7d89b8e215bb3d5349a614a6f6cc4d437d8a69e6 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 19 Jan 2017 16:24:47 -0800 Subject: [PATCH] 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 --- CONTRIBUTING.md | 41 ++++-- gulpfile.js | 34 ++++- scripts/ci-lite/test_js.sh | 4 + .../commit-message.json | 35 ++++++ tools/validate-commit-message/index.js | 1 + .../spec/support/jasmine.json | 11 ++ .../validate-commit-message.js | 58 +++++++++ .../validate-commit-message.spec.js | 117 ++++++++++++++++++ 8 files changed, 292 insertions(+), 9 deletions(-) create mode 100644 tools/validate-commit-message/commit-message.json create mode 100644 tools/validate-commit-message/index.js create mode 100644 tools/validate-commit-message/spec/support/jasmine.json create mode 100644 tools/validate-commit-message/validate-commit-message.js create mode 100644 tools/validate-commit-message/validate-commit-message.spec.js diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 109cd9fc57bfbc..92b771b1d5558d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -191,21 +191,46 @@ If the commit reverts a previous commit, it should begin with `revert: `, follow ### Type Must be one of the following: +* **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) +* **docs**: Documentation only changes * **feat**: A new feature * **fix**: A bug fix -* **docs**: Documentation only changes +* **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) -* **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 ### 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: diff --git a/gulpfile.js b/gulpfile.js index ff2c446ae026e3..21a832938715cc 100644 --- a/gulpfile.js +++ b/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,38 @@ 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'); + + 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 diff --git a/scripts/ci-lite/test_js.sh b/scripts/ci-lite/test_js.sh index 7f0c22b12378af..1c7bac81a0b034 100755 --- a/scripts/ci-lite/test_js.sh +++ b/scripts/ci-lite/test_js.sh @@ -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' diff --git a/tools/validate-commit-message/commit-message.json b/tools/validate-commit-message/commit-message.json new file mode 100644 index 00000000000000..ec19bb7e13a398 --- /dev/null +++ b/tools/validate-commit-message/commit-message.json @@ -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" + ] +} diff --git a/tools/validate-commit-message/index.js b/tools/validate-commit-message/index.js new file mode 100644 index 00000000000000..7bb7b84edc33f9 --- /dev/null +++ b/tools/validate-commit-message/index.js @@ -0,0 +1 @@ +module.exports = require('./validate-commit-message'); \ No newline at end of file diff --git a/tools/validate-commit-message/spec/support/jasmine.json b/tools/validate-commit-message/spec/support/jasmine.json new file mode 100644 index 00000000000000..31a39bc6a2af7a --- /dev/null +++ b/tools/validate-commit-message/spec/support/jasmine.json @@ -0,0 +1,11 @@ +{ + "spec_dir": "", + "spec_files": [ + "**/*[sS]pec.js" + ], + "helpers": [ + "helpers/**/*.js" + ], + "stopSpecOnExpectationFailure": false, + "random": false +} diff --git a/tools/validate-commit-message/validate-commit-message.js b/tools/validate-commit-message/validate-commit-message.js new file mode 100644 index 00000000000000..bcb326df8e331d --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.js @@ -0,0 +1,58 @@ +#!/usr/bin/env node + +/** + * GIT COMMIT-MSG hook for validating commit message. + * See https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines + * + * Installation: + * >> cd + * >> ln -s ../../tools/validate-commit-message/validate-commit-message.js .git/hooks/commit-msg + * >> chmod +x .git/hooks/commit-msg + * + * 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+)(?:\(([\w\$\.\*/-]*)\))?\: (.+)$/; + +module.exports = function(commitMessage) { + if (commitMessage.length > config['maxLength']) { + error(`The commit message is longer than ${config['maxLength']} characters`, commitMessage); + return false; + } + + const match = PATTERN.exec(commitMessage); + if (!match || match[2] === 'revert') { + error( + `The commit message does not match the format of "(): OR revert: type(): "`, + commitMessage); + return false; + } + + const type = match[2]; + if (config['types'].indexOf(type) === -1) { + error( + `${type} is not an allowed type.\n => TYPES: ${config['types'].join('|')}`, commitMessage); + return false; + } + + const scope = match[3]; + + if (scope && !config['scopes'].includes(scope)) { + error( + `"${scope}" inside of "${type}(...)" is not an allowed scope.\n => SCOPES: ${config['scopes'].join('|')}`, + commitMessage); + return false; + } + + return true; +}; + +function error(errorMessage, commitMessage) { + console.error(`INVALID COMMIT MSG: "${commitMessage}"\n => ERROR: ${errorMessage}`); +} diff --git a/tools/validate-commit-message/validate-commit-message.spec.js b/tools/validate-commit-message/validate-commit-message.spec.js new file mode 100644 index 00000000000000..8a437ba147da79 --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.spec.js @@ -0,0 +1,117 @@ +'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); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "fix(Compiler): something"\n' + + ' => ERROR: "Compiler" inside of "fix(...)" 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" inside of "feat(...)" 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" inside of "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', + 'INVALID COMMIT MSG: "style(webworker): something"\n' + + ' => ERROR: "webworker" inside of "style(...)" 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" inside of "refactor(...)" 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" inside of "refactor(...)" 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' + ]); + }); + + + 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 "(): " 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 "(): OR revert: type(): "' + ]); + }); + + + 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 "(): OR revert: type(): "' + ]); + }); + + + 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); }); + }); +}); \ No newline at end of file