4975: `rbt post` should infer default reviewers from Google/Gerrit-style OWNERS, GitLab-/GitHub-style CODEOWNERS files

beasleyr-vmw 
What version are you running?


Review Board 3.0.18

RBTools 2.0 (Python 3.6.8)


Describe the enhancement and the motivation for it.


Enhancement


rbt post optionally takes over the responsibility of determining which users/groups a review request should be sent to for a given change.  The mapping of source files to users/groups can be configured using one or more of the following de facto standards.


    

  • GitHub CODEOWNERS link
    • 

  • GitLab CODEOWNERS link
    • 

  • Google/Gerrit OWNERS link
    • 



I suggest implementing support for all 3 types.  RBTools can either probe to determine which style is in use for a given repo, or it can let users choose which style applies their repo by setting the appropriate flag in .reviewboardrc.


Motivation


Defining default reviewers in large repositories is very difficult, because Review Board's default reviewers configuration is based on regular expressions that require superuser privileges to edit.  This prevents users from fine-tuning default reviewer configurations, because (a) the desired behavior needs to somehow be able to be captured by one or more regexps, and (b) each tweak to the set of regexps requires a support ticket.  Using OWNERS/CODEOWNERS would push this configuration out from the RB server to the actual project repository where changes in ownership can be managed by end developers instead of an admin.


Please provide any additional information below.


I filed this under RBTools (as opposed to Review Board) for two reasons:

1. rbt post already supports --target-groups and --target-people, so I figured it'd make sense for rbt post to infer appropriate values based on OWNERS files.

2. Users can upgrade RBTools at their convenience, and so folks can easily adopt this feature w/o waiting for central admins to upgrade central infra first.


I may be able to allocate cycles to contribute patches for this.
chipx86
#1 chipx86 
Hey Ryan.


Interesting idea. We'll have to mull this one over.


Review Board is probably still where we'd prefer to have default reviewer logic live, because:


    

  1. Not everyone uses RBTools (there are a lot of in-house clients and integrations out there using our API)
    2. 

  2. Not everyone upgrades (as an example, your RBTools 2.0 is a major version behind, soon two major versions)
    3. 

  3. Having default reviewer logic live both client-side and server-side is messy (and we don't want it to live fully client-side -- we have customer who require complete server-side control of reviewers)
    4. 



We've had a long-standing goal to change up how default reviewers work. Right now they're regex-based, as you noted. The plan though is to let admins define rules using our Conditions support (which we use for service integrations). These would allow for anything from regex rules to "look up default reviewers from this file/URL" to "ask this extension/webhook for reviewers".


With that in place, an admin could define a rule that allows review requests posted against one set of repos to use CODEOWNERS while review requests against another set would have very strict default reviewers.


That'd have the additional benefits of:


    

  1. Keeping default reviewer management centralized, working whether RBTools is used (or whether people are even running latest versions -- yours is a major version behind, for example)
    2. 

  2. Being able to always use the latest official source for a CODEOWNERS (people may be working locally in older branches that aren't up-to-date)
    3. 

  3. Letting the CODEOWNERS inclusion be part of a ruleset
    4. 

  4. Treating CODEOWNERS as just one possible source (we have customers today who use other sources and use hacks to tie into default reviewers)
    5. 



We have some internal tracking around this work. I'm including this as part of that task.
#2 beasleyr-vmw 






We've had a long-standing goal to change up how default reviewers work. Right now they're regex-based, as you noted. The plan though is to let admins define rules using our Conditions support (which we use for service integrations). These would allow for anything from regex rules to "look up default reviewers from this file/URL" to "ask this extension/webhook for reviewers".




This is very encouraging news. I look forward to seeing it appear in the final product. :)


With that in mind, I believe we can close/resolve this public ticket.  (I'm not sure if users can resolve tickets or if that's limited to Splat/project admins.)