- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 6.2k
 
Add block on pending codeowner reviews branch protection #34995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
7ff6bd4    to
    7141ade      
    Compare
  
    39ba543    to
    18cce17      
    Compare
  
    a5035a7    to
    5df3166      
    Compare
  
    | 
               | 
          ||
| hasApprovals := true | ||
| 
               | 
          ||
| for _, rule := range rules { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic appears overly complex and doesn’t seem correct. I ran a test, and it didn’t work as expected. Could you please rewrite it to make it clearer and more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran a test, and it didn’t work as expected.
Can you give me more info about that, like the codeowner file you used and what you expected vs. what happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also went and amended it to be a bit more concise. The behavior should be unchanged though, so I'd still be interested in your findings.
b448f2c    to
    e01ac3d      
    Compare
  
    | 
           Relevant PR for gitea docs, for if/when this is merged: https://gitea.com/gitea/docs/pulls/245  | 
    
d1f75f3    to
    1201827      
    Compare
  
    1201827    to
    7f9d373      
    Compare
  
    7f9d373    to
    7cb6808      
    Compare
  
    | 
               | 
          ||
| pr.Issue.Repo = pr.BaseRepo | ||
| 
               | 
          ||
| if pr.BaseRepo.IsFork { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may conflict with the UI for protected branches in forked repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with the same logic as here:
Which ensures code owners don't get review requests in forked repositories. I think it makes sense to do something similar here? That is, don't require reviews from code owners who potentially have nothing to do with your forked repo.
ce73de0    to
    e40c846      
    Compare
  
    a45a220    to
    a25ec77      
    Compare
  
    a25ec77    to
    f8cba0b      
    Compare
  
    
This commit introduces a new branch protection rule that allows merge blocking if there are pending reviews from one or more code owners (as defined in any valid
CODEOWNERSfile). This is determined by evaluating each rule present in theCODEOWNERSfile individually. For every rule, at least one named code owner (or member of a code owner team) must have given an approving review for merging to be possible.Closes #32602
This PR does NOT display code owners separately from other reviewers (#28137), as I think it makes sense to do that in a separate PR, as it doesn't require any database migrations.
Screenshot
Pull Request
Branch Protection Rule
Unresolved questions
Does this warrant a doc change?Relevant PR: https://gitea.com/gitea/docs/pulls/245