build,meta: don't fail Travis for commit message #23739
Conversation
|
/CC @nodejs/testing @nodejs/build-files |
|
I'm not against attempting to lint the commit message, but not in its current form.
|
|
(Optimistic iteration to write a message to hopefully make it clear that the commit message linting is about the commit message: #23742) |
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 |
There was a problem hiding this comment.
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.
165f77a to
75dab04
Compare
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
|
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. |
|
PR run |
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>
c69a937 to
bec505b
Compare
|
Turning this off before/during Code & Learn seems like unfortunate timing. Oh well... |
|
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)? |
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)
I agree that this is a lacuna in our guidelines.
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 |
FWIW this works for me locally: 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 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 <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>
That was my intuition, that or some other network issue.
I'm +1 on |
|
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>
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>
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>
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>
/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), orvcbuild test(Windows) passes