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

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

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 27, 2019

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

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
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants