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.

Sign up for GitHub

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

Jump to bottom

tools: allow the travis commit message job to fail #31116

Closed
wants to merge 1 commit into from
Closed

tools: allow the travis commit message job to fail #31116

wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 27, 2019 •
edited
Loading

Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.

image

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR
tools: allow the travis commit message job to fail
68accef 
Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 27, 2019
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2019
@sam-github
Copy link
Contributor

Isn't failing desired, it reports that the commit messages failed to adhere to the guidelines in CONTRIBUTING.md, and wouldn't contributors want to know that so they can fix it?

Someone landing can rewrite the original commit messages, but I alwasy feel a bit uncomfortable about that when I do it. It can be considered helpful, but its also attributing my words to another person, possible spelling and grammatical errors and all, and means that the message that lands never gets reviewed by collaborators, because I invent it on the spot.

I don't think tests that run but don't report a failure when they fail are very useful, since people don't tend to look at test results when the result is pass. We added the commit message test to notify people that there is a problem, might as well just remove the test if its not going to cause a red X in the github UI.

And to be clear: this is not a blocking comment, just a question.

@lpinca
Copy link
Member

lpinca commented Dec 27, 2019

I've approved but @sam-github has a valid point.

@BridgeAR
Copy link
Member Author

The test as it is right now does not seem user friendly. I would still like to notify new contributors about faulty commit messages for the points mentioned above but doing it personally seems better than the travis job. Having a failing travis due to the commit message is confusing quite a few persons and I personally tend to ignore the travis result as "unreliable".
I also guess/hope that at least a few persons might check the travis results to see what was actually checked and passed.

I could check if it's possible to get different notifications back from travis.

BridgeAR added a commit that referenced this pull request Dec 31, 2019
@BridgeAR
tools: allow the travis commit message job to fail
3abe3f2 
Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.

PR-URL: #31116
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 3abe3f2 🎉

@BridgeAR BridgeAR closed this Dec 31, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
@BridgeAR
tools: allow the travis commit message job to fail
85d152f 
Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.

PR-URL: #31116
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@BridgeAR @targos
tools: allow the travis commit message job to fail
bedc6e2 
Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.

PR-URL: #31116
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
@BridgeAR BridgeAR deleted the 2019-12-27-travis-allow-commit-message-failures branch January 20, 2020 12:08
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@BridgeAR @BethGriggs
tools: allow the travis commit message job to fail
1974fd9 
Travis often fails due to a commit message that does not adhere to
the commit guidelines. We are able to fix the commit message while
landing and it often confuses new contributors that travis fails.
Keeping the check in place but allowing a test failure to report
success should be a good middle ground.

PR-URL: #31116
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@richardlau richardlau mentioned this pull request Mar 18, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@trivikr trivikr trivikr approved these changes

@devnexen devnexen devnexen approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@BridgeAR @sam-github @lpinca @Trott @addaleax @devnexen @trivikr @ZYSzys @nodejs-github-bot