CODEOWNER Required Reviews

I’ve got a repository with 3 total collaborators, and the master/dev branches are set up to be:

  • protected
  • require PR reviews
  • require review from Code Owners

Now, as I understood things, my open PR towards the dev branch should request a review from all Code Owners, but a single approved review should be enough to review.

My PR automatically requested review from the other two collaborators, since both are Code Owners. So far, so good. One of them requested changes, which we discussed, made, and then approved. So the current status of his review is “approved”–yet this is not enough to merge the PR?

The status box says

Code owner review required
Waiting on code owner review from xxxxxxxxxxxx.

Required statuses must pass before mergingAll required status checks on this pull request must run successfully to enable automatic merging.

There are no status checks enabled (beyond required reviews), and I have at least one approved review, so I fail to see why the merge button didn’t turn that shiny Green (I have full privilege on the repo, so presumably I can merge when the review is approved).

Is there any way to make sure at least one approved review is enough to merge the PR, or is this one of those “since it’s your PR you can’t merge it” things?

It’s hard to say why you’re experiencing this behavior without some more information. Is the repository in question public? Can you share a link to a specific PR that is behaving in a way that you believe is not expected?

Assuming that the repository in question is private, I can guess at one reason why more than one reviewer might be required. Let’s say you have a CODEOWNERS like the following:

* @foo @bar @baz

In this case, any PR would require one (and only one) of those three people to approve it before it could be merged. But let’s say you have a more complex CODEOWNERS file:

*.js @foo @bar
docs/ @baz

And you submit a PR that includes both a JavaScript change as well as a change to some associated documentation. That PR would require one of either foo or bar to approve it for the JavaScript change and a separate approval from baz for the documentation.

Maybe you could share just your CODEOWNERS file?

6 Likes

I figured it out; it was similar to what you describe below, where my changes were targeting two different patterns, requiring a review from two separate people to approve each change.

Thanks for your help!

2 Likes

Elsewhere you have said you can specify code owners for specific branches. Can you please give an example?

There is an example in the CODEOWNERS help documentation. Let us know if you have any questions about it.

1 Like

hey @lee-dohm

I don’t see any code example.  I found a blurb about branches, but nothing telling us how

Each CODEOWNERS file assigns the code owners for a single branch in the repository. Thus, you can assign different code owners for different branches, such as @octo-org/codeowners-team for a code base on the master branch and @octocat for a GitHub Pages site on the gh-pages branch.

on the docs page; what does an actual branch reference look like?  would we have a new CODEOWNERS file per branch, or would we have the same CODEOWNERS file with all the branches and reviewers listed?

1 Like

It’s probably better explained by saying that the one CODEOWNERS file has different contents per branch. So if there is one rule in the file on the master branch but a different rule in the file on the foo branch, then the first rule applies when things are being merged into master and the second rule applies when things are being merged into foo. In both cases, the format of the CODEOWNERS file is the same and there is no mention of a branch reference within the contents of the CODEOWNERS file itself.

2 Likes
* @foo @bar @baz

“In this case, any PR would require one (and only one) of those three people to approve it before it could be merged”

Is there any way to require all of the listed people to approve?

1 Like

No, there currently isn’t a way to do that built-in to the CODEOWNERS feature. If that was the one and only rule, you could do a hacky solution where you required a PR to have three approvals to be merged but if the CODEOWNERS contents ever changed, you would have to remember to manually update the number of required reviews. So it isn’t something I would recommend as a solution to the question you’re asking.

1 Like

Is there a way to make it so the universal code owner can approve PRs to subdirectories that also have code owners. For example:

\* @foo  
  
/docs @bar @baz  

Let’s say someone does a PR to the docs/ directory to fix a missing comma. While I like that bar and baz get requested for reviews on the docs/ directory, it seems like if foo adds themselves as a reviewer and approves the PR then that should satisfy the “1 owner approves” rule.

I realize I can work around this by adding foo as an owner to docs/ but in a project with many owners it seems a bit inconvenient to not rely on a pattern when an owner can approve PRs to their directory and all subdirectories. Additional owners on subdirectories don’t override the ownership of the person higher up in the hierarchy—they just get added in.

I can understand the rationale behind what you’re describing. However, that isn’t how the feature is currently implemented. But even if the rules were additive as you describe, it wouldn’t only notify @bar and @baz, @foo would get notified as well. The workflow you describe appears to indicate that @foo wouldn’t get notified but would still count as an owner if they review the PR.

For workflows customized to your particular development team, I would recommend investigating Probot or GitHub Actions to see if either of them can help you build a solution to fit your team’s needs.

So how do you avoid the PR merge from one branch to another overwrite the CODEOWNERS file with different content?

update the .gitignore file in all branches to include CODEOWNERS

and from there you can specify different codeowners that won’t overwrite on merge

Bumping this: @lee-dohm 

I’m not sure why you’re bumping this @logankilpatrick.

Hey! Sorry, it does not look like my bump was in the right place, but the final question on the thread was whether and how you could set it up such that you require multiple code owners to approve the PR.  Thanks! 

My post on 28 November, 2019 is in response to the question about how to require multiple owners to review a PR before it could be merged.

As I stated in my post above, if the current CODEOWNERS implementation isn’t what you’re looking for, then I’d suggest investigating Probot or GitHub Actions to see if they’ll allow you to model the workflow you do want.

I hope that helps!

A related question @lee-dohm: what happens if I require more PR approvals than there are code owners?

Our use case: we want to require code owner review on a specific file. e.g CODEOWNERS file:

some-important-file.js @foo @bar @baz

From my understanding, these 3 users would be codeowners for that file but if a PR doesn’t touch that file, there are 0 code owners.

In this situation, is Github’s logic smart enough to not require approvals on a PR that only touches files without any owners? Or would every PR still require 3 approvals?
Is there any way to achieve my desired outcome: to only require PR approvals on PRs that touch a specific file?

Thanks in advance!

Great question @samajammin :+1:

Requiring a specific number of reviews means that every PR must have that number of reviews before it can be merged. The logic will not forego requiring a PR review on PRs that have no files covered by CODEOWNERS.

As I mentioned above, the general solution to having any sort of custom check logic that allows or prevents merging PRs is to use GitHub Actions or some similar system like Probot.

1 Like