Skip to content
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

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 23, 2018

Fixes: #24567

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

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 23, 2018
@rvagg
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
Copy link
Contributor

refack commented Nov 23, 2018

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

@refack 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
@@ -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!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to removing this TODO comment.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea! One user feedback issue.

@@ -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!"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to removing this TODO comment.


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 }' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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$

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested diff as suggested changes to fix user feedback.


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 }' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*** 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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
${MESSAGE}
${MESSAGE}

@richardlau
Copy link
Member

Ping @rvagg.

@Trott
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
Copy link
Member

Trott commented Dec 1, 2018

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one remaining nit.

tools/lint-pr-commit-message.sh Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Dec 1, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 1, 2018
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
Copy link
Member

Trott commented Dec 1, 2018

Landed in 76faccc

@Trott Trott closed this Dec 1, 2018
BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
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>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
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>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
rvagg added a commit that referenced this pull request Feb 28, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants