defenseunicorns / uds-core

A FOSS secure runtime platform for mission-critical capabilities
https://uds.defenseunicorns.com
GNU Affero General Public License v3.0
53 stars 21 forks source link

Implement merge queue for PRs #295

Open mjnagel opened 8 months ago

mjnagel commented 8 months ago

Describe what should be investigated or refactored

Currently we require PR status checks to pass + the branch to be up to date with main before they are able to be merged. In practice this is what we want in terms of testing surface area, but it can introduce friction if we want to merge several PRs back to back.

We should consider implementing merge queues possibly since they appear to alleviate some of this pain by stacking PRs up, but still ensuring the proper tests run against the combined changes of everything ahead of it in the queue.

Links to any relevant code

Additional context

This may not be a huge impact most of the time since we aren't dealing with significant volume of PRs, but I think it doesn't seem like it would hurt even if the queue is a single PR? It feels like it would alleviate some of the pain of waiting on another merge / something getting merged before your PR?

mjnagel commented 8 months ago

I think it would be worth exploring this in a personal repo/fork to see in practice how it works and figure out plus/minus to it.

TristanHoladay commented 8 months ago

So far the merge queues do seem like they could help but with some caveats.

Looks like you have to select required checks in order to prevent merging from the queue even though merge_group is added as a workflow trigger. This means that even if a merge queue kicks off a workflow, if it's not selected in the branch protection as a required check, the merge group does not care if that workflow passes or fails.

This potentially makes things tricky for our dynamic tests, meaning we won't want to make single-package tests required since they may or may not kick off per PR. We could make the all and upgraded tests required which seems good, but this presents another blehhh. If we make a check required, then it has to run before the PR can be added to the merge-queue. So the tests have to run 2x (PR then again in merge queue). Really the big loss here is we probably can't move our bulky tests off of the PR run and onto the merge-queue run.

Also, we'll want our tests to fail a lot less often for stupid reasons since PRs are kicked from the queue when tests fail and that then affects any trailing PRs in the queue, causing re-runs of their tests.

mjnagel commented 3 months ago

Revisiting the findings here - we've recently had more back to back PRs and would like to get some final thoughts from the team and implement or close this out.

If we make a check required, then it has to run before the PR can be added to the merge-queue. So the tests have to run 2x (PR then again in merge queue).

This appears to be the only real downside - as mentioned here in PRs that are already up to date with main checks would unnecessarily run twice. For PRs that are out of date checks would run 2x regardless of the number of PRs merged before them, which would be a value add.

Overall I see this as a positive - you would never have to tick that "Update branch" button (although it may be desirable in some cases to ensure things don't get kicked out of the merge queue. PRs that get merged without needing a main update feel pretty rare these days.