search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
326 stars 206 forks source link

Prevent manual merges #8396

Open mhofman opened 12 months ago

mhofman commented 12 months ago

What is the Problem Being Solved?

8253 added a required check that a merge strategy was selected, making it slightly more difficult to bypass Mergify accidentally, but not impossible.

We have historically not been able to force that PRs are merged with mergify for protected branches for a variety of reasons:

Mergify has now solutions for all these issues

Description of the Design

Security Considerations

Makes sure CI jobs pass before merging, no accidental bypass

Scaling Considerations

The priority queue should help getting urgent fixes, but if mergify breaks for some reason, we'll have to fallback to admins disabling the extra protection rules.

Test Plan

We now have a dedicated repo where we can experiment with these kind of changes before impacting the agoric-sdk repo.

Upgrade Considerations

None

turadg commented 11 months ago

For opt-in safety we could make a userscript like https://gist.github.com/turadg/a6871cafba99d6b3fc281f407cd64e55

mhofman commented 11 months ago

Yeah but that requires all users to run that user script in their browsers. Unless we have a browser extension mandated by org policy (like my previous job had), it's probably too brittle.

On the other hand, mergify breaking requiring admin action is also a pretty big downside.

frazarshad commented 1 month ago
  • Change branch protection rules to only allow the mergify bot app account to write / merge
  • remove the adhoc "mergify-ready" check

@mhofman do you think we can complete these two tasks now to fully close this issue?

mhofman commented 1 month ago

I think we're in a good place to try. The first bullet will require some experiments. In particular I'm not sure the effects this may have on rebased PRs authorship. We likely need to think through having an escape hatch if mergify is down or broken. And we need to test updating mergify using mergify itself.