tools: don't use GH API for commit message checks #24574

rvagg · 2018-11-23T01:58:39Z

.patch URLs contain enough predictable and usable data to make this work without the API

nodejs-github-bot · 2018-11-23T01:58:41Z

@rvagg build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1681/pipeline

rvagg · 2018-11-23T02:00:45Z

script in action on this PR: https://travis-ci.com/nodejs/node/jobs/160424788#L470

refack · 2018-11-23T02:04:35Z

And we have green - https://travis-ci.com/nodejs/node/jobs/160424788

mscdex · 2018-11-23T02:38:13Z

tools/lint-pr-commit-message.sh

@@ -25,21 +25,15 @@ if [ -z "${PR_ID}" ]; then
  echo " e.g. $0 <PR_NUMBER>"
  exit 1
 fi
-# Retrieve the first commit of the pull request via GitHub API
-# TODO: If we teach core-validate-commit to ignore "fixup!" and "squash!"


Isn't this comment still relevant? The only thing that's different in it is the url in the command line example?

Ah, you're right, I didn't read all of it and assumed it was just talking about switching to npx -q core-validate-commit --no-validate-metadata which is already there. It's still not quite right because it's clear we can't continue to use the API. core-validate-commit is going to have to be fed the list of commits.

I'd like to just remove this comment entirely and leave it to future evolution of core-validate-commit to resolve rather than having a TODO. @richardlau?

No objections to removing this TODO comment.

richardlau

Nice idea! One user feedback issue.

richardlau · 2018-11-23T09:51:50Z

tools/lint-pr-commit-message.sh

@@ -25,21 +25,15 @@ if [ -z "${PR_ID}" ]; then
  echo " e.g. $0 <PR_NUMBER>"
  exit 1
 fi
-# Retrieve the first commit of the pull request via GitHub API
-# TODO: If we teach core-validate-commit to ignore "fixup!" and "squash!"


No objections to removing this TODO comment.

richardlau · 2018-11-23T09:56:15Z

tools/lint-pr-commit-message.sh

+
+PATCH=$( curl -sL https://github.com/nodejs/node/pull/${PR_ID}.patch | grep '^From \|^Subject: ' )
+if FIRST_COMMIT="$( echo "$PATCH" | awk '/^From [0-9a-f]{40} / { if (count++ == 0) print $2 }' )"; then
+  MESSAGE=$( echo "$PATCH" | awk '/^Subject: \[PATCH 1/ { gsub(/^Subject: \[PATCH[^\]]*\] /, ""); print }' )


It looks like the MESSAGE logic isn't working -- Probably because the message has been filtered out of PATCH above?

e.g.

-bash-4.2$ bash tools/lint-pr-commit-message.sh 24574 *** Linting the first commit message for pull request 24574 *** according to the guidelines at https://goo.gl/p2fr5Q. *** Commit message for 6f645d3675 is: ✔ 6f645d367591a2d3734caa28dacdd7bcdcef3528 ✔ 1:7 Valid fixes url fixes-url ✔ 0:0 blank line after title line-after-title ✔ 0:0 line-lengths are valid line-length ✔ 0:0 valid subsystems subsystem ✔ 0:0 Title is formatted correctly. title-format ✔ 0:0 Title is <= 50 columns. title-length -bash-4.2$

I would expect to see

... *** Commit message for 6f645d3675 is: tools: don't use GH API for commit message checks Fixes: https://github.com/nodejs/node/issues/24567 ...

The reason for printing the message is that it should make it obvious what is being linted (just in case, for example, the wrong SHA is used).

diff --git a/tools/lint-pr-commit-message.sh b/tools/lint-pr-commit-message.sh index 0bc873f..7ea75ab 100644 --- a/tools/lint-pr-commit-message.sh +++ b/tools/lint-pr-commit-message.sh @@ -28,12 +28,12 @@ fi PATCH=$( curl -sL https://github.com/nodejs/node/pull/${PR_ID}.patch | grep '^From \|^Subject: ' ) if FIRST_COMMIT="$( echo "$PATCH" | awk '/^From [0-9a-f]{40} / { if (count++ == 0) print $2 }' )"; then - MESSAGE=$( echo "$PATCH" | awk '/^Subject: \[PATCH 1/ { gsub(/^Subject: \[PATCH[^\]]*\] /, ""); print }' ) + MESSAGE=$( git show --quiet --format='format:%B' $FIRST_COMMIT ) echo " *** Linting the first commit message for pull request ${PR_ID} *** according to the guidelines at https://goo.gl/p2fr5Q. *** Commit message for $(echo $FIRST_COMMIT | cut -c 1-10) is: - ${MESSAGE} +${MESSAGE} " npx -q core-validate-commit --no-validate-metadata "${FIRST_COMMIT}" fi

e.g.

-bash-4.2$ bash tools/lint-pr-commit-message.sh 24574 *** Linting the first commit message for pull request 24574 *** according to the guidelines at https://goo.gl/p2fr5Q. *** Commit message for 6f645d3675 is: tools: don't use GH API for commit message checks Fixes: https://github.com/nodejs/node/issues/24567 ✔ 6f645d367591a2d3734caa28dacdd7bcdcef3528 ✔ 1:7 Valid fixes url fixes-url ✔ 0:0 blank line after title line-after-title ✔ 0:0 line-lengths are valid line-length ✔ 0:0 valid subsystems subsystem ✔ 0:0 Title is formatted correctly. title-format ✔ 0:0 Title is <= 50 columns. title-length -bash-4.2$

richardlau

Suggested diff as suggested changes to fix user feedback.

richardlau · 2018-11-23T11:13:35Z

tools/lint-pr-commit-message.sh

+
+PATCH=$( curl -sL https://github.com/nodejs/node/pull/${PR_ID}.patch | grep '^From \|^Subject: ' )
+if FIRST_COMMIT="$( echo "$PATCH" | awk '/^From [0-9a-f]{40} / { if (count++ == 0) print $2 }' )"; then
+  MESSAGE=$( echo "$PATCH" | awk '/^Subject: \[PATCH 1/ { gsub(/^Subject: \[PATCH[^\]]*\] /, ""); print }' )


Suggested change

MESSAGE=$( echo "$PATCH" | awk '/^Subject: \[PATCH 1/ { gsub(/^Subject: \[PATCH[^\]]*\] /, ""); print }' )

MESSAGE=$( git show --quiet --format='format:%B' $FIRST_COMMIT )

Just realised that if this change is made we can also drop |^Subject: from the grep for PATCH above.

richardlau · 2018-11-23T11:13:48Z

tools/lint-pr-commit-message.sh

+*** Linting the first commit message for pull request ${PR_ID}
+*** according to the guidelines at https://goo.gl/p2fr5Q.
+*** Commit message for $(echo $FIRST_COMMIT | cut -c 1-10) is:
+      ${MESSAGE}


Suggested change

${MESSAGE}

${MESSAGE}

richardlau · 2018-11-29T06:45:11Z

Ping @rvagg.

Trott · 2018-12-01T05:18:09Z

@richardlau I went ahead and applied your two nits because this is a change I desperately want to see land. Can you confirm that everything looks good to you now and, if so, clear your request for changes?

Trott · 2018-12-01T05:21:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19109/

richardlau

LGTM with one remaining nit.

tools/lint-pr-commit-message.sh

Trott · 2018-12-01T06:48:55Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1762/

Fixes: nodejs#24567 PR-URL: nodejs#24574 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Trott · 2018-12-01T07:10:45Z

Landed in 76faccc

Fixes: #24567 PR-URL: #24574 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Fixes: nodejs#24567 PR-URL: nodejs#24574 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Fixes: #24567 PR-URL: #24574 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

tools: don't use GH API for commit message checks

6f645d3

Fixes: nodejs#24567

rvagg requested a review from richardlau November 23, 2018 01:58

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 23, 2018

rvagg mentioned this pull request Nov 23, 2018

Hitting rate limits with Travis commit message linting #24567

Closed

refack approved these changes Nov 23, 2018

View reviewed changes

refack added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. meta Issues and PRs related to the general management of the project. labels Nov 23, 2018

devsnek approved these changes Nov 23, 2018

View reviewed changes

mscdex reviewed Nov 23, 2018

View reviewed changes

danbev approved these changes Nov 23, 2018

View reviewed changes

richardlau requested changes Nov 23, 2018

View reviewed changes

This was referenced Nov 23, 2018

lib: skip fixup! and squash! commits nodejs/core-validate-commit#33

Closed

lib: fix worker threads can't read stdout #24652

Closed

fixup! tools: don't use GH API for commit message checks

722a07b

gireeshpunathil approved these changes Dec 1, 2018

View reviewed changes

richardlau approved these changes Dec 1, 2018

View reviewed changes

tools/lint-pr-commit-message.sh Outdated Show resolved Hide resolved

fixup! fixup! tools: don't use GH API for commit message checks

edc59a2

Trott closed this Dec 1, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs added the land-on-v10.x label Feb 12, 2019

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: don't use GH API for commit message checks #24574

tools: don't use GH API for commit message checks #24574

rvagg commented Nov 23, 2018

nodejs-github-bot commented Nov 23, 2018

rvagg commented Nov 23, 2018

refack commented Nov 23, 2018

mscdex Nov 23, 2018

rvagg Nov 23, 2018

rvagg Nov 23, 2018

richardlau Nov 23, 2018

richardlau left a comment

richardlau Nov 23, 2018

richardlau Nov 23, 2018

richardlau Nov 23, 2018

richardlau left a comment

richardlau Nov 23, 2018

richardlau Nov 23, 2018

richardlau Nov 23, 2018

richardlau commented Nov 29, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

richardlau left a comment

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

	MESSAGE=$( echo "$PATCH" \| awk '/^Subject: \[PATCH 1/ { gsub(/^Subject: \[PATCH[^\]]*\] /, ""); print }' )
	MESSAGE=$( git show --quiet --format='format:%B' $FIRST_COMMIT )

tools: don't use GH API for commit message checks #24574

tools: don't use GH API for commit message checks #24574

Conversation

rvagg commented Nov 23, 2018

nodejs-github-bot commented Nov 23, 2018

rvagg commented Nov 23, 2018

refack commented Nov 23, 2018

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

richardlau 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

richardlau 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

richardlau commented Nov 29, 2018

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018

richardlau left a comment

Choose a reason for hiding this comment

Trott commented Dec 1, 2018

Trott commented Dec 1, 2018