build: lint commit message in separate Travis job #24254

richardlau · 2018-11-08T18:46:59Z

#23739 changed the Travis Linter job so that it never fails if the first
commit message linting step fails. This was done in response to a
false positive which we speculate was perhaps due to hitting the
GitHub API rate limit or a network issue (#23739 (comment)).

This PR:

Moves the first commit message linting step to a separate Travis job. This will keep the amount of output in the individual jobs down.
Runs the script in bash debug mode to capture any issues communicating with the
GitHub API (e.g. network issues, rate limits).

Checklist

commit message follows commit guidelines

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits).

nodejs-github-bot · 2018-11-08T18:47:01Z

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

Trott

Thank you for this!

richardlau · 2018-11-08T19:07:00Z

Just noticed that the following message from the script:

Linting the first commit message for pull request 24254
according to the guidelines at https://goo.gl/p2fr5Q.

is now lost amongst all the debug output.

Pushed a fixup! commit to add the guidelines URL to the
job name.

refack · 2018-11-08T19:32:03Z

Checks tab gets confusing (the link is to https://travis-ci.com/nodejs/node/jobs/157336968 not https://goo.gl/p2fr5Q)

richardlau · 2018-11-08T19:37:42Z

Checks tab gets confusing (the link is to https://travis-ci.com/nodejs/node/jobs/157336968 not https://goo.gl/p2fr5Q)

I'm open to any suggestions to improve the user experience here.

refack · 2018-11-08T19:37:48Z

.travis.yml

+      node_js: "node"
+      script:
+        - if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then
+            bash -x tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST};


if you split this into install & script the install part will fold.

install: - RET=0 if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash -x tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST} > out.txt RET=$? fi export RET script: - cat out.txt || true exit $RET

Redirecting stdout to file will lose coloring (okay not a major issue 😛). But if we do want to allow folding perhaps the thing to do is split the shell script so that the first part works out the first commit (this can be the install (before_script?) step) and then script would directly run npx core-validate-commit ....

As in install == fetch from GitHub...
It will be a nicer UX, but will make the script more complex.
Your call.

refack · 2018-11-08T20:39:14Z

.travis.yml

@@ -5,6 +5,7 @@ os: linux
 matrix:
  include:
    - name: "First commit message adheres to guidelines at https://goo.gl/p2fr5Q"
+      if: type = pull_request


Refs:
https://docs.travis-ci.com/user/conditional-builds-stages-jobs/
https://docs.travis-ci.com/user/conditions-v1

refack · 2018-11-08T20:43:55Z

fixup! try html in job name

Seems to work:

@jrkong did a great job fixing travis-ci/travis-ci#10028 🏆

richardlau · 2018-11-08T20:45:45Z

Apparently html is allowed in the job name, so I can make the https://goo.gl/p2fr5Q bit link directly to the guidelines:

Link isn't clicky in the Travis UI:

richardlau · 2018-11-08T21:07:59Z

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

Trott · 2018-11-11T07:01:00Z

Landed in 19e5e78

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits). PR-URL: nodejs#24254 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits). PR-URL: #24254 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits). PR-URL: nodejs#24254 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

refack · 2018-11-17T16:46:28Z

FTR: true positive https://travis-ci.com/nodejs/node/jobs/159274549

richardlau · 2018-11-21T06:21:16Z

false positive which we speculate was perhaps due to hitting the
GitHub API rate limit or a network issue (#23739 (comment)).

FTR: Here's one example where the rate limit is hit:

https://travis-ci.com/nodejs/node/jobs/158565896#L447

$ if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash -x tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST}; fi
+GH_API_URL=https://api.github.com
+PR_ID=24366
+'[' -z 24366 ']'
+'[' -z 24366 ']'
++curl -s https://api.github.com/repos/nodejs/node/pulls/24366/commits
+PR_COMMITS='{
  "message": "API rate limit exceeded for 52.54.40.118. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'
++node -p 'JSON.parse(process.argv[1])[0].url' '{
  "message": "API rate limit exceeded for 52.54.40.118. (But here'\''s the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
  "documentation_url": "https://developer.github.com/v3/#rate-limiting"
}'
+FIRST_COMMIT=
+echo 'Unable to determine the first commit for pull request 24366.'
Unable to determine the first commit for pull request 24366.
+exit 1
The command "if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash -x tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST}; fi" exited with 1.

I suggest we keep an eye out for if this becomes a more common occurrence. Note that the rate limit for unauthenticated GitHub API requests is IP based so it's whatever Travis is running on that IP (so may not be entirely our jobs).

Authenticated GitHub API requests on Travis may be tricky to implement without exposing the token publicly. Encrypted environment variables are not available to pull requests from forks.

refack · 2018-11-21T22:50:39Z

Seen in #24498 (comment)

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits). PR-URL: #24254 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>

build: lint commit message in separate Travis job

1a79e37

Move the first commit message linting to a separate Travis job. Run the script in bash debug mode to capture any issues communicating with the GitHub API (e.g. network issues, rate limits).

richardlau mentioned this pull request Nov 8, 2018

build,meta: don't fail Travis for commit message #23739

Merged

3 tasks

Trott approved these changes Nov 8, 2018

View reviewed changes

fixup! add link to guidelines in job name

295c5cf

richardlau added the build Issues and PRs related to build files or the CI. label Nov 8, 2018

refack approved these changes Nov 8, 2018

View reviewed changes

refack added the meta Issues and PRs related to the general management of the project. label Nov 8, 2018

refack reviewed Nov 8, 2018

View reviewed changes

richardlau added 2 commits November 8, 2018 15:23

fixup! add condition to commit message job

5a01d5e

fixup! try html in job name

2857dbb

refack reviewed Nov 8, 2018

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 8, 2018

mmarchini approved these changes Nov 9, 2018

View reviewed changes

Trott closed this Nov 11, 2018

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 15, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#232

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-android-nodejs#249

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#233

Closed

This was referenced Nov 16, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#234

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#235

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#236

Closed

refack added landed and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 17, 2018

This was referenced Nov 22, 2018

Hitting rate limits with Travis commit message linting #24567

Closed

win, build: skip building cctest by default #21408

Closed

codebytere mentioned this pull request Jan 4, 2019

v10.15.1 proposal #25346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: lint commit message in separate Travis job #24254

build: lint commit message in separate Travis job #24254

richardlau commented Nov 8, 2018

nodejs-github-bot commented Nov 8, 2018

Trott left a comment

richardlau commented Nov 8, 2018

refack commented Nov 8, 2018

richardlau commented Nov 8, 2018

refack Nov 8, 2018 •

edited

Loading

richardlau Nov 8, 2018

refack Nov 8, 2018

refack Nov 8, 2018

richardlau Nov 8, 2018

refack commented Nov 8, 2018

richardlau commented Nov 8, 2018

richardlau commented Nov 8, 2018

Trott commented Nov 11, 2018 •

edited by refack

Loading

refack commented Nov 17, 2018

richardlau commented Nov 21, 2018

refack commented Nov 21, 2018

build: lint commit message in separate Travis job #24254

build: lint commit message in separate Travis job #24254

Conversation

richardlau commented Nov 8, 2018

Checklist

nodejs-github-bot commented Nov 8, 2018

Trott left a comment

Choose a reason for hiding this comment

richardlau commented Nov 8, 2018

refack commented Nov 8, 2018

richardlau commented Nov 8, 2018

refack Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

richardlau Nov 8, 2018

Choose a reason for hiding this comment

refack Nov 8, 2018

Choose a reason for hiding this comment

refack Nov 8, 2018

Choose a reason for hiding this comment

richardlau Nov 8, 2018

Choose a reason for hiding this comment

refack commented Nov 8, 2018

richardlau commented Nov 8, 2018

richardlau commented Nov 8, 2018

Trott commented Nov 11, 2018 • edited by refack Loading

refack commented Nov 17, 2018

richardlau commented Nov 21, 2018

refack commented Nov 21, 2018

refack Nov 8, 2018 •

edited

Loading

Trott commented Nov 11, 2018 •

edited by refack

Loading