Repository navigation
Bot to check if PR is not against the master branch, and it is not a backport PR #211
Description
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?
Activity
brettcannon commentedon Jan 15, 2018
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 themaster."ncoghlan commentedon Jun 24, 2018
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 (
masterif there's no prefix,python:X.Yif there's a[X.Y]prefix), or else the relevant commit message prefix added.Mariatta commentedon Jun 24, 2018
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
nidoes).One limitation of status check is the text length.
Mariatta commentedon Jun 24, 2018
Wait I misspoke 😅
nidoesn'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 commentedon Jun 24, 2018
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":
Post a status check for backport PR title.
Mariatta commentedon Jun 24, 2018
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 commentedon Jun 24, 2018
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 commentedon Jun 24, 2018
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.
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 commentedon Jul 2, 2018
I think this should be optional, and one of the admins can make it optional.
Yes they should.
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.
Post a status check for backport PR title.
Post a status check for backport PR title. (#112)
serhiy-storchaka commentedon Jul 26, 2018
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 commentedon Jul 26, 2018
@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 commentedon Jul 28, 2018
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 commentedon Jul 28, 2018
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 commentedon Aug 4, 2018
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 commentedon Aug 4, 2018
@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 commentedon Aug 4, 2018
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 commentedon Aug 4, 2018
🍝
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:
