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.

[serhiy-storchaka]

How to make this check successful if the PR is not against master and is not a backport? For example if it fixes 2.7 only bug?

[Mariatta]

@serhiy-storchaka does that happen often?
In any case this check is optional, and you can ignore it and still merge the PR.
To make it pass I'll need to change the regex to never check for the GH number.

[ncoghlan]

For 2.7, never checking for the GH number would be the right thing to do - even the documentation has diverged sufficiently that it's usually easier to just make two PRs than it is to even try to backport a 3.x change to 2.7.

For 3.x branches, the current behaviour is likely OK - while we may occasionally get version-specific issues because master has since been refactored or otherwise changed to avoid the problem completely, that doesn't happen very often, and when it does, we can either ignore the check, or else add a reference to the refactoring or enhancement PR that made the issue inapplicable to more recent branches.

[terryjreedy]

To me, adding the 2.7 backport tag when adding others, and letting the bot tell me if it works or not, is easy. It worked for the last small doc change I merged.

If a patch is a backport from master done by hand, it is easy to add (GH-NNNN) to the title when copying the title for the initial commit. Most 2.7 merges are backports.

The issue here is PRs for x.y that are not backports and cannot have such a tag. Rather than having to ignore a warning, we could add a 'non backport' tag, similar to 'no issue' and 'no news', to negate the check.

[ncoghlan]

I attempted to work around this issue by editing the title on python/cpython#8659 to be self-referential, but it looks like title edits don't trigger any of the bots to re-run.

[Mariatta]

@ncoghlan python/cpython#8659 needs to start with [3.7]. As I've said, the check is currently "not required" and does not block merging. The merge button is still enabled (but it is not green).

[ncoghlan]

That worked, thanks! So in the absence of a dedicated label (which we may decide not to do, since this doesn't come up very often), we can use a self-reference to the same PR to make the check happy

(While I was a proponent of the "just ignore it" approach, it turns out GitHub make it quite disconcerting to review a PR when even an advisory check is failing - it gets a red X in lots of places, including the page favicon, and the big green Merge button remains grey, even though it's active)

[Mariatta]

🍝

I've deployed the latest change in bedevere, so now it is only looking for the [X.Y] prefix, and it does not care about the (GH-NNNN) anymore.
But this status is now "Required".

Example: