ideas icon indicating copy to clipboard operation
ideas copied to clipboard

Merge pull requests when all status checks are passing

Open zeke opened this issue 7 years ago • 48 comments

For some projects (with really good test coverage?) it would be cool to have a bot that automatically merges any PR that has passing status checks, e.g. tests passing, reviews approved, etc.

If this existed, we would probably use it on the Electron website: https://github.com/electron/electronjs.org/issues/1046

zeke avatar Jan 25 '18 19:01 zeke

@gr2m suggested that there might currently be some roadblocks for such a bot: https://platform.github.community/t/repositories-which-have-protected-branches-with-push-restrictions-have-no-ability-to-grant-push-rights-to-integrations/1376

zeke avatar Jan 25 '18 19:01 zeke

we're thinking about doing something similar for Angular, we already have a probot app that checks if the PR is good to merge, but it doesn't do the merge itself yet

ocombe avatar Feb 12 '18 17:02 ocombe

Hey @ocombe is that bot source public?

zeke avatar Feb 12 '18 19:02 zeke

yes! it's here: https://github.com/angular/github-robot we're going to add some new PR triage functionalities soon as well, see https://github.com/angular/angular/issues/21884 for a first draft of what we intend to do

GitHub
Contribute to github-robot development by creating an account on GitHub.

ocombe avatar Feb 12 '18 19:02 ocombe

Good stuff. Thanks for sharing, @ocombe.

zeke avatar Feb 13 '18 04:02 zeke

I seen https://github.com/angular/angular/issues/21884. I know to work with github api's but no working experience for bot in github.

mritunjayz avatar Feb 14 '18 15:02 mritunjayz

I would like to work on this any suggestions on how the bot should behave if the mentioned issue where bots not being able to merge pull requests with protected branches

heshanpadmasiri avatar Feb 15 '18 01:02 heshanpadmasiri

So I went through this https://platform.github.community/t/repositories-which-have-protected-branches-with-push-restrictions-have-no-ability-to-grant-push-rights-to-integrations/1376. And it doesn't look like GitHub has released a fix yet. Further AFAIK there is no realistic workaround for this? Can anyone give some idea so as to how we might go about this?

rsarky avatar Feb 26 '18 07:02 rsarky

I don't think that an app should be able to bypass protected branches. Those are typically very sensitive branches where things would break if merged incorrectly, and I think that folks would be upset to see an app ignore their intentional decision to treat that branch more carefully.

JasonEtco avatar Feb 26 '18 20:02 JasonEtco

Protected branches refer to branches with some restriction on who can push to the branch. Isn't that the case for most branches?

rsarky avatar Feb 27 '18 01:02 rsarky

@JasonEtco it’s not about working around protected branches, it's about automated merging once all the checks are green

gr2m avatar Feb 27 '18 19:02 gr2m

it’s not about working around protected branches, it's about automated merging once all the checks are green

I see - I guess it depends on the level of protection (which is ultimately up to the user). One available option is the only allow merges with at least 1 green review from someone with write access; I think that as long as that protection isn't ignored then its all good. I only bring it up because I'm not sure if that's considered a "status."

JasonEtco avatar Feb 27 '18 20:02 JasonEtco

Sadly the GitHub API doesn't support GitHub apps merging pull requests with protected branches yet. @JasonEtco

rsarky avatar Feb 28 '18 12:02 rsarky

GitHub API doesn't support GitHub apps merging pull requests with protected branches yet.

I found the internal GitHub issue for this and gave it a little bump. It's a known issue, GitHubbers are aware that integrators are frustrated by it, and a solution is actively being discussed. I'll report back here with any updates.

zeke avatar Mar 01 '18 01:03 zeke

@zeke That's awesome.Thanks

rsarky avatar Mar 01 '18 06:03 rsarky

@zeke I would like to work on this. I have completed hello-world bot. What's the next step?

pranay414 avatar Mar 16 '18 15:03 pranay414

@pranay414 GitHub folks are actively working on a fix for the bot permissions problem, but it hasn't shipped yet. I will post here when that happens.

zeke avatar Mar 16 '18 16:03 zeke

@zeke so should I start drafting a proposal for this?

pranay414 avatar Mar 16 '18 16:03 pranay414

@pranay414 sure. If you want to outline the expected behavior that could be helpful.

zeke avatar Mar 16 '18 16:03 zeke

@zeke when someone submits a PR then it is put into testing and once all checks are passed then it should be merged or it should be reviewed by someone and then merged?

pranay414 avatar Mar 16 '18 20:03 pranay414

@pranay414 One of the status checks will probably be getting the PR reviewed.

rsarky avatar Mar 16 '18 20:03 rsarky

@rsarky so once it is reviewed then it should be merged right?

pranay414 avatar Mar 16 '18 20:03 pranay414

@pranay414 Once all the status checks pass. The status checks may include CI, reviews etc.

rsarky avatar Mar 16 '18 20:03 rsarky

Ok got it, Thanks :)

pranay414 avatar Mar 16 '18 20:03 pranay414

@gr2m @JasonEtco I would like to consider this for my GSoC proposal. Can you please guide me how do I proceed?

jainkeval avatar Mar 19 '18 10:03 jainkeval

@gr2m @JasonEtco . I have given this task a try and it works for ci-validated and mergeable PRs. Please find my repository here gsoc_probot_task_1

vishalseshagiri avatar Mar 26 '18 01:03 vishalseshagiri

@zeke A new API endpoint is now available and it is perfect for this use case :tada: Merge PR

rsarky avatar Apr 18 '18 03:04 rsarky

@rsarky you beat me to it! Thanks to @kytrinyx for helping move this forward.

https://twitter.com/kytrinyx/status/986409138568810496

zeke avatar Apr 18 '18 04:04 zeke

i'm not sure how helpful any of this is to this thread, but i figure its related enough to at least mention. i maintain greenkeeper-keeper that serves a very similar purpose, but is purposely limited to PRs that are from greenkeeper. the current implementation is a hapi plugin, but a probot version is on my list even though i haven't found time to make that happen yet. i'm very interested in where this goes as i could see potentially replacing greenkeeper-keeper with this.

one thing that might be worth considering is the ability to limit the automatic merge to a certain type of PR. the important detail for greenkeeper-keeper is that it is limited to only greenkeeper PRs. they are only dependency upgrades, so there isnt anything to review beyond passing commit checks. contributions from humans usually are more more worth a human review, so my team likes to handle those differently.

travi avatar Apr 19 '18 02:04 travi

I almost never want to merge pull requests unseen as soon as they pass—it's almost always a thing where I've reviewed it, and want to merge it, but the build is still running, so I go look at instagram and check my email and completely forget what I was actually doing. That's when I would want to have a way to signal to a bot: go ahead and merge this as soon as it passes.

I could see this working with protected branches that have required builds and require an approval from a human reviewer. Then as soon as all the requirements are met, it could merge it.

kytrinyx avatar Apr 19 '18 02:04 kytrinyx