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

brew bump* commands should enable auto-merge #14538

Closed
1 task done
MikeMcQuaid opened this issue Feb 7, 2023 · 21 comments
Closed
1 task done

brew bump* commands should enable auto-merge #14538

MikeMcQuaid opened this issue Feb 7, 2023 · 21 comments
Assignees
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age

Comments

@MikeMcQuaid
Copy link
Member

Verification

Provide a detailed description of the proposed feature

This could be either conditional through e.g. --automerge or through an environment variable such as HOMEBREW_BUMP_AUTO_MERGE.

This requires write access to enable so will be limited to maintainers.

Once enabled this will merge when the branch protection requirements are met i.e. a ✅ and, on most repositories, passing CI.

This is possible when using GitHub's Pull's API and setting the auto_merge attribute.

What is the motivation for the feature?

Save maintainers time and clicks when reviewing PRs from other prolific maintainers.

How will the feature be relevant to at least 90% of Homebrew users?

Maintainers with more time will do more things users like

What alternatives to the feature have been considered?

Use an auto-merge action to enable this on all PRs: https://github.com/marketplace/actions/enable-pull-request-automerge

@MikeMcQuaid MikeMcQuaid added help wanted We want help addressing this features New features labels Feb 7, 2023
@MikeMcQuaid
Copy link
Member Author

https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions#enable-auto-merge-on-a-pull-request could do this

I note that we can't really do auto-merge properly on homebrew-core as-is, though, because we don't have required CI jobs.

@reitermarkus
Copy link
Member

Auto-merge is now automatically enabled for maintainer version bumps in homebrew/cask, see Homebrew/homebrew-cask#142896.

The same can be done when it is possible to have a required check in homebrew/core.

@MikeMcQuaid
Copy link
Member Author

Thanks @reitermarkus.

@Homebrew/core what would you think about having required builds again that requires you to do a manual override (i.e. click the merge button more than once) to merge with the merge button? I would give you all Maintain access to make this possible.

@dawidd6
Copy link
Member

dawidd6 commented Mar 13, 2023

But we mainly don't use the merge button in homebrew/core? We still need to pull the bottles somehow. Are we going to change our workflows? Sorry, I'm a bit lost with this thread 😅.

@SMillerDev
Copy link
Member

Same, I'm not sure how we could do this in core.

@ZhongRuoyu
Copy link
Member

ZhongRuoyu commented Mar 13, 2023

Also, there may occasionally be pre-releases that are not ready for widespread distribution. If we figure out how to handle bottles (not sure about this either, sorry), we should also figure out how to deal with pre-releases. Maybe, to be more conservative, only enable it for the ones that we can make sure are marked as latest.

Update: I figured that I might have misunderstood how this mechanism works. Since review is still required: I guess pre-releases are not a concern. But if I understand this correctly now, I'm not sure if that would make much difference, since it's usually @BrewTestBot that handles the merge hourly.

@MikeMcQuaid
Copy link
Member Author

@dawidd6 @SMillerDev @ZhongRuoyu Sorry, to clarify:

  • I'm not proposing (yet/here) to change the homebrew/core workflow
  • You are right that we have a different workflow that generally does not support auto-merge and that that's different enough to mean we'd need more changes than what @reitermarkus suggests
  • I still think it's worth having "required builds" for everyone except @BrewTestBot with an admin override so that we can use native auto-merge sometimes (rather than never today because it ignores CI)
  • I think we should make whatever the existing "auto-merge and pull bottles" workflow is for homebrew/core do the described behaviour here only for maintainers i.e. once you have a ✅ from another maintainer: bottles will be pulled/pushed automatically
  • This may already be the existing functionality here, excuse me if so 😅

@MikeMcQuaid
Copy link
Member Author

Somewhat unrelated to this thread:

  • I want to adjust brew test-bot and the various workflows to push to PRs and then we can use native GitHub auto-merge behaviour and have a similar flow between homebrew/core and homebrew/cask

@MikeMcQuaid
Copy link
Member Author

The ideal end flow for Homebrew/core and Homebrew/cask bumps should be:

  • a maintainer submits a bump
  • this enables auto-merge when CI is 🟢 and another maintainer has done a ✅
  • another maintainer does a ✅
  • GitHub's native auto-merge merges the PR

@reitermarkus
Copy link
Member

  • this enables auto-merge when CI is 🟢 and another maintainer has done a ✅
  • another maintainer does a ✅

So you want to auto-merge when two maintainers have approved a PR?

@MikeMcQuaid
Copy link
Member Author

@reitermarkus Every homebrew-core or homebrew-cask PR should have at least one person, other than the person who created the PR, giving a ✅ before it is merged. If it's auto-bumped by @BrewTestBot: only one maintainer needs to ✅ it. If it's bumped by a maintainer: another maintainer should ✅ it other than the one who created it.

Make sense?

@SMillerDev
Copy link
Member

You can't ✅ your own PR though, so GitHub sets those rules for us already if we require one ✅ .

@reitermarkus
Copy link
Member

Okay, so exactly like homebrew/cask works right now:

  1. Maintainer submits a version bump.
  2. Enable auto-merge.
  3. Another maintainer approves the PR.
  4. GitHub auto-merges once CI passes.

For homebrew/core we would need the “pull and upload bottles” step somewhere between 3. and 4. though.

@dawidd6
Copy link
Member

dawidd6 commented Mar 14, 2023

That's where merge queue comes in, I suppose. The PR goes into the merge queue where we run our workflow that uploads the bottle and pushes the bottle commit back to PR's branch or straight to master? I'm still a bit confused how this would exactly work.

@MikeMcQuaid
Copy link
Member Author

The PR goes into the merge queue where we run our workflow that uploads the bottle and pushes the bottle commit back to PR's branch or straight to master? I'm still a bit confused how this would exactly work.

I think if/when we land Homebrew/homebrew-core#125556: we should be able to do this without the need for the merge queue.

@reitermarkus
Copy link
Member

Pushing to the PR branch has two issues:

  • Required approvals are dismissed.
  • Not all contributors allow editing their PR.

@MikeMcQuaid
Copy link
Member Author

  • Required approvals are dismissed.

@reitermarkus Easily dealt with: can just disable this.

  • Not all contributors allow editing their PR.

We will make this a required part of contributing to Homebrew.

@reitermarkus
Copy link
Member

I'd argue disabling required approvals is more of a workaround than a true solution. Maybe we can have BrewTestBot re-approve the PR once the bottles are pushed.

@carlocab
Copy link
Member

  • Not all contributors allow editing their PR.

We will make this a required part of contributing to Homebrew.

This may be difficult for PRs from org forks. Just an FYI -- we can decide that we don't care, but I think it's important that this is made explicit.

@MikeMcQuaid
Copy link
Member Author

I'd argue disabling required approvals is more of a workaround than a true solution. Maybe we can have BrewTestBot re-approve the PR once the bottles are pushed.

Could do but I think I'd ideally like to get to a point where BrewTestBot has far reduced permissions for this stuff. Personally, I'm not terribly convinced that bots should be able to do things like this (long-term).

This may be difficult for PRs from org forks. Just an FYI -- we can decide that we don't care, but I think it's important that this is made explicit.

@carlocab Yeh. We'll need a flow for figuring out how to handle this when it "doesn't work" e.g. a maintainer manually intervenes. Similarly, we may want to consider adding a check for this (somehow?) before PRs are ready to merge so we don't waste time on a branch that will need moved.

@MikeMcQuaid MikeMcQuaid self-assigned this Jul 28, 2023
@MikeMcQuaid MikeMcQuaid added in progress Maintainers are working on this and removed help wanted We want help addressing this labels Jul 28, 2023
@MikeMcQuaid
Copy link
Member Author

No longed needed by homebrew/core or homebrew/cask. See discussion in #15784

@github-actions github-actions bot added the outdated PR was locked due to age label Aug 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants