Bot to check if PR is not against the master branch, and it is not a backport PR

[Mariatta]

Once in a while people make a PR against the maintenance branch, but it should be made against the master first.

From the GitHub web UI, we can see which branch the PR is targeting to, for example python:master or python:3.6, but not when viewing it using mobile device.
It is also easy to miss, basically it's another mental checklist core devs have to do before merging a PR.

Wondering if this is something bedevere can detect and remind people about?

[brettcannon]

Bedevere could leave a comment saying something like "We noticed you made a pull request against a branch other than master. While this is occasionally on purpose, it can also easily be by accident. If this is a case of the latter, please close this pull request and open a new one against the master."

[ncoghlan]

From python/bedevere#111: when @terryjreedy asked about this, I realised we could use the [X.Y] prefixes on commit messages to distinguish between folks targeting a maintenance branch deliberately (e.g. to resolve a backport conflict, or to fix a bug in something that's removed entirely in a later release) and folks doing it accidentally.

That means the change in Bedevere could be to fail the PR consistency check if the target branch and the commit message prefix don't match, and add a comment pointing out that either the target branch needs to be changed (master if there's no prefix, python:X.Y if there's a [X.Y] prefix), or else the relevant commit message prefix added.

[Mariatta]

Do we really want it as a status check though? I was thinking posting a message was good enough.
We can do both too (like what ni does).
One limitation of status check is the text length.

[Mariatta]

Wait I misspoke 😅 ni doesn't do a status check. It does a label and a comment.
I know there is an open issue about making it post a status check.

[ncoghlan]

Bedevere already does a status check for missing BPO references and NEWS entries, so I was figuring it made sense to tie it into that as a general "Metadata consistency check":

• bpo reference, or skip-issue label
• NEWS entry, or skip-news label
• master PR with no version prefix, or maintenance branch PR with maintenance branch prefix

[Mariatta]

In PR https://github.com/python/bedevere/pull/112/files, I added a new status check, and it is done only for PRs not against the master branch.

If the PR title does not match [X.Y] title (GH-NNNN), it will post a failure status, with the message "Not a valid backport PR title.", with link to DevGuide on how to format the message.

If the PR title match, it will post a success status, with the message "Valid backport PR title.".

Let me know if these messages will work. At least the status will provide some indication that something is not quite right with the PR.

There is not enough space there to say "alternatively, close the PR and target the master branch".

[terryjreedy]

Should not every PR against x.y have [x.y] in the title? or only backports of another PR? If the former, then the messages would be relevant but not quite right for original x.y PRs.

Would this be a 'required' check, like 'bedevere/news'?

[terryjreedy]

The devguide link could explain that status failure might be because the PR is against the wrong branch, and give the solution. I believe there are at least 3 cases.

1. Branch was derived from master and author for some reason changed that on PR form. Unlikely.
2. Branch was derived from x.y branch. More likely.
   2a. Remaking PR by selecting 'master' instead of x.y gives a PR that merges, and likely passes tests.
   IE, if it had be made against master originally, it would have backported cleanly.
   2b. Doing above gives PR that does not merge. It might be better to start over with branch off of master.

The issue that prompted me to open the duplicate bedevere issue was 2a.

[Mariatta]

Would this be a 'required' check, like 'bedevere/news'?

I think this should be optional, and one of the admins can make it optional.

Should not every PR against x.y have [x.y] in the title?

Yes they should.

or only backports of another PR?

Backports of another PR will additionally have reference to the original PR (GH-NNNN)

I think PRs to [X.Y] that is not a backport should be rare, that is why I decided to check for both.