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

refack · 2018-10-18T21:22:12Z

I think in its current form the false negatives have outweighed the true negatives. All in all I don't think commit message formatting has been a limiting factor for landing velocity.

Fixes: #23737
Refs: #22452

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-10-18T21:22:14Z

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

refack · 2018-10-18T21:24:19Z

/CC @nodejs/testing @nodejs/build-files
(Since this changes only .travis.yaml the Travis run & Lite CI should give sufficient regression testing coverage)

richardlau · 2018-10-18T22:24:16Z

I'm not against attempting to lint the commit message, but not in its current form.

It's currently not obvious what it is actually linting, or what you (as the PR author) need to do to fix it.
It's currently only run on Travis and therefore is not something the PR author would see running the lint makefile targets locally.
False positives mainly due to not being able to accurately determine what is the first commit.

Trott

I think we should land #23725 to fix the false-positives and iterate on improving the message so it's clear that it's about the commit message. But I won't block this.

(Not going to block this.)

Trott · 2018-10-18T23:10:55Z

(Optimistic iteration to write a message to hopefully make it clear that the commit message linting is about the commit message: #23742)

refack · 2018-10-18T23:22:16Z

I think we should land #23725 to fix the false-positives and iterate on improving the message so it's clear that it's about the commit message. But I won't block this.

We could convert this to a beta test (make the check non failing until we are confident it has low false negatives?)

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

We could convert this to a beta test (make the check non failing until we are confident it has low false negatives?)

I was hoping to convert it into a warning, but alas, Travis only allows success or failure, no in-between state.

If we let it always be success, I don't think we'll ever see it checked enough to be confident that there aren't false-positives.

That said, if the -z check on the environment variable doesn't solve the false-positives issue, I'd be inclined to give up. 😄

Trott

I'm going to block this one for now on the grounds that we should see if the recent changes have fixed the false positives and the confusion around what the messages mean. If there's still problems in 48 hours, feel free to dismiss my objection and land.

Unblocking. I don't want this to land, but if it's still causing problems, this can land as long as there's a clear explanation of the problem. I'll try to resolve it and enable again. If there aren't any significant problems anymore, please close this PR. :-D

Trott · 2018-11-06T05:28:10Z

Seems like the false positives problem has been fixed and this can be closed. Of course, re-open if I'm wrong/there's disagreement about that.

refack · 2018-11-06T14:37:27Z

PR run

non PR run

PR-URL: nodejs#23739 Fixes: nodejs#23737 Refs: nodejs#22452 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]>

Trott · 2018-11-06T16:04:38Z

Turning this off before/during Code & Learn seems like unfortunate timing. Oh well...

Trott · 2018-11-06T16:09:49Z

Re-opening a closed issue and landing immediately without further review is also...not something that sits entirely well with me. (Not a rule violation, I don't think, but kinda funky, no?) This had last been reviewed almost three weeks ago. Problems had gone to near zero in that time. Or at least that seemed to be the case. Maybe I'm ignorant of significant problems? Or are we landing this based on a single (or very small number of) failure(s)/edge case(s)?

refack · 2018-11-06T16:23:24Z

Re-opening a closed issue and landing immediately without further review is also...not something that sits entirely well with me.

I was indeed ambigues about this, but to counter that, this was closed only 11h prior, after only 7 days of inactivity on this PR, and recent activity on the underlinying issue #24093 (48h ago)

This had last been reviewed almost three weeks ago.

I agree that this is a lacuna in our guidelines.

Turning this off before/during Code & Learn seems like unfortunate timing. Oh well...

I was not aware of a planned C&L. I only found out after the fact when it started.
IMHO it is actually an opportune moment, with the potential to reduce friction.

Or are we landing this based on a single (or very small number of) failure(s)/edge case(s)?

The motivation for landing was receiving a question from a first-time contributor that got a false positive, and consideration of the C&L

tl;dr revering is easy

richardlau · 2018-11-06T18:08:54Z

So it seems like there still friction, so imana land this as a temporary measure:

$ if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST}; fi
Unable to determine the first commit for pull request 24081.

The command "if [ "${TRAVIS_PULL_REQUEST}" != "false" ]; then bash tools/lint-pr-commit-message.sh ${TRAVIS_PULL_REQUEST}; fi" exited with 1.

store build cache

Done. Your build exited with 1.

FWIW this works for me locally:

-bash-4.2$ bash tools/lint-pr-commit-message.sh 24081
Linting the first commit message for pull request 24081
according to the guidelines at https://goo.gl/p2fr5Q.
Commit message for ab420ae2d7817008512a74744e0801022906f5b5 is:
crypto: add support for chacha20-poly1305 for AEAD

Openssl support AEAD_CHACHA20_POLY1305(rfc7539) since 1.1.

Fixes: https://github.com/nodejs/node/issues/24080
Refs: https://tools.ietf.org/html/rfc7539
  ✔  ab420ae2d7817008512a74744e0801022906f5b5
     ✔  3:7      Valid 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$

How close to the Code & Learn did the failure occur? Is it possible we hit the GitHub API rate limit? If we want to be able to debug this we can run bash -ex in the Travis job (which would echo the json returned from the API) but it would be very verbose -- maybe break commit message linting into a third Travis job, separate from source/docs linting?

Also although this PR is still titled "don't try to lint commit message" the actual commit that landed is "don't fail Travis for commit message" and still attempts to run the lint but ignores all failures.

PR-URL: #23739 Fixes: #23737 Refs: #22452 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]>

refack · 2018-11-07T00:09:15Z

Is it possible we hit the GitHub API rate limit?

That was my intuition, that or some other network issue.

If we want to be able to debug this we can run bash -ex in the Travis job (which would echo the json returned from the API) but it would be very verbose

I'm +1 on -ex for this line.

richardlau · 2018-11-08T18:49:04Z

If we want to be able to debug this we can run bash -ex in the Travis job (which would echo the json returned from the API) but it would be very verbose

I'm +1 on -ex for this line.

#24254

PR-URL: nodejs#23739 Fixes: nodejs#23737 Refs: nodejs#22452 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]>

PR-URL: #23739 Fixes: #23737 Refs: #22452 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]>

refack requested a review from Trott October 18, 2018 21:22

refack added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. meta Issues and PRs related to the general management of the project. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Oct 18, 2018

refack mentioned this pull request Oct 18, 2018

Travis linter false-negatives for commit message checks #23737

Closed

addaleax approved these changes Oct 18, 2018

View reviewed changes

jasnell approved these changes Oct 18, 2018

View reviewed changes

cjihrig approved these changes Oct 18, 2018

View reviewed changes

richardlau approved these changes Oct 18, 2018

View reviewed changes

hiroppy approved these changes Oct 18, 2018

View reviewed changes

Trott previously requested changes Oct 18, 2018

View reviewed changes

vdeturckheim approved these changes Oct 18, 2018

View reviewed changes

vdeturckheim mentioned this pull request Oct 18, 2018

doc: clarify use of timeout option in vm #23512

Closed

4 tasks

refack mentioned this pull request Oct 18, 2018

tools: do not lint commit message if var undefined #23725

Closed

2 tasks

Trott previously requested changes Oct 18, 2018

View reviewed changes

refack added the blocked PRs that are blocked by other issues or PRs. label Oct 19, 2018

refack force-pushed the no-lint-commit-on-travis branch from 165f77a to 75dab04 Compare October 30, 2018 21:10

Trott closed this Nov 6, 2018

refack added stalled Issues and PRs that are stalled. and removed blocked PRs that are blocked by other issues or PRs. labels Nov 6, 2018

refack force-pushed the no-lint-commit-on-travis branch from c69a937 to bec505b Compare November 6, 2018 14:39

refack mentioned this pull request Nov 6, 2018

crypto: add support for chacha20-poly1305 for AEAD #24081

Merged

4 tasks

refack merged commit bec505b into nodejs:master Nov 6, 2018

refack deleted the no-lint-commit-on-travis branch November 6, 2018 14:41

refack changed the title ~~build,meta: don't try to lint commit message~~ build,meta: don't fail Travis for commit message Nov 7, 2018

richardlau mentioned this pull request Nov 8, 2018

build: lint commit message in separate Travis job #24254

Closed

1 task

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

richardlau mentioned this pull request Nov 22, 2018

Hitting rate limits with Travis commit message linting #24567

Closed

codebytere added the land-on-v10.x label Nov 29, 2018

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

refack commented Oct 18, 2018 •

edited

Loading

nodejs-github-bot commented Oct 18, 2018

refack commented Oct 18, 2018 •

edited

Loading

richardlau commented Oct 18, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

Trott commented Oct 18, 2018

refack commented Oct 18, 2018

Trott commented Oct 18, 2018

Trott left a comment

Trott commented Nov 6, 2018

refack commented Nov 6, 2018

Trott commented Nov 6, 2018

Trott commented Nov 6, 2018 •

edited

Loading

refack commented Nov 6, 2018

richardlau commented Nov 6, 2018

refack commented Nov 7, 2018

richardlau commented Nov 8, 2018

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

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

Conversation

refack commented Oct 18, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 18, 2018

refack commented Oct 18, 2018 • edited Loading

richardlau commented Oct 18, 2018 • edited Loading

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Oct 18, 2018

refack commented Oct 18, 2018

Trott commented Oct 18, 2018

Trott left a comment

Choose a reason for hiding this comment

Trott commented Nov 6, 2018

refack commented Nov 6, 2018

Trott commented Nov 6, 2018

Trott commented Nov 6, 2018 • edited Loading

refack commented Nov 6, 2018

richardlau commented Nov 6, 2018

refack commented Nov 7, 2018

richardlau commented Nov 8, 2018

refack commented Oct 18, 2018 •

edited

Loading

refack commented Oct 18, 2018 •

edited

Loading

richardlau commented Oct 18, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

Trott commented Nov 6, 2018 •

edited

Loading