★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24574Nouvelle recherche | Portail wanayoo
Skip to content

/node

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

Reviewers

@mscdex mscdex

@refack refack

@danbev danbev

@richardlau richardlau

@devsnek devsnek

@gireeshpunathil gireeshpunathil

Assignees
No one assigned
Labels
build land-on-v10.x meta test tools
Projects
None yet
Milestone
No milestone
10 participants
@rvagg @nodejs-github-bot @refack @richardlau @Trott @mscdex @danbev @devsnek @gireeshpunathil @BethGriggs
@rvagg
Copy link
Member

rvagg commented Nov 23, 2018

Fixes: #24567

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

@rvagg rvagg requested a review from richardlau Nov 23, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the tools label Nov 23, 2018

@rvagg rvagg referenced this pull request Nov 23, 2018

Closed

Hitting rate limits with Travis commit message linting #24567

@rvagg

This comment has been minimized.

Sign in to view
Copy link
Member Author

rvagg commented Nov 23, 2018

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

refack approved these changes Nov 23, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Nov 23, 2018

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

@refack refack added build test meta labels Nov 23, 2018

@devsnek

devsnek approved these changes Nov 23, 2018
View changes

@mscdex

mscdex reviewed Nov 23, 2018
View changes

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!"

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Nov 23, 2018

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@rvagg

rvagg Nov 23, 2018

Author Member

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.

This comment has been minimized.

Sign in to view
Copy link
@rvagg

rvagg Nov 23, 2018

Author Member

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?

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member

No objections to removing this TODO comment.
@danbev

danbev approved these changes Nov 23, 2018
View changes

@richardlau

richardlau requested changes Nov 23, 2018
View changes

Copy link
Member

richardlau left a comment

Nice idea! One user feedback issue.
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!"

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member

No objections to removing this TODO comment.
tools/lint-pr-commit-message.sh Outdated

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 }' )

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member

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).

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member 
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

richardlau requested changes Nov 23, 2018
View changes

Copy link
Member

richardlau left a comment

Suggested diff as suggested changes to fix user feedback.
tools/lint-pr-commit-message.sh Outdated

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 }' )

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

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

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member

Just realised that if this change is made we can also drop |^Subject: from the grep for PATCH above.
tools/lint-pr-commit-message.sh Outdated
*** 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}

This comment has been minimized.

Sign in to view
Copy link
@richardlau

richardlau Nov 23, 2018

Member
Suggested change
${MESSAGE}
${MESSAGE}

This was referenced Nov 23, 2018

Closed
Closed
@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

richardlau commented Nov 29, 2018

Ping @rvagg.
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Dec 1, 2018

@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

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Dec 1, 2018

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

gireeshpunathil approved these changes Dec 1, 2018
View changes

@richardlau

richardlau approved these changes Dec 1, 2018
View changes

Copy link
Member

richardlau left a comment

LGTM with one remaining nit.
Show resolved Hide resolved tools/lint-pr-commit-message.sh Outdated
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Dec 1, 2018

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 1, 2018

@rvagg @Trott
tools: don't use GH API for commit message checks 
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>
76faccc
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Dec 1, 2018

Landed in 76faccc

@Trott Trott closed this Dec 1, 2018

BridgeAR added a commit that referenced this pull request Dec 5, 2018

@rvagg @BridgeAR
tools: don't use GH API for commit message checks 
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>
Verified
This commit was signed with a verified signature.
@BridgeAR BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
9e5a79a

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

This was referenced Dec 7, 2018

Closed
Closed
Closed
Closed
Closed

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

@rvagg @refack
tools: don't use GH API for commit message checks 
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>
40ec412

BethGriggs added a commit that referenced this pull request Feb 12, 2019

@rvagg @BethGriggs
tools: don't use GH API for commit message checks 
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>
Verified
This commit was signed with a verified signature.
@BethGriggs BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
691e1a6

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

@BethGriggs BethGriggs referenced this pull request Feb 12, 2019

Merged

v10.15.3 proposal #26063

rvagg added a commit that referenced this pull request Feb 28, 2019

@rvagg
tools: don't use GH API for commit message checks 
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>
d6cd146
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.