refactor: add a commit-msg git hook to check commit messages #22969

vicb · 2018-03-23T23:24:19Z

The commit command will fail if the commit message header does not follow the
Angular conventions as defined in /CONTRIBUTING.md.

You can force the commit by adding the --no-verify option.

NOTE:
You should remove all unused hooks (in /.git/hooks) before running
yarn so that husky hooks are installed correctly.

The idea is to merge this one first and then I can work on the formatting hook to be merged after a few days (when we get feedback about how this one work for everybody).

Exemple output (success/fail)

$ git commit -m 'feat(compiler): blabla'
Found '/Users/berchet/Code/angular/.nvmrc' with version <8.9>
Now using node v8.9.4 (npm v5.6.0)
Found '/Users/berchet/Code/angular/.nvmrc' with version <8.9>
Now using node v8.9.4 (npm v5.6.0)
husky > npm run -s commitmsg (node v8.9.4)

[0323-checkMsgHook 214d6eb1c8] feat(compiler): blabla
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 scripts/git/commit-msg.js

$ git commit --amend -m 'sdfkjghfsdkj'
Found '/Users/berchet/Code/angular/.nvmrc' with version <8.9>
Now using node v8.9.4 (npm v5.6.0)
Found '/Users/berchet/Code/angular/.nvmrc' with version <8.9>
Now using node v8.9.4 (npm v5.6.0)
husky > npm run -s commitmsg (node v8.9.4)

INVALID COMMIT MSG: "sdfkjghfsdkj"
 => ERROR: The commit message does not match the format of '<type>(<scope>): <subject>' OR 'Revert: "type(<scope>): <subject>"'

husky > commit-msg hook failed (add --no-verify to bypass)

The commit command will fail if the commit message header does not follow the Angular convetions as defined in /CONTRIBUTING.md. You can force the commit by adding the `--no-verify` option. NOTE: You should remove all unused hooks (in <angular>/.git/hooks) before running `yarn` so that husky hooks are installed correctly.

IgorMinar

This is a good start. Thanks.

vicb · 2018-03-24T05:55:39Z

package.json

@@ -23,10 +23,12 @@
    "preinstall": "node tools/yarn/check-yarn.js",
    "postinstall": "yarn update-webdriver && node ./tools/postinstall-patches.js",
    "update-webdriver": "webdriver-manager update --gecko false $CHROMEDRIVER_VERSION_ARG",
-    "check-env": "gulp check-env"
+    "check-env": "gulp check-env",
+    "commitmsg": "./scripts/git/commit-msg.js"


would that be ok on windows ?
do we need node ./scripts/git/commit-msg.js ?
@ocombe do you know ?

I can't try it, but I am almost sure you need node 😁

please @gkalpak it would really help to have someone confirms what works on win

I am afk until Sunday evening. I can confirm then (since you don't trust my "almost-sureness" 😛).

it doesn't work without node:

git commit -m "jlkjnljn" husky > npm run -s commitmsg (node v8.9.2) '.' n’est pas reconnu en tant que commande interne ou externe, un programme exécutable ou un fichier de commandes. husky > commit-msg hook failed (add --no-verify to bypass)

if I add node it works:

git commit -m "jlkjnljn" husky > npm run -s commitmsg (node v8.9.2) INVALID COMMIT MSG: "jlkjnljn" => ERROR: The commit message does not match the format of '<type>(<scope>): <subject>' OR 'Revert: "type(<scope>): <subject>"' Check CONTRIBUTING.md at the root of the repo for more information. husky > commit-msg hook failed (add --no-verify to bypass)

the good news is that it also works with GUI apps like git kraken, which is awesome :)

gkalpak · 2018-03-24T12:13:02Z

package.json

@@ -23,10 +23,12 @@
    "preinstall": "node tools/yarn/check-yarn.js",
    "postinstall": "yarn update-webdriver && node ./tools/postinstall-patches.js",
    "update-webdriver": "webdriver-manager update --gecko false $CHROMEDRIVER_VERSION_ARG",
-    "check-env": "gulp check-env"
+    "check-env": "gulp check-env",
+    "commitmsg": "./scripts/git/commit-msg.js"


I can't try it, but I am almost sure you need node 😁

gkalpak · 2018-03-24T12:13:31Z

package.json

  },
  "dependencies": {
    "core-js": "^2.4.1",
+    "husky": "^0.14.3",


Shouldn't this be a devDependency?

yep it should I thought It was there. (rant: why doesn't yarn understand --save-dev ???)

you should upgrade to husky rc because there are some bug in this version, and the new version have a separate config for hooks instead of config in script

why doesn't yarn understand --save-dev

You have to speak in its language (--dev) if you want it to understand you 😛

That's what I ended up doing but obviously I did it wrong. But isn't it kind of stupid that --save-dev 1) is not understood 2) does not even print a warning ? Fragmentation sucks !

googlebot · 2018-03-26T08:51:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

ocombe · 2018-03-26T08:52:22Z

I've updated the PR to add husky as a dev dep, and to add node in the script command.
(And it turns out that the script works with fixups!)

ngbot · 2018-03-26T16:13:02Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "cla/google" is failing
status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

vicb · 2018-03-26T16:13:04Z

Excellent Olivier, Thanks !!

vicb · 2018-03-26T16:15:08Z

merge-assistance:

cla is ok (authors are @ocombe and myself)
travis failure is unrelated (just restarted the build to see if this helps)

angular-automatic-lock-bot · 2019-09-13T22:21:37Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

vicb added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Mar 23, 2018

googlebot added the cla: yes label Mar 23, 2018

IgorMinar approved these changes Mar 24, 2018

View reviewed changes

vicb commented Mar 24, 2018

View reviewed changes

gkalpak reviewed Mar 24, 2018

View reviewed changes

vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 24, 2018

fixup! refactor: add a commit-msg git hook to check commit messages

629133b

googlebot added cla: no and removed cla: yes labels Mar 26, 2018

ocombe added cla: yes and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: no labels Mar 26, 2018

ocombe approved these changes Mar 26, 2018

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 26, 2018

vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 26, 2018

matsko added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 26, 2018

matsko closed this in 7a406a3 Mar 26, 2018

vicb deleted the 0323-checkMsgHook branch August 3, 2018 19:07

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add a commit-msg git hook to check commit messages #22969

refactor: add a commit-msg git hook to check commit messages #22969

vicb commented Mar 23, 2018 •

edited by ocombe

Loading

IgorMinar left a comment

vicb Mar 24, 2018

gkalpak Mar 24, 2018

vicb Mar 24, 2018

gkalpak Mar 25, 2018

ocombe Mar 26, 2018 •

edited

Loading

gkalpak Mar 24, 2018

gkalpak Mar 24, 2018

vicb Mar 24, 2018

sandangel Mar 24, 2018

gkalpak Mar 25, 2018

vicb Mar 26, 2018

googlebot commented Mar 26, 2018

ocombe commented Mar 26, 2018

ngbot bot commented Mar 26, 2018

vicb commented Mar 26, 2018

vicb commented Mar 26, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

refactor: add a commit-msg git hook to check commit messages #22969

refactor: add a commit-msg git hook to check commit messages #22969

Conversation

vicb commented Mar 23, 2018 • edited by ocombe Loading

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Mar 26, 2018

ocombe commented Mar 26, 2018

ngbot bot commented Mar 26, 2018

vicb commented Mar 26, 2018

vicb commented Mar 26, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

vicb commented Mar 23, 2018 •

edited by ocombe

Loading

ocombe Mar 26, 2018 •

edited

Loading