Code Review Problems

I’m having trouble pinning down the exact steps for us to do this correctly.

We want to have all PRs from a project_branch in people’s forks to the same project_branch in the main repo (these are private organization repos) to be forcibly subject to reviews.

In order to facilitate this I added a branch protection rule to all project branches in the main repo and in that rule specified we need 2 approvals before merging is allowed.

However when we do this we get a problem, imagine we’re now done with project_branch in the main repo so we create a PR to merge that into master. Well when we do this GitHub ordinarily tells us we can now delete project_branch but now it does not and in fact it tells us that the branch cannot be deleted!

It seems that it isn’t possible to apply this two approval rule to project branches without also making the branch not deleteable but this seems dumb.

Furthermore we see weird stuff when we approve, we expected (after two approvals) that GitHub would automatically merge the PR but it didn’t and in fact at the top of the PR page (top right) we see clearly that three people approved the PR yet at the bottom of the same page it says only a single person has approved.

This is just driving us insane trying to fathom this out.

Thx

1 Like

Hi @korporal,

Thanks for being part of the GitHub Community Forum! I’ll do all I can to help.

To address your question about not being able to delete that branch, this is expected behavior per the documentation on branch protections

> When you create a protected branch rule in a repository, collaborators cannot force push to the protected branch or delete the branch by default.

That said, a repo admin should still be able to delete that branch (unless, when setting up the branch protections, “Include administrators” was checked). If you want repo admins to be able to delete branches (or otherwise circumvent the branch protections), you’ll want to make sure that checkbox is left unchecked.

As for some of the other things you describe:

>…we expected (after two approvals) that GitHub would automatically merge the PR…

GitHub does not automatically merge PRs this way. A protected branch with required approvals will be unmergable until the approvals have occurred; however, collecting the required approvals only makes the PR mergable - it does not merge the PR automatically. This is expected behavior.

>…at the top of the PR page (top right) we see clearly that three people approved the PR yet at the bottom of the same page it says only a single person has approved…

This could be for a couple of reasons. If anyone outside of your organization approves the PR, those reviews wouldn’t count as “approvals” for the count at the bottom. Also, if you’re using CODEOWNERS or something else that restricts who can actually approve merges, the count at the bottom will also show a different number if just a member of the org (but not a CODEOWNER) approves the PR.

I hope this helps. Please let us know if you have more questions!

2 Likes

That said, a repo admin should still be able to delete that branch (unless, when setting up the branch protections, “Include administrators” was checked). If you want repo admins to be able to delete branches (or otherwise circumvent the branch protections), you’ll want to make sure that checkbox is left unchecked.

It doesn’t work, still having the same rejection.

GitHub UI:

You can't delete a protected branch.

Force push:

remote: error: GH006: Protected branch update failed for refs/heads/**.
remote: error: Cannot delete a protected branch
To https://github.com/ **/**.git
 ! [remote rejected] ** (protected branch hook declined)
error: failed to push some refs to 'https://github.com/ **/**.git'

> …enforce certain workflows or requirements…

How I understand, that certain workflows or requirements only available via pull requests. It’s that correct, @nadiajoyce?

@grabauskas Are you certain that “Include administrators” isn’t checked when setting up branch protections? If it is, that would prevent even admins from deleting a protected branch.

@nadiajoyce I’m sure, that “Include administrators” checkbox was not checked.

It’s simple to test, just create a new branch protection rule and go to https://github.com/{OWNER}/{REPO}/branches link. You will see, that you can’t delete protected branches.

Hi @grabauskas thank you for following up on this. Right you are! Sorry for any confusion. I talked with my teammates, and it’s true: protected branches cannot be deleted, even by admins. The “even admins” checkbox is more for review restrictions.

In order to delete a protected branch, you’d have to first remove the restrictions from it. This is because protected branches are meant to be more permanent, akin to master. That said, I know the OP here was asking for the ability to delete protected branches, so I will log that internally as a feature request.

Cheers!

10 Likes

One more vote for admins to be able to delete protected branches in sudo mode.  I understand that protected branches are ‘supposed to be’ long-lived like master, but the feature is more useful than just long-lived branches like master.

Our use-case:

My team’s workflow is that most work is done in short-lived feature branches by singly authors, but sometimes a long-lived feature branch being worked on by many authors is required.  We protect these long lived branches because for the force-push prevention.  We do eventually merge these branches into master and then we want to get rid of the feature branch, but we’re forced to do gymnastics and temporarily unprotect our other branches to delete it.

3 Likes

Hi, I’d like this feature request to be more extensive.  I too want to enforce that pull requests are reviewed by at least one person before being merged.  So I put that protection on all branches so this is enforced for all feature branches created over time.  However, we need to be able to remove those feature branches once they’re merged otherwise the repository is going to be a mess and using other tools to interact with branches is going to show hundreds to thousands of branches which is highly undersable.  Futhermore, if I’m the creator and thus the owner of a branch, I should be able to delete it no matter what.  Currently I find the branch protections are far too obstructive to basic operations that have nothing to do with the rules in the branch protections.  Thanks.

1 Like

Hey, we are using a similar workflow, when a lot of work is needed to shop a feature, we usually split the work and we have a hierarchy User Story (US) > Feature (FT) > Ticket (TK). Our US and FT are product issues and our TK issues are more tech-oriented

We spawn one branch per US so that we can isolate all the work that must be done to ship a new end-user feature. So we may have several branches like us/42-a-user-can-report-bad-behavior that can be used my multiple devs ==> we add a protection to this branch.

Each dev will then make some individual branches tk/111-open-report-behavior-popup that will be merged into this us/ branch. Then, once all the work is done, the Q/A team can do its job and once the US is validated, then we open a PR from the US branch into master, and after a successul build we merge this PR and start a release using Vincent Driessen git-flow.

At this stage, the US branch SHOULD disappear, but since we have protected it, we can’t do anything :’(. It would solve the problem to have a “sudo” like mode for git that would allow some people to force-remove branches (eg only a Tech lead cna merge the US into master and close this branch)

(I’m talking about the master branch, but understand the develop branch if referring to Vincent Driessen git flow

p=nosuchUser19890807