Support for OWNERS files
Open, Needs TriagePublic

Description

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)

NOTE: the submitter is considered an accept for these rules (this matters if they are an OWNER but the acceptors are not)

Owners are defined as:

  1. The owners for a given directory are the innermost OWNERS file on that tree.
  2. 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.
  3. 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:

  1. 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.
  2. At least one non-submitter must accept.

Since we switched to phabricator we additionally have the rule:

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

TaskIssueStatus
T731We have no way to describe these rules to phabricator
T9372Once described, the dashboard does not reflect the current status
T8887Owners packages can not automatically add reviewers
T10181We have no way to audit if commits made it without review (what we called TBR)
T10174This is just annoying but causes no functional issue
T10622We'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
T1279The above logic has no way to add "blocking" reviewers (and thus makes it harder to emulate T731)
T4631Warnings 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.

Details

Differential Revisions
D15926: Show reviewer status and authority on revision list view
Commits
D16139 / rARC2374403e8f80: Remove the "you have not specified reviewers" prompt from the `arc` client
D15951 / rP45718268a96e: Restore viewer() function to "Responsible Users" tokenizer in Differential
D15950 / rP7ae33d14ec7b: Use new Differential bucketing logic on default (non-dashboard) homepage
rP3aed39b8b072: Fix an issue with serializing reviewers over the wire
D15940 / rPde1a30efc740: Improve audit behavior for "uninteresting" auditors
D15939 / rP9c24798e64c9: Update Owners auditing rules for multiple reviewers
D15936 / rP809c7bf996af: Allow users to manage package dominion rules
D15935 / rP6cb2bde48d41: Add "Dominion" rules for Owners Packages
D15934 / rP29a060d7f13f: Allow blocking reviewers to be added via the CLI
D15933 / rPafec01129a09: Allow blocking reviewers to be added via the web UI
D15930 / rPbf5437212cc8: When a revision is accepted but has open dependencies, show a note in the…
D15929 / rPc5853b4f4820: Put revisions you're a reviewer on which need review and which you've…
D15928 / rP6e9828c743a7: Fix a dashboard bucketing bug
D15927 / rP8a98868bfbc4: Remove "days fresh" / "days stale" indictator in Differential revision list
D15925 / rPd46378df208f: Modernize "Responsible Users" tokenizer and add "exact(user)" token
D15924 / rP42d49be47bfc: Change Differential revision buckets to focus on "next required action"
D15923 / rPeade206625ea: Introduce search result buckets
D15921 / rP3a727c31e2f9: Modernize DifferentialRevisionSearchEngine
D15918 / rPc9365e48d8e4: Don't trigger "Auto Review" if the author is already an owner; document "Auto…
D15917 / rP92b9fa47d00d: Allow Herald to add package reviewers
D15916 / rP332d787dc8f4: Support "Review Changes" and "Block Changes" settings for Owners package…
D15915 / rP52ac242eb3b2: Implement "Auto Review" in packages with a "Subscribe" option
D15914 / rP70ddb1c45fe4: Allow packages to be added as revision reviewers via the UI
D15913 / rP4ba4cb971178: Use "fa-shopping-bag" instead of "fa-list-alt" for Owners package icon
D15912 / rP547abfe87391: Make packages mailable and subscribable
D15911 / rP3ea47d967ab7: Allow monogrammed objects to be parsed from the `arc` command line in…
D15910 / rP9abc16df4df5: Give Owners packages the "O" monogram
D15909 / rP44057ad26985: Consider packages when calculating Differential authority
D15907 / rPe5f2ccc57fb8: Don't trigger audits for archived packages

Related Objects

Mentioned In
T12272: Allow users to accept on behalf of packages they control a containing package for
T12010: Untangle the Gordian Knot of iterating on Differential/Diffusion/Arcanist
T4782: Provide short revision status summary in Differential query results?
T4631: Allow Differential to raise warnings on the server side via Conduit
T11118: Herald Rule to assign Owners to an Audit
T1279: Show per-reviewer status in Differential revision reviewer list
T10181: Create audit requests based on a package commits
D15935: Add "Dominion" rules for Owners Packages
T9372: Plan the future of "Bucketing" Query UIs
T4144: Users disagree on what "Blocking Others", "Action Required" and "Waiting on Others" mean, and/or which groups should exist
T4055: Accepted diffs with not-accepted depends-on diffs should not appear under "Action Required"
T9263: Provide ability to query for all revisions currently blocked by a specific project reviewer
T1258: Add an "Automatically CC Owners on Differential Revisions" checkbox to Owners
T5187: Make "Upload File" dialog have a vanilla file upload control
T3980: Support "Bind to External Thing" to define project membership
T10621: Create projects from external thing (extension to T3980)
Mentioned Here
T11118: Herald Rule to assign Owners to an Audit
T11010: Http502 when submitting or updating a diff
T4631: Allow Differential to raise warnings on the server side via Conduit
D15935: Add "Dominion" rules for Owners Packages
D15939: Update Owners auditing rules for multiple reviewers
D15940: Improve audit behavior for "uninteresting" auditors
T10978: Modernize Audit
T10967: Move Differential reviewers back to a dedicated database storage table
D15934: Allow blocking reviewers to be added via the CLI
D15933: Allow blocking reviewers to be added via the web UI
D15936: Allow users to manage package dominion rules
T3583: Fix the Swamp of Suck that is the home page content panel
D15921: Modernize DifferentialRevisionSearchEngine
D15923: Introduce search result buckets
D15924: Change Differential revision buckets to focus on "next required action"
D15925: Modernize "Responsible Users" tokenizer and add "exact(user)" token
D15927: Remove "days fresh" / "days stale" indictator in Differential revision list
D15926: Show reviewer status and authority on revision list view
D15928: Fix a dashboard bucketing bug
D15929: Put revisions you're a reviewer on which need review and which you've commented on in "Should Review"
D15930: When a revision is accepted but has open dependencies, show a note in the list UI
T4103: Implement "Role Profiles" to provide search, homepage and application defaults
T4055: Accepted diffs with not-accepted depends-on diffs should not appear under "Action Required"
T8080: Differential shown as "Blocking Others"
T9263: Provide ability to query for all revisions currently blocked by a specific project reviewer
T10417: Active Differentials are wrongly indicated as blocking when "request changes" is used
T10487: Reviews can return to "blocking others" even after approval
T10689: Display slightly more information about status of revision from main page.
D15909: Consider packages when calculating Differential authority
D15910: Give Owners packages the "O" monogram
D15911: Allow monogrammed objects to be parsed from the `arc` command line in "Reviewers" and similar fields
D15912: Make packages mailable and subscribable
D15913: Use "fa-shopping-bag" instead of "fa-list-alt" for Owners package icon
D15914: Allow packages to be added as revision reviewers via the UI
D15915: Implement "Auto Review" in packages with a "Subscribe" option
D15916: Support "Review Changes" and "Block Changes" settings for Owners package "Auto Review"
D15917: Allow Herald to add package reviewers
D15918: Don't trigger "Auto Review" if the author is already an owner; document "Auto Review"
T5055: Distribution mechanism for arc extensions
T9530: Release Server / Workflow app / Future of Releeph
T4144: Users disagree on what "Blocking Others", "Action Required" and "Waiting on Others" mean, and/or which groups should exist
T8426: permit owners to be bound to an "external thing" or let owners be determined by an extension
T10574: A pathway toward "All Reviewers Must Accept"
T6161: Support for "partially accepted" revisions
T731: Allow revisions to have alternate acceptance conditions
T1279: Show per-reviewer status in Differential revision reviewer list
T8887: Allow owners packages to add blocking reviewers
T9372: Plan the future of "Bucketing" Query UIs
T10174: Adding a owner package to an audit when package auditing is disable leads to a "not applicable" commit review
T10181: Create audit requests based on a package commits
T10622: Auto add reviewers based on custom logic
There are a very large number of changes, so older changes are hidden. Show Older Changes

I pushed that stuff live but bucketing has some bugs, looking into it.

This also indirectly flipped the ordering from oldest-first to newest-first. I'm inclined to retain that ordering, since it's consistent with every other application. The "oldest-first" ordering was a sort of paternalistic attempt to force users to review stuff, but I think we've generally moved the software away from that a bit and it doesn't feel very much at home to me any more.

(You can create an oldest-first dashboard yourself now if you prefer the old ordering, and can probably force it on all users after T4103 if you really want.)

Bucketing is now controlled by a "Bucket: ..." option in the search query controls instead of hardcoded as a behavior of the "Active Revisions" filter. This means that you can apply bucketing to other queries. You can also extend DifferentialRevisionResultBucket to implement your own bucketing algorithms (but: here be dragons).

In particular, you can bucket queries like "Responsible Users: SomeProject" to see bucketed revisions by a project reviewer, and "Responsible Users: exact(username)" to see bucketed revisions by individual reviewer, excluding the projects and packages they have authority over. The default ("Responsible Users: username") shows your stuff plus all the packages and projects you have authority over.

The buckets themselves are now more granular and action-oriented (here, "you" is "whatever is listed in 'Responsible Users' in the query"):

  • Must Review: Stuff you're blocking.
  • Ready to Review: Stuff you're not blocking explicitly, but which does need review.
  • Ready to Land: Your accepted revisions.
  • Ready to Update: Your authored "needs revision" revisions.
  • Waiting on Review: Your authored "needs review" revisions.
  • Waiting on Authors: Stuff you're reviewing that's waiting on someone to land or update it.

Reviewer calculation now looks at individual reviewer relationships rather than only at overall revision status. For example, accepting a revision always kicks it out of your "Must Review" and "Ready to Review" buckets, while it would previously retain it there if the revision as a whole did not move to "Accepted".

This might have a few weird edge cases and quasi-bugs (for example, if you have differential.allow-self-accept enabled, many of these states are ambiguous, but that option is fairly silly anyway), yell if you catch any revisions that seem out of place.

D15926 is a proposed change which would show more information about individual reviewers in the list view, but it also adds a fair amount of UI clutter and we aren't sure if it's still necessary given all the other changes. If the composition of the "Must Review" and "Ready for Review" buckets remains mysterious, we'd like specific feedback about use cases that remain difficult to parse or work with.

Broadly, this represents an update that makes bucketing better aware of individual reviewer status and of aggregate (package/project) reviewers.

These changes are live on this install, although it may be difficult to get much of a sense of them without more realistic data.

Next up, in some order:

  • Explicit manual blocking reviewers.
  • "Soft Accept" / "Unblock" action ("TBR").
  • Support for non-hierarchical packages / implicit ownership.

GoalRefTimeNotes
Dashboard BucketingD15921, D15923, D15924, D15925, D15927, D15928, D15929, D159303 HoursModernize dashboard bucketing.
Subtotal3 Hours
Cumulative Total7 Hours

I'm no longer able to search for "Responsible Users: (Current Viewer)". Is that expected?

Yes, since it would really mean exact(current-viewer) without additional work. Are you using it for something (dashboards?)

The default home page (and homepage sidebar notification count) are also still doing the old grouping / counts. We should probably fix that but it's not a strict dependency here and there's adjacent work (T3583).

  • "Soft Accept" / "Unblock" action ("TBR").

This doesn't matter for this task, but for ease of communication. TBR in our org stands for to be reviewed. Historically it should have been spelled as TBNR since it will never be reviewed. In reviewboard the mode of operation is that you create a diff with the correct reviewers and then submit with a special flag. In Phabricator land the flag is the default prompt for an unaccepted revision. We're excited about the ability to use the Audit application to enforce the eventual review but we're a bit aways from making that useful.

Just to make sure I'm understanding correctly:

After everything that's currently in flight lands (i.e., up to D15936) I expect you to be able to get this far:

  • Import existing OWNERS files by using the API to create packages, as you do today.
  • Have the script set "Auto Review: Blocking" and "Dominion: Weak" on the packages, so they automatically become blocking reviewers with non-hierarchical ownership.
  • "Auto Review" will add package reviewers to new revisions on the server-side, without the need for client-side rules. "Weak Dominion" will keep shallow-path package owners from being added to deep paths owned elsewhere.
  • The dashboard will reflect package reviewers and user state in a reasonable way.

At this point, you may end up with some packages that are blocking reviewers but whose owners are not currently available. The revision author can move forward by doing any of these:

  • Editing "Reviewers" and removing the blocks (after D15933, they can downgrade blocks to normal reviewers via the web UI).
  • Editing "Reviewers" and removing the reviewers entirely.
  • Editing "Reviewers: ..." from the CLI with --edit and removing blocks (after D15934) or reviewers.
  • Using "arc land" and "Y"'ing through the prompt about the revision not being accepted.

It sounds like that's sufficient for now, and the "Soft Accept" / "Partial Accept" / "Accept for Package" / "Unblock" actions aren't too important for the moment? You can sort of accomplish them with the mechanisms above anyway, it's just less convenient and your intent is less clear than if we had a real "Remove some of the blocking reviewers, explicitly" action.

Once the author forces their way through landing, I currently expect "Auditing: Enabled" to trigger an audit for any packages which did not accept, providing a safety net to catch these changes later in the flow. In at least some subset of cases this seems to work correctly (e.g., we use audits for this purpose on this server, and have for many years, and they appear to work as expected) although T10181 says it doesn't, and it almost certainly needs some updates to account for package reviewers. I plan to look at T10181, figure out what's going on there, then update the behavior of "Auditing: Enabled" to fix whatever actual bugs are described in T10181 / T10174 and account for package reviewers, but I'm not expecting this to involve any major changes. So the flow would then continue:

  • The API calls would also have set "Auditing: Enabled", above.
  • If a revision is forced through with any of the above mechanisms, the "Auditing" setting causes audits trigger automatically as a safety net to catch changes which didn't receive real review by some packages.

This stuff, discussed above, still won't have a clean solution:

  • Users with root ownership (say, on "/") won't have any special way to clear blocks made by subpackages (say, "/a/b/") which they don't also own. They can do all the "Edit Reviewers" stuff, but when anyone does this to clear blocks their intent won't be made explicitly clear. This is likely solvable culturally ("When you force your way through things, please note why you're doing it on the revision.") but an eventual explicit action would make this clearer.
  • Users with root ownership won't have an obvious way to see that they "indirectly" own some of the packages blocking a revision, although they can likely infer this from external knowledge.
  • Users won't have any special way to clear some of the blocks they own, e.g. "I'm comfortable accepting this for Platform, but someone else should clear Security". They can still use the non-special ways + comment, as above.

Does that sound reasonable, at least for now? If so, I'll focus on the audit stuff and skip the "Clear some blocks" action, at least for the moment.


Independent of this, I've also grown more hesitant about pursuing the "Clear some blocks" action before doing infrastructure work (T10967 + EditEngine). Now that I've reacquainted myself with the code I think it's significantly messier than I'd originally thought. Given the new availability of workarounds, I think it's a good candidate to wait until later for if everything above is roughly on target.

Yes, since it would really mean exact(current-viewer) without additional work. Are you using it for something (dashboards?)

I used it for custom Differential queries. after updating, all of my custom queries were broken. I did it this way to make it easier to share my queries with others.

(I'll restore viewer(), it's just not a one line change.)

Just to make sure I'm understanding correctly:

This stuff, discussed above, still won't have a clean solution:

  • Users with root ownership (say, on "/") won't have any special way to clear blocks made by subpackages (say, "/a/b/") which they don't also own.

But a root owner's accept will still count for the sub-package, provided dominion is weak?

  • Users with root ownership won't have an obvious way to see that they "indirectly" own some of the packages blocking a revision, although they can likely infer this from external knowledge.

If this fix for this is minor we'd appreciate this being fixed, but its non-blocking.

  • Users won't have any special way to clear some of the blocks they own, e.g. "I'm comfortable accepting this for Platform, but someone else should clear Security". They can still use the non-special ways + comment, as above.

As it is today.

Does that sound reasonable, at least for now? If so, I'll focus on the audit stuff and skip the "Clear some blocks" action, at least for the moment.

This is all reasonable. (cc @scode just in case I'm being unreasonable)

But a root owner's accept will still count for the sub-package, provided dominion is weak?

No, they currently (well, "currently" in the "slightly-ahead-of-HEAD" future sense) don't own the package at all.

We could conceivably add another rule like, uh, "Reverse Upward Dominion: Give owners of parent packages authority over this package without making them members", but dominion currently only works down the chain by ceding control of paths to sub-packages. I think incorporating this into an eventual "Clear some of the blocks" action is probably cleaner, where they don't really get authority but get to do an "easy clear" with a note instead of a "hard/forced clear".

Specifically, I'm vaguely imagining that this future UI looks something like this:

Action:(Clear Some Blocking Reviewers V)
Clear Blocks:[ ] Platform Engineering Owner
[X] UIE Indirect Owner
[ ] Security Not Affiliated

Except less bad, and I'd like to make you confirm / check five boxes / sign your name in blood if you tick any of the red "Force Unblock" boxes.

If this fix for this [indirect ownership] is minor we'd appreciate this being fixed, but its non-blocking.

I think this is probably enough work that it makes sense to hold until "Clear some of the blocks", but I can poke at it.

I've consolidated the general state of the world in Audit in T10978. I think we'll run into some of that eventually but I'm not sure how much is important vs nice-to-have, so my general plan here is to just scratch the surface for now:

  • Document/verify audit rules (T10181):
    • Fix some of the weird audit/package rules that we have today, some of which are inconsistent.
    • Document the expected rules.
    • Verify locally that stuff actually works in the way that I think it does.
  • Figure out what's going on with T10174.
  • Restore viewer() to Differential.

Then pause this until you have a chance to evaluate where things are and the Differential changes get a chance to settle for a bit. Then we can plan our way forward through continuing improvements to this workflow in Audit, or fixing any bugs or rough edges you hit prior to that, or building "Clear some blocks" sooner, or whatever else.

scode added a comment.May 17 2016, 3:18 PM

Thanks @epriestley!

Sounds good (including your last comment). I think that from our perspective, we're it seems likely that we'll end up realizing a few problematic edge cases or details once we start integrating fully and it's probably best to wait until then than to try to gain perfect confidence ahead of time. The work done so far and what you outline in your last comment seems sufficient to start doing the integration work and we can surface issues as they arise.

nickz added a subscriber: nickz.May 17 2016, 6:02 PM

Blocking reviewers can now be added, edited and removed via the web UI with the blocking(...) function. For example, entering blocking(epriestley) into the "Reviewers" typeahead will add epriestley as a blocking reviewer. You can use this to upgrade reviewers to blocking or downgrade blocking reviewers to non-blocking.

Blocking reviewers can now be added, edited and removed via the CLI. On the CLI, typing Reviewers: epriestley! will add epriestley as a blocking reviewer. This status will reflected in arc diff --edit, etc.

(Two limitations: the transcripts for these actions are lackluster, and the "Add Reviewers: ..." action doesn't support adding blocking reviewers yet -- only "Edit Revision". Improving these cases is probably blocked on T10967.)

Owners packages can now select "Dominion" rules, which allows them to switch from always controlling paths to ceding control to other packages which express a more specific ownership of a path. I picked a funny name here since the other names we were using ("indirect ownership") weren't very descriptive or evocative and this one can at least be searched for fairly easily, and is somewhat memorable/meaningful once you understand the feature. The documentation discusses this option in more detail.

I couldn't find any real bugs with the existing audit behavior (T10181). I updated it to handle multiple reviewers better and have a behavior more consistent with the "Auto Review" behavior. The new behavior is now documented. If you find it behaving in a way that isn't consistent with the documentation, let me know. Basically, I can't find any actual bugs and didn't see any repro steps anywhere, so I've fixed everything I could guess at, but might have missed something which just didn't get written down anywhere.

Internal audit relationships ("None", "Not Applicable") were previously given unusual prominence and behaviors. They no longer show as audits in the UI, and Herald can now overwrite them. This should resolve T10174. In particular, you can write stronger or more specific Herald audit rules if you want to enforce tighter rulesets than "Auditing: Enabled" does.

I've consolidated some additional audit issues beyond these workflows in T10978. Installs that are able to make greater use of packages and auditing after these improvements may start running into issues discussed there as the workflow progresses.

Per above, I'm planning to let these changes settle and stabilize for a bit now, and expect to plan the next phase of work after feedback about what is or isn't working so far.

(I'll fix viewer() in Differential by the time we promote to stable at the latest.)

These changes are live on this install, and I've manually updated the documentation since I think the cronjob runs at 2AM or something.


GoalRefTimeNotes
Explicit BlockingD15933, D159341 HourAllow blocking reviewers to be added/removed explicitly.
DominionD15935, D159361 HourImplement "Weak Dominion" rules for packages.
Audit FixesD15939, D159400.5 HoursFix some behaviors with uninteresting auditors.
Subtotal2.5 Hours
Cumulative Total9.5 Hours

#twitter hat off. FreeBSD hat on.

It seems as a result of the dashboarding changes project reviewers no longer cause the diff to show up on the dashboard. For example https://reviews.freebsd.org/D6135 has a group reviewer of #arm but even when I join the project I don't see it anywhere on the dashboard.

That's not expected, but I can't immediately reproduce it. Here's my local install, note that D4 appears in "Ready to Review" even though I (admin) am only a reviewer via project membership in Stonework (a project):

The default is still using the old algorithm, is that what you mean by dashboard (i.e., not "Active Revisions" in Differential)? But it looks like that install has a custom homepage dashboard with the active filter configured, which I'd expect to work.

The default is still

Er, "the default panel on the non-dashboardized homepage".

I'm referring to the home page dashboard. I just made some config changes (particularly, changing the limit of the number of revisions to show) and it seems to have 'fixed it' so the problem above would be PEBKAC :/

Ah! Okay, cool. Not really PEBCAK, I need to fix that dashboard too and it's super confusing that it does something weird/different with no clues about it. I just wanted to see if anyone caught major issues with the new one before swapping it.

My "before release cut" cleanup list is currently:

  • Fix viewer() token in Differential.
  • Fix homepage dashboard.
  • Fix homepage "Differential" action count in left application sidebar.

I'm referring to the home page dashboard.

By this I mean: as installed on the FreeBSD install.

epriestley moved this task from The Queue to Paused on the Prioritized board.May 19 2016, 10:26 PM

That cleanup stuff should be cleaned up now, so I'm formally pausing this for feedback.

I don't know how to reproduce the issue, but figured I'd let you know that I'm seeing this in our error logs:

[23-May-2016 10:25:49 Australia/Sydney] [2016-05-23 10:25:49] ERROR 2: Illegal string offset 'phid' at [/usr/src/phabricator/src/applications/differential/customfield/DifferentialReviewersField.php:246]
[23-May-2016 10:25:49 Australia/Sydney] arcanist(head=master, ref.master=2234c8cacc21), phabricator(head=master, ref.master=3d3fff4991ab, custom=1), phlab(head=master, ref.master=84b62e6917ec), phutil(head=master, ref.master=bd56873ae4c0)
[23-May-2016 10:25:49 Australia/Sydney]   #0 DifferentialReviewersField::validateCommitMessageValue(array) called at [<phabricator>/src/applications/differential/conduit/DifferentialParseCommitMessageConduitAPIMethod.php:70]
[23-May-2016 10:25:49 Australia/Sydney]   #1 DifferentialParseCommitMessageConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:122]
[23-May-2016 10:25:49 Australia/Sydney]   #2 ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
[23-May-2016 10:25:49 Australia/Sydney]   #3 ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
[23-May-2016 10:25:49 Australia/Sydney]   #4 ConduitCall::execute() called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:81]
[23-May-2016 10:25:49 Australia/Sydney]   #5 PhabricatorConduitAPIController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[23-May-2016 10:25:49 Australia/Sydney]   #6 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[23-May-2016 10:25:49 Australia/Sydney]   #7 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

Just FYI.

General question: Why is the owners option "Add as reviewer (blocking)" if an owner uploads a revision? I know that this is expected, but why? For example at Wikimedia we currently have the system, that every change needs a review from an "owner" before the patch gets merged, so it's not useful, if an owner of the package uploads a change, and no reviewers get added. Maybe the a compromise may be a setting, where you can choose, if a package gets added as reviewer too, if the author of the revision is an owner too? I think that would help all.

eadler updated the task description. (Show Details)May 23 2016, 12:05 PM

@joshuaspence, see T11010, I think. Not sure why nginx was 502'ing there, but should be fixed in HEAD.

As an alternative to Luke081515.2's request for an option, I just filed T11118. This would also allow an easy return to the old behaviour of always triggering audits, regardless of who the author is.

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jun 20 2016, 6:58 PM

General question: Why is the owners option "Add as reviewer (blocking)" if an owner uploads a revision? I know that this is expected, but why? For example at Wikimedia we currently have the system, that every change needs a review from an "owner" before the patch gets merged, so it's not useful, if an owner of the package uploads a change, and no reviewers get added. Maybe the a compromise may be a setting, where you can choose, if a package gets added as reviewer too, if the author of the revision is an owner too? I think that would help all.

This is essentially identical to the behavior described above, except that the "NOTE" above is untrue.

ohler added a subscriber: ohler.Jul 17 2016, 12:04 AM
urzds added a subscriber: urzds.Nov 28 2016, 12:37 PM

From elsewhere:

  • Create owner package with weak dominion A/ with path A/
  • Create a revision that modifies directory A/B/ with no owner package in it
  • Create a weak dominion owner package A/B/ for path A/B/ with myself as one of the owners
  • Update revision with any changes

This does not remove package A as a reviewer, but reasonably could if package A was added by automatic rules and has not been touched since then (we can track this after T10939). If a Herald rule or a user explicitly added it, we should not remove it, and probably should not remove it if other users have interacted with it by acting with authority on behalf of it.


Alongside this scenario, it was reported that package "A" can not be removed as a reviewer. If so, this is strictly a bug.

crazed added a subscriber: crazed.Apr 10 2017, 7:12 PM