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

build: lint commit message in separate Travis job #24254

Closed
wants to merge 4 commits into from

Conversation

richardlau
Copy link
Member

#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

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

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Thank you for this!

@richardlau
Copy link
Member Author

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.

@richardlau richardlau added the build Issues and PRs related to build files or the CI. label Nov 8, 2018
@refack
Copy link
Contributor

refack commented Nov 8, 2018

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

image

@refack refack added the meta Issues and PRs related to the general management of the project. label Nov 8, 2018
@richardlau
Copy link
Member Author

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

image

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

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

@refack refack Nov 8, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

😮

Copy link
Member Author

Choose a reason for hiding this comment

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

@refack
Copy link
Contributor

refack commented Nov 8, 2018

fixup! try html in job name

Seems to work:

image

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

@richardlau
Copy link
Member Author

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

Link isn't clicky in the Travis UI:
image

@richardlau
Copy link
Member Author

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

Trott commented Nov 11, 2018

Landed in 19e5e78

@Trott Trott closed this Nov 11, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 11, 2018
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@refack
Copy link
Contributor

refack commented Nov 17, 2018

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

@refack 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
@richardlau
Copy link
Member Author

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
Copy link
Contributor

refack commented Nov 21, 2018

Seen in #24498 (comment)

codebytere pushed a commit that referenced this pull request Dec 14, 2018
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@codebytere codebytere mentioned this pull request Jan 4, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants