eslintbot's commit message checks are sometimes misleading

[not-an-aardvark]

(Continuing from #8598)

When a user opens or modifies a PR, eslintbot always checks the commit message of the first commit for proper formatting. If that commit message is properly formatted, it leaves a "LGTM" message, otherwise it instructs the user to edit the commit message.

We use GitHub's "squash and merge" feature, so the only commit message that ends up on master is the message that appears in this box. (It's possible for team members to edit this when merging, but I'm assuming we generally want to avoid that because it has a higher risk of error if we forget to edit the message.) So eslintbot should try to ensure that the default message in that box is a valid commit message according to the guidelines.

From testing in another repo, I've concluded that the default message is the first commit message of the PR if the PR only has one commit. If the PR has multiple commits, the default message is the title of the PR. (In either case, the PR number is appended to the end.)

Since eslintbot always checks the first commit message of the PR, its messages can be misleading if a PR has multiple commits. If a user creates a PR and edits the commit message afterwards, then adds additional commits, eslintbot will say "LGTM" even though the title of the PR might not follow the commit message guidelines. If a PR has multiple commits and a user edits the title, then eslintbot will still report problems with the first commit message, even though the correct message will get used when the PR is merged.

For example, 936bc17 was added to master with an invalid commit message (too long) as a result of this problem.

I'm not sure about the best solution to this. We could update the eslintbot message to explain this to users, but it's kind of hard to explain clearly and might end up confusing users.

[soda0289]

Could we have the eslint bot enforce the PR title matches the first commit title.

[not-an-aardvark]

Would it do that by leaving another error message for the user? I'm concerned that flooding contributors with error messages will be discouraging.

I suppose another option would be for eslintbot to automatically edit the PR title to match the commit message, but I'm not sure eslintbot currently has the capability to do that, so we would need to create our own tool to listen for that.

[kaicataldo]

I often just rewrite the commit message myself to avoid having to ping the author again and hold the PR up, but it really is an important part of our process - given that we generate our docs and derive the release version from these messages - and I think it's worth treating with the same importance we do the code itself.

I think having the bot enforce this would be nice, but I think we should also start treating these as approval blockers during the PR review process. So, if a PR didn't have the correct title (for multiple commits) or correct commit message (for single commits), reviewers would not give approval and instead would request changes, even if the code itself looks good. We used to do this before we started squashing commits (out of necessity).

This would move the editing the of the final squashed message to the review phase rather than at the time of merging, and would put the onus on the PR author to actually follow the process we have set in place. It also ensures that more eyes are on the commit message as well as hopefully ends up being a less risky/easier experience when we're merging.

[kaicataldo]

As a concrete example, I believe what I suggest above would have helped prevent this: #8565 (comment)

[not-an-aardvark]

I'm fine with having this be a part of our review process, but in any case I think we need to improve our automation. At the moment, the bot is misleading us into thinking a PR is valid when it actually isn't, and it's misleading contributors into amending their commits when it's actually not necessary.

I've also been trying to look at the title before merging a PR, but as we've seen, it's easy to forget.

I think we should:

• Add a required status check to the PRs that checks whether the commit message is valid.
• When failing, the status check would link to a page with detailed instructions for how to fix the problems.
• Stop all automated comments on PRs, including "LGTM" comments.

We could fairly easily add this to eslint-github-bot, as soon as we figure out where we're hosting it.

[not-an-aardvark]

This is fixed now that we've deployed eslint-github-bot.