search

commontoolsinc / system

A constellation of Common tools
4 stars 0 forks source link

Auto-merge PRs after all checks pass #75

Open cdata opened 4 months ago

cdata commented 4 months ago

It would be nice if we could automate the merge step for PRs.

A candidate for auto-merge is any PR that passes all checks, including: lints, tests and human reviewer approval.

PRs should be merged using Github's squash/merge strategy.

We probably want to block this on https://github.com/commontoolsinc/system/issues/68 to ensure that squash commit messages are always valid conventional commits.

wfaithfull commented 3 months ago

Thought this was already enabled but it's probably because we're lacking a branch protection rule:

Note: The option to enable auto-merge is shown only on pull requests that cannot be merged immediately. For example, when a branch protection rule enforces "Require pull request reviews before merging" or "Require status checks to pass before merging" and these conditions are not yet met. For more information, see "Managing a branch protection rule."

@Mearman

Mearman commented 3 months ago

@cdata are you happy for me to set up the repo rules?

cdata commented 3 months ago

@Mearman @wfaithfull can you first verify that they are in fact not set up? My read is that we have set the appropriate rules up already.

wfaithfull commented 3 months ago

I'm afraid I lack the access to interrogate the settings on this repo.

Mearman commented 3 months ago

@cdata sorry I wasn't specific enough. Yes my plan is to review the current ones and set up / modify as required. I think @wfaithfull may have had it immediately merge because the checks were already passed.

wfaithfull commented 3 months ago

I was able to merge https://github.com/commontoolsinc/system/pull/144 yesterday without all the checks passing. You can see I merged it before the test flake comment was posted. It was a mistake, since I assumed it was was going to set auto-merge after the checks passed.

wfaithfull commented 3 months ago

I think it is working, and this is probably why. All our team are at a suitable level of access to be captured by this rule bypass, so everyone can ignore the branch protection rules. Maybe we should remove this bypass? I don't see the need for it personally and repo admins can circumvent it anyway.

Image Image

WillOnGit commented 3 months ago

Nudging @cdata, see above.

wfaithfull commented 2 months ago

@jsantell @anotherjesse @cdata I think we probably want to remove this bypass for the whole team so that we can't just merge before checks pass.