Test-infra: @-mentioning a GitHub user in a PR title creates merge commit that Prow then rejects

[sftim]

What happened:
Prow merged a PR (via GitHub). The merge commit contained a commit message that Prow later rejected when that branch was proposed for merge.

What you expected to happen:
It should not be possible to command Prow to create a commit that Prow then considers unacceptable. Alternatively, it _is_ possible but you have to explicitly opt-in to the problem.

How to reproduce it (as minimally and precisely as possible):
(untested)
Start from the _master_ branch
Create a new branch with changes and create a pull request targeting _master_
Ensure that the title (not commit message, title) of the PR mentions a GitHub user

Please provide links to example occurrences, if any:
https://github.com/kubernetes/website/pull/21604#event-3425477433

Anything else we need to know?:
Seen on k/website during a merge into the branch for the upcoming (v1.19) release.

[savitharaghunathan]

/cc

[zacharysarah]

@kubernetes/sig-testing-pr-reviews Boosting for priority. This issue affects the viability of the 1.19 release branch. The longer this issue goes unresolved, the more potential merge conflicts arise in the 1.19 release branch.

It's not obvious to SIG Docs how to fix this particular issue. The breaking PR's commit message (https://github.com/kubernetes/website/commit/53db74cd4604ac73a7d3e900231b36ff51d90be5) isn't the commit used in the branch history (https://github.com/kubernetes/website/pull/21428/commits/bb04eb5bf5b5863b83e0fb6732eaae6c25abbfa4), so there's nothing to amend--not that we could (or would want to) force push anyway, due to branch protection.

/sig testing
/priority important-soon

[BenTheElder]

So FWIW Prow does not create commits, it does the API equivalent of "clicking merge".

The commit contents are generated by github, which include the title.

The tooling that blocks merges of invalid commit messages should probably block invalid titles?

cc @cjwagner @alvaroaleman

To get out of this temporarily we can have a repo or org admin manually merge the affected PR. cc @mrbobbytables @cblecker

[spiffxp]

FWIW it's not just repo admin, people with write access to the repo (ability to add/remove labels manually) can solve this as well.

I agree with @BenTheElder that the long term fix is to have invalidcommitmsg check PR titles as well. In the short term I think you're unblocked unless you intend to keep https://github.com/kubernetes/website/pull/21604 open as a long-running PR

[zacharysarah]

@spiffxp Because dev-1.19 is a long running branch, I suspect we'll see the issue reappear in https://github.com/kubernetes/website/pull/20785. But--if peeling the label off works, that may solve the problem. 🤷

[zacharysarah]

The label didn't propagate downstream in #20785, so I think peeling the label off is a win in this specific situation. 👍

[nasa9084]

I'm sorry the problem is caused by my bad PR and thank you for working to fix!

And also I'm thinking this is a chance to contribute to k/test-infra.
Can I try to fix (checking PR titles in invalidcommitmsg)?

[sftim]

The tooling that blocks merges of invalid commit messages should probably block invalid titles?

I think so.

Another option is to teach Prow how to merge (which is a lot more involved) and then customize its merge behavior.

[cjwagner]

I agree that this is probably best handled by invalidcommitmsg.

Another option is to teach Prow how to merge (which is a lot more involved) and then customize its merge behavior.

It is possible to override the default merge commit message using this Tide config option: https://github.com/kubernetes/test-infra/blob/0a5f2cb0311abe52f98770f0f1f15b5aec4ac0e2/prow/config/tide.go#L91-L94
https://github.com/kubernetes/test-infra/blob/0a5f2cb0311abe52f98770f0f1f15b5aec4ac0e2/prow/config/tide.go#L67-L70

[nasa9084]

then we have two options,

• implement a feature which checks PR title, in invalidcommitmsg
• add merge_commit_template with body templating, in config/prow/config.yml

right? which one is better?
adding config seems easier way (also for contributors)?

[BenTheElder]

I mean sure we could implement a merge message template but it should still have the user supplied text in it, and that text triggering poor GitHub behavior is the problem here.

We should reject the PR title and inform the user. We have a command that allows anyone in the organization to retitle the PR for them.

[annajung]

Hi all, just wanted to raise this issue again since it's happening in the 1.20 release as well.
In addition to @, it looks like we also need to look for # in the PR title as well.

Example:

• https://github.com/kubernetes/website/pull/24290#issuecomment-701713998
• https://github.com/kubernetes/website/pull/24062

cc @jimangel @savitharaghunathan

[nikhita]

Created https://github.com/kubernetes/test-infra/pull/19491 to fix this

/assign