Make ghstack land work with protected master branch

[ezyang]

ghstack land directly pushes to master, which means that if master is protected, you can't use it. It would be nice to figure out some way to work around this.

One silly way to do it is to open a NEW pr that is based on master, and then use GitHub API to merge it. However, if you have mandatory checks on a project, you have to wait for all the checks to finish before you can merge. Another possibility is to get a bot with push to protected rights and teach ghstack to ask that bot to do the merge.

cc @fmassa

[ailzhang]

Hey Ed! What's the latest workaround for this issue? Is opening up master branch still the only way to do it? :P

[ezyang]

Nope, no new ideas. Do you have mandatory checks? If you don't, opening a new PR and immediately merging it should work pretty good.

[ezyang]

My new idea is you can enable force pushes on the protected branch

and I am going to make ghstack land force push

[ezyang]

Another idea is we can create a bot integration that knows how to land ghstack PRs, and then let people install it to their repos.

[ailzhang]

Another idea is we can create a bot integration that knows how to land ghstack PRs, and then let people install it to their repos.

+1 on this! Opening up force pushes to everyone is kinda risky so delegating that to a bot account seems much more reasonable.
(In fact I did allow force pushes from myself so that I was the only one who can land ghstack PRs in taichi team but that made me effectively a landing bot haha :P

[aspiers]

Totally agree - opening up force pushes for everyone is absolutely not going to work for very many projects who need tighter security than that. Also creating a separate PR just for merging would create an unholy mess of duplicate / overlapping PRs, so I don't think that's a good option either.

I would suggest finding one or more great existing mergebots (i.e. ones which support flexible policies around what is required in terms of approvals, CI passing etc. in order to allow a merge) and integrate the landing process with those.

[ailzhang]

For anyone who're interested in using ghstack in an open source project, you may find @feisuzhu's land bot command which limits landing from repo writers helpful. It also performs a few additional checks before landing, e.g.

• all github action jobs must pass
• PRs(including all PRs below) must be approved

This allows us to use ghstack project wide without granting everyone admin access, cheers!

[smeenai]

I've been evaluating ghstack for use in LLVM, which is switching from Phabricator to GitHub PRs and looking for a solution for stacked PRs. Requiring force pushes is a deal-breaker, unfortunately, since it's way too easy for bad things to happen. LLVM allows non-force pushes without going through a PR though though; could ghstack just do a regular push in that scenario instead of requiring a force push?

[smeenai]

I've been evaluating ghstack for use in LLVM, which is switching from Phabricator to GitHub PRs and looking for a solution for stacked PRs. Requiring force pushes is a deal-breaker, unfortunately, since it's way too easy for bad things to happen. LLVM allows non-force pushes without going through a PR though though; could ghstack just do a regular push in that scenario instead of requiring a force push?

Actually, I think I misread the code here ... it only tries to force push if there's branch protections, so it doesn't need force pushing in all cases. In this case LLVM has some branch protections set up, but I also don't think non-admins have access to the branch protection info, so it might just end up working out?

[ezyang]

The force push only happens if you call ghstack land but if you use a merge action like Ailing posted about above you don't expect to ever use ghstack land at all (in fact I should probably just delete this functionality lol.)

[smeenai]

The merge action would need to be set up for the repository though, right? Also, if I'm understanding taichi-dev/taichi@037db43 correctly, it's still using ghstack land under the hood?

[ezyang]

Yes. And I guess it is most straightforward to use ghstack land to do the actual merge.

[felix-seifert]

Today, I wanted to try out ghstack and also experienced that master cannot be protected, i.e. it has to allow force pushes. In slightly bigger orgs, I cannot imagine allowing force pushes to master.

You described it to be a problem that a merge of a newly created PR has to wait until all status checks passed (e.g. GH Actions). Do you think that it might be an option to use the auto-merge functionality? It executes a merge automatically once all checks are passed. The flow would therefore be the following.

1. Create several commits
2. Execute ghstack to create stacked PRs
3. Land the first PR of the stack --> automatically create a PR to master with auto-merge
4. If another PR of the same stack is landed, append to previously created PR to automatically retrigger checks

[ezyang]

Having auto merge work is a sort of orthogonal problem. #122 would make it work. But with ghstack as it exists today, the problem is that even though you can turn on auto merge, all it would do is merge head into base, which is pointless because you need to cherry-pick orig onto main...

[ezyang]

I guess hypothetically, you could author a GitHub action that checks if ghstack branches are merged to their bases, and if that happens, trigger a ghstack land on main.

But it's fairly delicate. In particular, the problem is that there is no guarantee that the PR actually merges cleanly on main (in particular, the GitHub test is only checking it merges cleanly into its base branch, which by construction is always a clean merge.) So you could end up "merging" the PR but then the ghstack land fails. This would be very confusing. To make matters worse, it's impossible to reopen a "merged" PR once it is merged, so you'd have to create a new PR if this happened. Very unsatisfactory.

So I think the right thing to do is just have a separate GitHub action that does automerge with ghstack land via a label or something and give up the GitHub UI concept (or fix #122 ).