build: fail Travis if commit message check fails #32334
Conversation
"Allowed to fail" tests are easy to miss. With the failure on Travis, new contributors can learn our commit guidelines. Collaborators are still allowed (per our current policy) to land PRs where this check fails, as long as the warnings are fixed before pushing to master. Signed-off-by: Matheus Marchini <[email protected]>
nodejs-github-bot
added
the
build
This comment has been minimized.
This comment has been minimized.
|
One of the motivations behind this PR is preparing for nodejs/build#2201. If the commit message don't follow the guidelines, we can't use a commit queue to land it. And if a collaborator needs to manually update the commit message, it's easier to land with Also, we have a checklist item explicitly stating that the commit follow our guidelines. If folks are checking it without reading the guidelines, we could remove that item to keep the summary leaner. Or, if the guidelines are not clear, we should update them to be. One of the assumptions from #31116 is that (at least some) folks will check the result from Travis. Not sure about others, but if CI shows green, I don't open it to double check. IMO if the check is allowed to fail, we can remove it (especially since #32336 uses Actions). |
|
On the other hand, I'm starting to question our strict validation of commit messages. While some basic linting makes sense (72 characters), I'm starting to feel we focus more on the format than the content of commit messages. I can't remember the last time I asked someone to add more information to a commit message, even when the commit message is not super descriptive. I don't think I know any other projects with commit message rules as strict as ours. At most they enforce Maybe it's time to revisit our commit guidelines. |
|
@mmarchini our commit title format is very strict because of how the changelog is generated using it. |
|
Right, but as a changelog reader, the tagging on several commit messages don't add extra value. Some examples from recent commits:
I'm not saying the format is bad, I personally like it. But it seems hard for new contributors to get it right (and recently I landed PRs from collaborators where I had to fix the title, so even for us it's not entirely clear), adds extra burden to those landing pull requests, and can get in the way of an automated landing process. On a somewhat related topic: would it be possible to automatically identify which subsystems changed based on which files were touched? If so, our testing could output a suggested commit message for the user. |
|
People who only write code don't need to care about commit messages, and often don't, but speaking as someone who often is trying to figure why things are broken, consistently structured commit messages help me a lot. Also, when the commit message description isn't well formatted, then its just pushing the work of writing up the commit description to the releasers when they prepare the changelogs, and that isn't fair. Or effective. Its the author of the commit who is best positioned to describe their change, and to do so in a consistent way. In your example from above, I regularly scan commit logs visually, or with grep, and having consistent prefixes helps me a lot. This has a prefix:
This has a suffix, harder to scan:
And it lacks a capital letter, the releasers would have to fix the punctuation when building the changelog. Its true that authors seem to just tick the checkboxes off without reading, I'll avoid singling people out by linking to examples, but I think the best we can do here is to start failing PRs that don't follow the (well-advertised!) conventions. We might also want to provide a pre-canned commit message comment pointing to conventions, if that's possible across the project. Though the commit message fail text already links to the docs on the convention, maybe they just need a bit of
We could add "check the automatically applied labels" of the PR, but the currently doced advice is the best, and its not Node.js specific! I do one-off commits to a fair number of projects across the ecosystem, and I always run |
This might vary from person to person, but consistent structure hasn't helped me when things are broken, meaningful messages did. And as I mentioned above, I think we are failing at enforcing meaningful messages.
I agree we should avoid pushing the work of writing descriptions to releasers. I believe this is a good enough reason to preserve the current format.
This would be interesting, except when folks commit with
We already have some git rebase advice (pull-requests.md#step-5-rebase), but it could be improved.
I don't think this is clear enough for non-collaborators though. I'm gonna try something with GitHub Actions, maybe we can get something similar to JS linter with annotations on the commit message, so folks have actionable things to fix on it? |
Perhaps you can think of some improvements to the text. Willingness to use git is a pre-requisite to contributing to projects on github, IMO. Anyhow, there are clearly variant opinions on whether commit messages should have conventions, just as there are projects that are perfectly happy to not have conventions on whitespace, or any other coding conventions. That's fair enough, but I'm personally OK with Node.js's stance on this. I'd be stoked if someone updated |
Kind of. The Node.js GitHub bot does apply labels based on changed files (https://github.com/nodejs/github-bot/blob/master/lib/node-labels.js) although there isn't a 1:1 mapping between subsystems used in the commit messages and labels used in the web interface. Despite its name |
|
I'll close this one since #32417 will probably have better results. |
"Allowed to fail" tests are easy to miss. With the failure on Travis,
new contributors can learn our commit guidelines. Collaborators are
still allowed (per our current policy) to land PRs where this check
fails, as long as the warnings are fixed before pushing to master.
Signed-off-by: Matheus Marchini [email protected]
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes