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,meta: don't fail Travis for commit message #23739

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 18, 2018

/CC @Trott

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

@refack refack requested a review from Trott October 18, 2018 21:22
@refack
Copy link
Contributor Author

refack commented Oct 18, 2018

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

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

richardlau commented Oct 18, 2018

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
Trott previously requested changes Oct 18, 2018
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.

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.

@Trott Trott dismissed their stale review October 18, 2018 23:06

(Not going to block this.)

@Trott
Copy link
Member

Trott commented Oct 18, 2018

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

@refack
Copy link
Contributor Author

refack commented Oct 18, 2018

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

Trott commented Oct 18, 2018

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
Trott previously requested changes Oct 18, 2018
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.

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.

@refack refack added the blocked PRs that are blocked by other issues or PRs. label Oct 19, 2018
@Trott Trott dismissed their stale review October 31, 2018 01:41

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

Trott commented Nov 6, 2018

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.

@Trott Trott closed this Nov 6, 2018
@refack 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
Copy link
Contributor Author

refack commented Nov 6, 2018

PR run
image
non PR run
image

PR-URL: nodejs#23739
Fixes: nodejs#23737
Refs: nodejs#22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
@refack refack merged commit bec505b into nodejs:master Nov 6, 2018
@refack refack deleted the no-lint-commit-on-travis branch November 6, 2018 14:41
@Trott
Copy link
Member

Trott commented Nov 6, 2018

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

@Trott
Copy link
Member

Trott commented Nov 6, 2018

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

refack commented Nov 6, 2018

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

  1. I was not aware of a planned C&L. I only found out after the fact when it started.
  2. 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
Copy link
Member

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.

targos pushed a commit that referenced this pull request Nov 6, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
@refack 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
@refack
Copy link
Contributor Author

refack commented Nov 7, 2018

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

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

kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#23739
Fixes: nodejs#23737
Refs: nodejs#22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
PR-URL: #23739
Fixes: #23737
Refs: #22452
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
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. flaky-test Issues and PRs related to the tests with unstable failures on the CI. meta Issues and PRs related to the general management of the project. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Travis linter false-negatives for commit message checks
10 participants