tools: do not lint commit message if var undefined by Trott · Pull Request #23725 · nodejs/node

Trott · 2018-10-18T02:23:18Z

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)

nodejs-github-bot · 2018-10-18T02:23:20Z

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

Trott · 2018-10-18T05:00:21Z

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

lpinca · 2018-10-18T06:50:46Z

.travis.yml

        - 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


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

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

#23741 to test

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.

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

mmarchini

Nice, this should fix #23737

mmarchini · 2018-10-18T19:58:05Z

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

refack · 2018-10-18T23:32:15Z

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

Trott · 2018-10-18T23:33:44Z

Assuming this was closed accidentally?

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 · 2018-10-18T23:50:06Z

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

Trott · 2018-10-18T23:51:04Z

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 · 2018-10-18T23:54:54Z

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 · 2018-10-19T00:03:40Z

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.

Refs: nodejs#23725 (comment)

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>