Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent manual merges #8396

Open
mhofman opened this issue Sep 27, 2023 · 4 comments
Open

Prevent manual merges #8396

mhofman opened this issue Sep 27, 2023 · 4 comments
Assignees
Labels
enhancement New feature or request tooling repo-wide infrastructure

Comments

@mhofman
Copy link
Member

mhofman commented Sep 27, 2023

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 didn't use a consistent app account for merging
  • mergify couldn't be used for PRs updating its own config
  • we wanted the ability to force a PR to land immediately

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

@mhofman mhofman added enhancement New feature or request tooling repo-wide infrastructure labels Sep 27, 2023
@turadg
Copy link
Member

turadg commented Oct 5, 2023

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

@mhofman
Copy link
Member Author

mhofman commented Oct 5, 2023

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 frazarshad self-assigned this Jul 26, 2024
mergify bot added a commit that referenced this issue Aug 4, 2024
refs: #8396

## Description

This PR adds a new high-priority queue to mergify. A high priority queue allows us to merge urgent PRs before low priority ones, essentially speeding up patch/hotfix release.
This does not bypass any CI checks for merging so all the normal standard is still followed even for high priority queues

Resources: https://docs.mergify.com/merge-queue/multi/

The way to use this new high priority queue is to add a new label `priority:high` github and add that on high priority PRs. 

### Fallback:
In case the new queue does not work as expected, the original queue is sill present and working

### Security Considerations


### Scaling Considerations


### Documentation Considerations


### Testing Considerations


### Upgrade Considerations
@frazarshad
Copy link
Contributor

  • 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
Copy link
Member Author

mhofman commented Aug 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tooling repo-wide infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants