Enable merge_group trigger for merge queues #35062

tobiasdiez · 2023-02-10T16:08:57Z

📚 Description

Two days ago github announced the public beta of merge queues. The idea is pretty similar the current release/merge process (as far as I understand it): Given you want to merge three PRs into develop, you add them to the merge queue and github will create three temporary branches:

PR1
PR1 + PR2 merged
PR1 + PR2 + PR3 merged

It then runs all checks with the merge_group trigger on each branch. Once all checks for all these temporary branches are successful, the whole group is merged into develop. If there are merge conflicts, say between PR3 and PR1, or the build for the PR1+2+3 branch fails, then PR3 is removed from the merge queue and only the first two PRs are merged into develop.

In this way, one could never end up in a situation where the checks are not passing in the develop branch, without requiring devs to always merge the latest develop branch themselves. (E.g., we will no longer hit the situation that PR2 introduces new linter rules which are not fullfiled by PR3 but both PRs are merged into develop).

References:

In this PR we add the merge_group trigger on the current PR-workflows, which enables to run these in the merge queue as well.

Since this effectively implements the current version of "Volker's temporary private develop branch", I was wondering if this would be eneough to give more people rights to merge PRs. What else would be necessary for such a step? @vbraun

📝 Checklist

I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

dimpase · 2023-02-10T16:20:40Z

why is master branch mentioned there?

tobiasdiez · 2023-02-10T16:31:37Z

Github will create temporary branches that will trigger the push event. This change restricts the push trigger to the only two branches that we currently have and ignores these temporary branches. Currently the push trigger is equivalent to push: -master -develop.

dimpase · 2023-02-10T16:32:25Z

maybe I'm mistreading what the patch does, but appearnce of master right next to develop makes me think you want to do this queue on master as well as on develop

dimpase · 2023-02-10T16:33:47Z

change restricts the push trigger to the only two branches that we currently have

but why do we need it on master? I'd say we don't...

tobiasdiez · 2023-02-10T16:39:58Z

maybe I'm mistreading what the patch does, but appearnce of master right next to develop makes me think you want to do this queue on master as well as on develop

This is a bit of a stupid yml syntax. The indented "branches" only apply to "push" and not to "merge_queue".

change restricts the push trigger to the only two branches that we currently have

but why do we need it on master? I'd say we don't...

I was not sure about this change, since the role of the master branch is not clear to me. If it is indeed only an old copy of the develop branch and does not receive any updates, then sure we don't need to run these workflows on it (but then why is there a master branch in the first place?). If the master branch may be updated (hotfixes?) than you probably want to verify that the new commits adhere at least to these basic tests.

mkoeppe · 2023-02-10T18:13:39Z

The master branch is fast-forwarded to stable release tags made on the develop branch.

.github/workflows/build.yml

tobiasdiez · 2023-07-12T09:22:49Z

@vbraun Did you already found the time to look into merge queues? In its current form this PR doesn't replace your buildbots but it ensures that PR's don't introduce changes that make other PRs fail (when merged at the same time). This happened a few times in the last months and is pretty annoying since then the github workflow fails for every new PR.

tobiasdiez · 2023-09-28T03:05:27Z

@dimpase since you were interested in the merge queue feature.
My proposal would be to merge this PR now. Then we can test it by merging some test PRs into some test branch in the main repo (for which I can activate the merge queue independent of the develop branch). Once these tests are successful, we can approach @vbraun and discuss how to best integrate the merge queues with his release manager scripts (refs sagemath/sage-release-management#2). What do you think?

github-actions · 2023-09-28T06:15:47Z

Documentation preview for this PR (built with commit 043e76a; changes) is ready! 🎉

dimpase · 2023-09-28T06:49:56Z

could it rather have a manual trigger, for the time being?

dimpase · 2023-09-28T08:23:17Z

master branch ought to be removed here, IMHO

tobiasdiez · 2023-09-28T08:38:01Z

One can only trigger a merge queue by merging PRs. There is no other way.

(The master branch there is just for completeness - it looks like its no longer used, but officially its still there.)

dimpase · 2023-09-28T08:42:20Z

does this mean that PRs are merged automatically?
Into a temporary branch?

So then they are still mergeable in the usual way?

tobiasdiez · 2023-09-28T09:06:15Z

For the PRs targeting develop nothing changes. But I can configure it in github ui that all PRs that target a new 'test' branch have to go through merge queues

dimpase · 2023-09-28T11:03:29Z

what does it mean for PR to " target" the 'test' branch?

tobiasdiez · 2023-09-28T12:00:26Z

If you open a PR you can say into which branch you want to merge this PR.

dimpase · 2023-09-28T12:34:05Z

Oh, right, of course. So, suppose this happens, and the PR gets more or less automatically merged into 'test'.
How is it going to reach 'develop' branch?

tobiasdiez · 2023-09-28T13:00:41Z

Never ;-) The point of the 'test' branch would be to merge stupid test PRs (some nonsensical changes in the readme say, some others that break the build etc) in order to test that the merge queue works. Once this is established, we can approach @vbraun and ask that real PRs are merged into the develop branch using the merge queue.

dimpase

OK

mkoeppe · 2023-09-28T19:55:35Z

Do these experiments to get familiar with merge queues really have to be done in this repo? I'd suggest not.

dimpase · 2023-09-28T21:53:51Z

Why not? It's not breaking anything

tobiasdiez · 2023-09-29T06:39:25Z

Pull request merge queues are available in any public repository owned by an organization

So you cannot try it out in a private fork.

Thanks @dimpase for your review!

@vbraun

### 📚 Description Two days ago [github announced the public beta of merge queues](https://github.blog/changelog/2023-02-08-pull-request-merge- queue-public-beta/). The idea is pretty similar the current release/merge process (as far as I understand it): Given you want to merge three PRs into `develop`, you add them to the merge queue and github will create three temporary branches: - PR1 - PR1 + PR2 merged - PR1 + PR2 + PR3 merged It then runs all checks with the `merge_group` trigger on each branch. Once all checks for all these temporary branches are successful, the whole group is merged into `develop`. If there are merge conflicts, say between PR3 and PR1, or the build for the PR1+2+3 branch fails, then PR3 is removed from the merge queue and only the first two PRs are merged into `develop`. In this way, one could never end up in a situation where the checks are not passing in the develop branch, without requiring devs to always merge the latest develop branch themselves. (E.g., we will no longer hit the situation that PR2 introduces new linter rules which are not fullfiled by PR3 but both PRs are merged into develop). References: - https://docs.github.com/en/repositories/configuring-branches-and- merges-in-your-repository/configuring-pull-request-merges/managing-a- merge-queue - https://docs.github.com/en/actions/using-workflows/events-that- trigger-workflows#merge_group --- In this PR we add the merge_group trigger on the current PR-workflows, which enables to run these in the merge queue as well. --- Since this effectively implements the current version of "Volker's temporary private develop branch", I was wondering if this would be eneough to give more people rights to merge PRs. What else would be necessary for such a step? @vbraun    ### 📝 Checklist    - [ ] I have linked an issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#35062 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Matthias Köppe, Tobias Diez

tobiasdiez · 2023-10-09T12:33:21Z

I've now enabled the merge queue feature. You can try it out by creating a PR against https://github.com/sagemath/sage/tree/test_merge_queue. For example, #36432.

Currently, only the linter workflow has to pass for the merge to succeed.

Tests if the merge queue added in #35062 is working. ### 📝 Checklist     - [ ] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies

Enable merge_group trigger for merge queues

2d85ece

tobiasdiez requested review from mkoeppe, dimpase and vbraun February 10, 2023 16:10

tobiasdiez added the s: needs review label Feb 10, 2023

mkoeppe reviewed Feb 10, 2023

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

tobiasdiez mentioned this pull request Feb 21, 2023

Merge PR through github API sagemath/sage-release-management#2

Open

Merge branch 'develop' into merge-group

d9a02e1

tobiasdiez mentioned this pull request Aug 29, 2023

Prepare for updating Cython to 3.0.0 #36109

Merged

3 tasks

Merge branch 'develop' into merge-group

043e76a

dimpase approved these changes Sep 28, 2023

View reviewed changes

tobiasdiez added s: positive review and removed s: needs review labels Sep 29, 2023

vbraun merged commit d687f2e into sagemath:develop Oct 8, 2023
12 of 13 checks passed

tobiasdiez deleted the merge-group branch October 8, 2023 14:15

github-actions bot removed the s: positive review label Oct 8, 2023

mkoeppe added this to the sage-10.2 milestone Oct 8, 2023

tobiasdiez mentioned this pull request Oct 9, 2023

Initial test of merge queue #36432

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable merge_group trigger for merge queues #35062

Enable merge_group trigger for merge queues #35062

tobiasdiez commented Feb 10, 2023 •

edited

Loading

dimpase commented Feb 10, 2023

tobiasdiez commented Feb 10, 2023

dimpase commented Feb 10, 2023

dimpase commented Feb 10, 2023

tobiasdiez commented Feb 10, 2023

mkoeppe commented Feb 10, 2023

tobiasdiez commented Jul 12, 2023

tobiasdiez commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023

dimpase commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023 •

edited

Loading

dimpase commented Sep 28, 2023 •

edited

Loading

tobiasdiez commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023

dimpase left a comment

mkoeppe commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 29, 2023 •

edited

Loading

tobiasdiez commented Oct 9, 2023

Enable merge_group trigger for merge queues #35062

Enable merge_group trigger for merge queues #35062

Conversation

tobiasdiez commented Feb 10, 2023 • edited Loading

📚 Description

📝 Checklist

⌛ Dependencies

dimpase commented Feb 10, 2023

tobiasdiez commented Feb 10, 2023

dimpase commented Feb 10, 2023

dimpase commented Feb 10, 2023

tobiasdiez commented Feb 10, 2023

mkoeppe commented Feb 10, 2023

tobiasdiez commented Jul 12, 2023

tobiasdiez commented Sep 28, 2023 • edited Loading

github-actions bot commented Sep 28, 2023

dimpase commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023 • edited Loading

dimpase commented Sep 28, 2023 • edited Loading

tobiasdiez commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 28, 2023

dimpase left a comment

Choose a reason for hiding this comment

mkoeppe commented Sep 28, 2023

dimpase commented Sep 28, 2023

tobiasdiez commented Sep 29, 2023 • edited Loading

tobiasdiez commented Oct 9, 2023

tobiasdiez commented Feb 10, 2023 •

edited

Loading

tobiasdiez commented Sep 28, 2023 •

edited

Loading

tobiasdiez commented Sep 28, 2023 •

edited

Loading

dimpase commented Sep 28, 2023 •

edited

Loading

tobiasdiez commented Sep 29, 2023 •

edited

Loading