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: add script to lint first PR commit message #24030

Closed
wants to merge 1 commit into from

Conversation

richardlau
Copy link
Member

Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:

  1. supplied on the command line (use this to test against any PR)
  2. TRAVIS_PULL_REQUEST if set and != "false"
  3. derived from the HEAD commit via the GitHub API

The GitHub API should be more consistent with the web UI in terms of finding
the first commit for a pull request.

This is an alternative to #23758 with the bonus that
you can run the script against any pull request number.

Checklist

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 2, 2018
@richardlau
Copy link
Member Author

Examples:

Run locally, no pull request specified (script works out based on current HEAD commit):

-bash-4.2$ bash tools/lint-pr-commit-message.sh
Linting the first commit message for pull request 24030
according to the guidelines at https://goo.gl/p2fr5Q.
Commit message for 826115ee852b8aef3dd6e9578c8b0190605c0e98 is:
tools: add script to lint first PR commit message

Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) TRAVIS_PULL_REQUEST if set and != "false"
 3) derived from the HEAD commit via the GitHub API
  ✔  826115ee852b8aef3dd6e9578c8b0190605c0e98
     ✔  0:0      skipping 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$

Checking a completely different PR:

-bash-4.2$ bash tools/lint-pr-commit-message.sh 23758
Linting the first commit message for pull request 23758
according to the guidelines at https://goo.gl/p2fr5Q.
Commit message for fa99a11b207932c6d02ccfc9e8308984ed0f1f76 is:
tools: add script to lint commit messages

This script attempts to guess the target upstream branch.
To manually specify, e.g. to target `canary-base`:
  TARGET_BRANCH=canary-base bash lint-commit-message.sh
  ✔  fa99a11b207932c6d02ccfc9e8308984ed0f1f76
     ✔  0:0      skipping 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$

On Travis (for this PR), https://travis-ci.com/nodejs/node/jobs/155889224:
image

@refack refack added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Nov 2, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API
@richardlau
Copy link
Member Author

Moved the Travis specific stuff out of the script back into .travis.yml.

@richardlau
Copy link
Member Author

Longer term we could teach core-validate-commit:

  1. to take a PR argument and lint all the commits for that PR (as determined by GitHub's API)
  2. to ignore "fixup!" and "squash!" commit messages

and then replace the shell script here with a direct call to npx -q core-validate-commit ....

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2018
@Trott
Copy link
Member

Trott commented Nov 4, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 4, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API

PR-URL: nodejs#24030
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 4, 2018

Landed in 7489ee8

@Trott Trott closed this Nov 4, 2018
@addaleax
Copy link
Member

addaleax commented Nov 4, 2018

@Trott @richardlau This seems to have broken Travis CI for non-PR branches without TRAVIS_PULL_REQUEST?

addaleax added a commit to addaleax/node that referenced this pull request Nov 4, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: nodejs#24030
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 4, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: nodejs#24030

PR-URL: nodejs#24076
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@richardlau richardlau deleted the pr-commit-lint branch November 5, 2018 00:43
targos pushed a commit that referenced this pull request Nov 5, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API

PR-URL: #24030
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Nov 5, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: #24030

PR-URL: #24076
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API

PR-URL: #24030
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: #24030

PR-URL: #24076
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API

PR-URL: #24030
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: #24030

PR-URL: #24076
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
Decouple first commit in pull request linting from Travis by using
the GitHub API to work out the first commit.

The shell script obtains the pull request number in one of the
following ways:
 1) supplied on the command line (use this to test against any PR)
 2) derived from the HEAD commit via the GitHub API

PR-URL: #24030
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
Do not run any linting at all when `TRAVIS_PULL_REQUEST` is `false`.
This would otherwise break Travis CI for `master` and release branches.

Refs: #24030

PR-URL: #24076
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants