tock icon indicating copy to clipboard operation
tock copied to clipboard

Optimize GitHub Merge Queue

Open lschuermann opened this issue 2 years ago • 9 comments

As announced in this month's Bors newsletter, we will have to say goodbye to an old friend: Bors-NG is now officially deprecated. Along with that comes the announcement that the public bors GitHub app and instance may be turned off in the future.

We should take this opportunity to experiment with the GitHub pull request merge queue public beta and get that set up before things break, to be able to retire Bors-NG for good and thank it for its service. :smile:

lschuermann avatar May 02 '23 02:05 lschuermann

Let's plan to check that box on June 1, 2023?

On the master branch:

image

bradjc avatar May 17 '23 18:05 bradjc

Let's plan to check that box on June 1, 2023?

Yes, I think that's a reasonable timeline. Given Alex' concerns on the call though, I do believe that we should test it on some lower-impact repository, e.g. libtock-c. I will reach out to @hudson-ayers to try and get this working as discussed.

lschuermann avatar May 17 '23 20:05 lschuermann

What are the possible issues?

Bors doesn't work with multiple PRs anymore.

bradjc avatar Jun 09 '23 19:06 bradjc

IIUC, we also need to check the Require branches to be up to date before merging to match the functionality of bors.

image

The status checks bors currently requires include qemu working and don't include netlify, but otherwise that default list matches.

ppannuto avatar Jun 09 '23 22:06 ppannuto

IIUC, we also need to check the Require branches to be up to date before merging to match the functionality of bors.

I may well misunderstand this, but I thought that this was the switch to enable if we want the same guarantees as bors (merge queue) gives us, without actually having a merge queue. Instead, this puts the burden on the developer to always rebase branches before they're able to be merged. Having the merge queue should take care of testing the batched merged result for us.

lschuermann avatar Jun 10 '23 08:06 lschuermann

I will have some time today / tonight to test this on a fork of the Tock repo.

lschuermann avatar Jun 10 '23 08:06 lschuermann

Here's what I found out today:

  • The GitHub actions merge queue seems to not behave exactly the same as bors does. Whereas bors always batches queued PRs and builds them exactly once per merged batch via push to the staging branch, the merge queue builds all the individual batched PRs on (successively merged) individual staging branches.

    It seems that this is configurable through the number of parallel builds though, which I am investigating right now.

  • In addition to that, GitHub requires a new merge_group actions trigger to be used for all status checks, which should be run for enqueued PRs. Because PRs are also pushed to temporary branches, this means that we build everything a couple of times:

    • On pushes to pull-request branches, run every workflow for both the pull_request and push trigger (identical behavior to bors),
    • on the merge-queue temporary branches, run every workflow for both the merge_group and push trigger (one more workflow run compared to bors), and
    • on the final push to master, run all workflows through the push trigger.

    Given our very extensive (and compute-heavy) CI, we should find ways to avoid running so many duplicate workflows.

  • Most importantly, GitHub merge queue will still only merge something if the final merged result passes CI (even without the checkbox @ppannuto mentioned). This means that, while the merge queue seems to be more liberal in spending CI minutes, it should be functionally equivalent to bors and seems to work on my fork.

To set it up, we need to

  • add the merge_group triggers to all required workflows,
  • make sure all workflows are marked as required in the branch protection settings,
  • and then enable the merge-queue in the branch protection settings.

Currently investigating how we can get more aggressive batching to happen, to replicate bors' behavior. Presumably the merge-queue supports Octopus Merges, but I couldn't get that to work. Hoping that "Maximum pull requests to build" = 1 will get that to work.

lschuermann avatar Jun 10 '23 18:06 lschuermann

I opened #3483, which includes all repository modifications to get the GitHub merge queue working on the Tock repository. I still didn't manage to have the merge queue perform octopus-merges, so the consumption of GitHub actions minutes & time to merge PRs will go up (when we have congestion).

lschuermann avatar Jun 14 '23 18:06 lschuermann

It seems we've (more or less) successfully made the switch, #3440 was successfully built and merged with the merge queue.

We're still building everything twice before merging, #3486 addresses this.

Also, our Netlify integration does not (yet) work with GitHub's merge queue. I have thus removed the netlify/docs-tockosorg/deploy-preview status check as required from the branch protection settings of master. We're not alone in this, others are also reporting issues: https://answers.netlify.com/t/github-merge-queue-w-netlify/92522. One workaround is certainly to enable Netlify deploys on all branches, however this would significantly increase the number of builds we run, and would also create tons of subdomains for those branches.

We're not losing any thing here, bors technically also didn't require Netlify deploys to go through: https://github.com/tock/tock/blob/aa619112eacec756a6c6ef31434252a9501b2874/.github/bors.toml

However, having docs built on the merged result is certainly nice, and so I propose a workaround to build them through GitHub actions in #3485.

Re-opening this until we've addressed these last issues.

lschuermann avatar Jun 15 '23 16:06 lschuermann