This is a meta task for our organizations specific needs for OWNERS files. There are other tasks which are related but this is supposed to reflect our exact root cause and need.
We have a notion of an OWNERS file. This file appears in some, but not all directories. It controls who is allowed to ship code under a given directory. The rules for shipping are fairly simple but writing them out makes them seem more complex than they really are. They are functionally similar to other notions of OWNERS (see https://www.chromium.org/developers/owners-files)
Owners are defined as:
- The owners for a given directory are the innermost OWNERS file on that tree.
- Alternatively an "ancestral" owner may act as an OWNER. That is if /a/b/c is being modified the owners for /a/b, /a, and / each may act as a shipper.
- Owners are always derived from master and not from the branch being submitted.
Note that ancestral owners are not generally considered primary owners and should see most reviews unless they are a direct owner.
For a given change:
- For each file modified at least one OWNER for that given file must accept. Directories are considered files in the directory in which they reside.
- At least one non-submitter must accept.
Since we switched to phabricator we additionally have the rule:
- no one may be "requesting changes" for the revision.
We have a script which creates an Owners package for each directory with an OWNERS file. Note that the packages are owned by People and not Projects since scripting the addition/modification/addition of projects is painful right now.
However we are missing a number of features
|T731||We have no way to describe these rules to phabricator|
|T9372||Once described, the dashboard does not reflect the current status|
|T8887||Owners packages can not automatically add reviewers|
|T10181||We have no way to audit if commits made it without review (what we called TBR)|
|T10174||This is just annoying but causes no functional issue|
|T10622||We've had to build some logic into arcanist as a result of the above missing features. This leads to ALL owners being on the review and thus adds diffusion of responsibility|
|T1279||The above logic has no way to add "blocking" reviewers (and thus makes it harder to emulate T731)|
|T4631||Warnings raised by Arcanist are incorrect once Owners are added|
I may have missed some tasks or logic in the above description but we'll edit it to amend as required.
Another issue that I can't find an existing task for already is that OWNERS are non-hierarchical. That is, if one has defined an OWNERS package for /a/ and /a/b/ there is no way to assert that /a/ shouldn't audit if /a/b/ accepted and vice-versa. In addition the yellow highlighting for anyone in e.g., the root owners file, will be "wrong" as they are non-direct owners. Ideally it would be good if we had separate coloring/icon for direct/indirect ownership (so you know what you're shipping for and what files you truly own).
It might be a good idea to separate out T6161 or "accept for this package" or "accept for this project" style accepts for full fledged shipits. This would solve other issues with root owners accepting on behalf of everyone accidentally).
The above tasks are roughly ordered by priority for us, with the top two (T731 and T9372) standing out. Among other things, it would be a big win by giving us an actionable dashboard. It would be great to get an idea of the cost and timeline for at least these two tasks.