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: do not lint commit message if var undefined #23725

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 18, 2018

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: nodejs#23572 (comment)
Refs: nodejs#22842 (comment)
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2018
@Trott
Copy link
Member Author

Trott commented Oct 18, 2018

Since .travis.yml is not exercised in our Jenkins tests, light CI + Travis should suffice.

@@ -13,7 +13,7 @@ matrix:
script:
- make lint
# Lint the first commit in the PR.
- git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata
- \[ -z "$TRAVIS_COMMIT_RANGE" \] || git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no need to escape [ and ].

Copy link
Member Author

Choose a reason for hiding this comment

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

[ and ] have special meaning in YAML, so I think you do need to escape at least the opening one, but I'm not sure, so let's try...

Copy link
Member Author

Choose a reason for hiding this comment

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

#23741 to test

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the result there is malformed YAML and Travis doesn't kick off. Confirmed with https://codebeautify.org/yaml-validator that escaping is needed.

Copy link
Member

@lpinca lpinca Oct 19, 2018

Choose a reason for hiding this comment

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

You are right, I was tricked by https://github.com/primus/eventemitter3/blob/991b9454dd977cbfb327203a03885c8f721095e3/.travis.yml and many other similar cases but that works because the whole string is wrapped in ''.

Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

Nice, this should fix #23737

@mmarchini
Copy link
Contributor

👍 if you believe this should be fast-tracked (reason: we're having multiple Travis linter failures and this should fix it)

@Trott Trott mentioned this pull request Oct 18, 2018
4 tasks
@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 18, 2018
@refack
Copy link
Contributor

refack commented Oct 18, 2018

This should land. I don't want to fast-track #23739
Landed in 6afaebc

@refack refack closed this Oct 18, 2018
@Trott
Copy link
Member Author

Trott commented Oct 18, 2018

Assuming this was closed accidentally?

@Trott Trott reopened this Oct 18, 2018
refack pushed a commit that referenced this pull request Oct 18, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@Trott Trott closed this Oct 18, 2018
@jasnell
Copy link
Member

jasnell commented Oct 18, 2018

This appears to have landed early with only a single fast-track approval.

image

@Trott
Copy link
Member Author

Trott commented Oct 18, 2018

This appears to have landed early with only a single fast-track approval.

@jasnell In that situation, I assume @mmarchini was in favor of fast-tracking, so that would be two approvals.

@jasnell
Copy link
Member

jasnell commented Oct 18, 2018

Per Collaborator Guide:

The pull request can be landed once two or more Collaborators approve both the pull request and the fast-tracking request, and the necessary CI testing is done.

I would say that @mmarchini's post counts as the "fast-tracking request" and not as an approval.

@Trott
Copy link
Member Author

Trott commented Oct 19, 2018

I would say that @mmarchini's post counts as the "fast-tracking request" and not as an approval.

We may be splitting hairs here, but I would say it is both a request and an approval. As the change author, I can make a request, but not an approval. A reviewer can do both, though. At least, that's my interpretation.

jasnell added a commit to jasnell/node that referenced this pull request Oct 19, 2018
@jasnell jasnell mentioned this pull request Oct 19, 2018
2 tasks
jasnell added a commit that referenced this pull request Oct 19, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
jasnell added a commit that referenced this pull request Oct 21, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit
messages in Travis CI.

Refs: #23572 (comment)
Refs: #22842 (comment)

PR-URL: #23725
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Refs: #23725 (comment)

PR-URL: #23744
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Nov 29, 2018
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
@Trott Trott deleted the travis-env-check branch January 13, 2022 22:50
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. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants