Consider switching to native GitHub automerge

[CAM-Gerlach]

Presently, as originally added per #29 and most recently discussed and further documented per python/devguide#518 , CPython PRs are auto-merged by the bots via the automerge GItHub label. However, for a while now, GitHub has now implemented a built-in, native automerge feature, which is integrated with the GitHub UI, familiar to devs on other repos, can be customized in the repo's branch protection settings, and doesn't require a bunch of bespoke code and processes on our end.

Do we need to continue maintaining and using the bespoke label-based bot automerge, or can we switch to using the standard, built-in GitHub feature?

One thing I can think of is automerge being triggered automatically on miss-islington PRs—but perhaps that could be changed to just having miss-islington enable GitHub automerge on the PR through the API, if that's possible?

[AlexWaygood]

One thing I can think of is automerge being triggered automatically on miss-islington PRs—but perhaps that could be changed to just having miss-islington enable GitHub automerge on the PR through the API, if that's possible?

The automerge label actually has no effect on whether miss-islington merges her own PRs; miss-islington always merges her own PRs as soon as they've passed CI and been approved by a core dev, whether or not the PR has the "automerge" label attached to it. The "automerge" label only impacts whether miss-islington merges somebody else's PR.

[CAM-Gerlach]

Right; in that bit I was referring to the core automerge logic and functionality (that @hugovk referred to when suggesting creating this issue in python/cpython#102513 ) rather than just the label that manually triggers it.

[kumaraditya303]

+1

I like using Github's built-in automerge. I guess @ambv has the necessary permissions to enable it for us?

[CAM-Gerlach]

He's an org owner, so yup, assuming there are no objections. It can be enabled under Settings > General > Pull Requests (the previous is a direct link to that section).

[gpshead]

github based automerge is going to wind up merging things when we are not yet ready and will wind up with poor commit messages.

[gpshead]

Our current workflow is manual merge-trigger based. Thus we do not have CI checks that actually understand when a PR is "officially" ready. Having had a single core dev hit Approve on a PR does not mean it is ready to be merged.

We still have human judgement in that pipeline and may be awaiting feedback from others, etc. Approve is basically a thumbs up while we wait for humans to decide. Explicitly adding the automerge label is our existing way of indicating we believe on everyones behalf that it is ready to go in with whatever random-ass commit message the robot comes up with after all defined Required CI steps pass, as opposed to just indicating that "our part of the review is done and satisfactory" which is how many of us view Approve today.

Using github automerge would be a significant workflow change that will trip us up. What is the actual goal behind doing it? Less bot maintenance and reliance?

Once size doesn't fit all PRs.

[CAM-Gerlach]

I don't follow, sorry. As far as I'm aware, the criteria the bot uses to automerge is the same as the built-in GitHub method—all required checks must pass, and all other branch protection/merge requirements must be met. And unlike the bot-based automerge, its triggered by actually explicitly clicking the merge button, (which only a core dev can do vs. any triager can add the label, although I think the bot might check if a core dev added it). Furthermore, unlike the bot method, the merging core dev can either accept the default commit message, edit it or write one of their own, just like with normal merging. Therefore, I don't really understand how this would change anything you mention above, except for the better?

[gpshead]

I assumed "native GitHub automerge" meant something doing it based on CI status rather than an explicit click. It sounds like I'm mistaken and that their concept of "auto" just means "a committer clicked through and wrote the message and declared it ready" same as we do by applying the label today but with more control?

In that case... Yes, github's feature sounds better! Even if the name and their opening description didn't match my understanding.

[Mariatta]

The one status check we're still missing is the "do-not-merge", which should be set when someone applies the "do not merge"label. If we can set that up then I think we can go with GitHub automerge feature.

[hugovk]

@gpshead In both cases a human has made the judgement to merge the PR, and wants to as soon as the CI required checks are complete.

Before: human applies automerge label. Bot merges when required checks pass.

After: the merge button becomes an "Enable auto-merge" button. The human clicks it. GitHub merges when required checks pass.

[hugovk]

The one status check we're still missing is the "do-not-merge", which should be set when someone applies the "do not merge"label. If we can set that up then I think we can go with GitHub automerge feature.

I use https://github.com/mheap/github-action-required-labels to require the presence of a label before merge (e.g. https://github.com/hugovk/pepotron/blob/main/.github/workflows/require-pr-label.yml).

It can also require the absence of a label:

- uses: mheap/github-action-required-labels@v4
  with:
    mode: exactly
    count: 0
    labels: "do not merge" 

https://github.com/mheap/github-action-required-labels#prevent-merging-if-a-label-exists

[hugovk]

Please see PR python/cpython#103337.

[hugovk]

python/cpython#103337 has been merged to main and 3.11.

Next steps:

• Org admin: in the branch protection rules (for all branches with rules), set this new check as required (we can do this now)

• Remove or disable auto-merge functionality from https://github.com/python/miss-islington

• Org admin: check "Allow auto-merge" at https://github.com/python/cpython/settings (docs)

• Remove automerge label from https://github.com/python/cpython/labels?q=automerge

• Remove automerge label from https://devguide.python.org/triage/labels/#labels-specific-to-prs and document GitHub auto-merge feature

[arhadthedev]

Include GitHub auto-merge in the label description at https://devguide.python.org/triage/labels/#labels-specific-to-prs

Should we keep the automerge label there?

automerge: for automatically merging PRs approved by a core dev once all CI checks pass.

[hugovk]

Good point, we can remove it afterwards. Checklist updated!

[CAM-Gerlach]

I would suggest the following order, to ensure the new functionality is available and documented before disabling the old, plus one new item for miss-islington:

1. Mark DO-NOT-MERGE as a required check in branch protection on all CPython branches (except for *)
2. Enable "Allow automerge" repo option on cpython, devguide, peps and core-workflow
3. Remove automerge label from devguide and document GitHub automerge instead
4. After ensuring there are no existing PRs with the automerge label, remove the label on GitHub
5. Convert miss-islington to activate the built-in GitHub automerge feature via the Git API instead (if possible) and remove the old bespoke automerge codepaths where practical

[Mariatta]

I will work on removing automerge from miss-islington

[Mariatta]

We still don't have PR to devguide to document the GitHub automerge yet. Anyone wanna write some docs?

[hugovk]

Auto-merge has been enabled for the CPython, devguide, peps, and core-workflow repos.

The automerge label has been removed from the CPython repo.

[arhadthedev]

We still don't have PR to devguide to document the GitHub automerge yet. Anyone wanna write some docs?

This is the only remaining item.

[hugovk]

We have a volunteer to document it at python/devguide#1079

The GitHub docs are at: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/automatically-merging-a-pull-request

Also looks like python/miss-islington#610 removed a bit too much: miss-islington should still merge approved backports.

Let's revert it to bring the backport behaviour back. The Miss Islington automerge functionality won't trigger because the label doesn't exist. We can make a new PR that's more focused.

[AlexWaygood]

Also looks like python/miss-islington#610 removed a bit too much: miss-islington should still merge approved backports.

Let's revert it to bring the backport behaviour back. The Miss Islington automerge functionality won't trigger because the label doesn't exist. We can make a new PR that's more focused.

I'm actually not sure that this is an issue, now that it's been done. Enabling the GitHub automerge on miss-islington-created backport PRs is very easy -- it worked very well on this backport PR just now: python/cpython#103772. We would just need to get the word out to core devs that they now need to enable automerge on backport PRs instead of just approving them and hoping miss-islington merges them.

[hugovk]

Sounds good, let's try that. Better to use also GitHub's own functionality in other places, and we can avoid having more code to maintain.

[AlexWaygood]

Better to use also GitHub's own functionality in other places, and we can avoid having more code to maintain.

+10

[hugovk]

I think we're all done here, thanks everyone! The PyCon sprints work!

[CAM-Gerlach]

Don't we still need to change the docs workflow to not filter by path but rather just skip based on a check in the first step, like the existing code checks do? It seems this is causing an issue otherwise as either the docs check is marked as required, which never runs on PRs that don't touch them and thus cannot be merged, whereas is it is not required, PRs can get merged if they break the docs build.

I discussed this yesterday with @ambv ; not sure if this is in progress yet or I should just jump on that part? Haven't talked to him today about it due to the impact of the Bedevere issue.

[ambv]

I was on this today until @bedevere-bot got blocked by GitHub after his email address got unverified because it was set to a mailing list address, and that mailing list got deleted. This is addressed now.

I will be resuming fixing Docs and Doctest tomorrow.

[CAM-Gerlach]

Yup, figured you were busy with that as mentioned. Thanks for the quick fix there!

[AlexWaygood]

Here's a PR to make the docs build more resilient to changes in transient dependencies, which should make it less risky to make the Docs build a "required" check:

• Docs build: Add upper bounds to transitive dependencies cpython#103860

[CAM-Gerlach]

@ambv One thing to note—due to the new check for requested-changes reviews (by way of the Bedevere label), PR commits are now all red even if anything else passes so long as there is at least one request-changes review active on the PR, which is rather annoying/unhelpful. For example, on python/cpython#103853 .

Not sure there's a way to work around it, though, other than just not making that check required and having core devs just not automerge if another core dev has requested changes (just like they had to previously with regular merges), or cancel automerge or apply DO-NOT-MERGE if it is already in progress.