Help
cancel
Showing results for 
Search instead for 
Did you mean: 
Highlighted
Pilot Lvl 1
Message 1 of 9

Code Review Problems

Solved! Go to Solution.

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

 

8 Replies
Moderator
Message 2 of 9

Re: Code Review Problems

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!

 

 

Mark helpful posts with Accept as Solution to help other users locate important info. Don't forget to give Kudos for great content!

Copilot Lvl 2
Message 3 of 9

Re: Code Review Problems


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?

 

Moderator
Message 4 of 9

Re: Code Review Problems

@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.

Mark helpful posts with Accept as Solution to help other users locate important info. Don't forget to give Kudos for great content!

Copilot Lvl 2
Message 5 of 9

Re: Code Review Problems

@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.

Solution
Moderator
Message 6 of 9

Re: Code Review Problems

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!

Mark helpful posts with Accept as Solution to help other users locate important info. Don't forget to give Kudos for great content!

Ground Controller Lvl 1
Message 7 of 9

Re: Code Review Problems

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.

Ground Controller Lvl 2
Message 8 of 9

Re: Code Review Problems

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

Copilot Lvl 2
Message 9 of 9

Re: Code Review Problems

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.