search

googleforgames / quilkin

Quilkin is a non-transparent UDP proxy specifically designed for use with large scale multiplayer dedicated game server deployments, to ensure security, access control, telemetry data, metrics and more.
Apache License 2.0
1.28k stars 92 forks source link

Use Merge Queue instead of requiring PRs to be up to date #737

Open XAMPPRocky opened 1 year ago

XAMPPRocky commented 1 year ago

GitHub has a new merge queue feature, this would replace the need to have the requirement that all PRs are up to date with main before they can be merged, as the point of the merge queue is to handle that automatically for you to prevent merge skew conflicts. This would save a lot of time, as if I open a lot of different pull requests, and even if they're all approved it could an additional hours of manually clicking "update" on each the PR as another gets merged before everything gets merged.

https://github.blog/changelog/2023-02-08-pull-request-merge-queue-public-beta/

markmandel commented 1 year ago

Merge queue is really nice - it doesn't tend to work well with our CLA bot at the moment.

I did setup on Agones adRise/update-pr-branch (https://github.com/googleforgames/agones/blob/main/.github/workflows/pr_update.yml) and that's been working really well.

XAMPPRocky commented 1 year ago

It's now generally available. Would we be able to try it?

markmandel commented 1 year ago

I just did some hunting internally, and I think it does work now with our CLA bot. Let's take it for a spin!

There's some things I'll need to do with our CI I think, but they should be pretty easy: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#triggering-merge-group-checks-with-third-party-ci-providers

markmandel commented 1 year ago

I've turned on merge queues, and I'm fairly sure I have Cloud Build CI setup (assuming my regex is good), so we can take this for a spin and see how it goes.

markmandel commented 1 year ago

So here lies a tricky problem I'm not 100% sure how to solve.

You can see it here: image

Status: CI (quilkin) only runs on GitHub PR (current not required) Status: CI-merge-queue (quilkin) only runs on merge queue branch creation. (required).

764 only merge because I edited the required status checks during a merge queue process.

I don't think I can make the two the same.

Also not sure How I'm allowed to "merge when ready" when there aren't enough approval on a PR.

See https://github.com/googleforgames/quilkin/pull/712 for example. Looks like we could click "merge when ready", even though there are no approvals.

Also noting you can't reword using merge queues either.

I think this above is going to be a dealbreak for us. I'll leave it alone for now, see how it goes, and see if we see another way through.

So I may suggest we also adopt the workflow in https://github.com/googleforgames/quilkin/issues/737#issuecomment-1516046536 with automatically updates PRs that are set to automerge on passing, so that people don't have to click the button to update either.

It's been working really well for the Agones project.

markmandel commented 1 year ago

Also not sure How I'm allowed to "merge when ready" when there aren't enough approval on a PR.

Chatted with GitHub friends - so the PR won't be added to the merge queue unless it's approved. Not well documented, but that's actually kinda useful as you can pre-bank PRs that are awaiting approval.

markmandel commented 1 year ago

I think this above is going to be a dealbreak for us. I'll leave it alone for now, see how it goes, and see if we see another way through.

Just playing with this some more - not 100% sure on this. The current branch protection requirements might be fine. We may have have to try it and see. So leaving alone as it is.

markmandel commented 1 year ago

Going to go bug some Cloud Build PMs to see what's possible 😄

In the meantime, I'll get https://github.com/googleforgames/agones/blob/main/.github/workflows/pr_update.yml going, so we can automate PR's that are set to auto-merge.

Not as nice as merge queues, but at our rate of throughput, should be good enough for now.