phab reviews

[Harald Sitter]

On Fri, Aug 25, 2017 at 2:00 PM, Ben Cooksley <bcooksley at kde.org> wrote:
> On Fri, Aug 25, 2017 at 8:59 PM, Harald Sitter <sitter at kde.org> wrote:
>> For one it makes it hard to keep on top of reviews across all our software.
>> More importantly though, how exactly do we expect a drive-by
>> contributor to figure this out? I work in the community for years and
>> yet often enough struggle to find a relevant person to set as
>> reviewer.  Not just figuring out their name on phab, but even finding
>
> You can search by someone's actual name in any autocomplete prompt
> where names are accepted I believe, so not knowing their username
> shouldn't be an issue.

Not from `arc` which is a wholly different problem I suppose. But yes,
it's kinda manageable, just not all that convenient. The tricky bit is
really figuring out who to set as reviewer to begin with.

>> out who is relevant in the context of the repo. There are projects who
>> have no real prominent head so looking at the git log basically gives
>> you a bunch of drive-by commits.
>>
>> Can we improve this somehow?
>>
>> Not particularly enjoyable but working solutions I have in mind:
>>
>> a) establish phab team of volunteers to get spammed by all reviews (or
>> more ideally reviews without reviewer set) and then either sort out
>> the best reviewer or review it themself
>>
>> b) perhaps more scalable: no automatic subscription of the team but we
>> change the wiki to tell the submitter to use the aforementioned team
>> as reviewer when he doesn't know any better person
>
> Phabricator has two different tools which can help us here: Herald and Owners.
>
> Owners is a utility which allows someone to say what paths in a given
> repository they are responsible for. These packages can then be either
> used by themselves, or utilised by Herald rules.
>
> Herald is an extremely powerful utility, which has the power to do ...
> way too many things to list here.
>
> One of those things is the ability to automatically subscribe Projects
> or Individuals to reviews, or add them as reviewers or blocking
> reviewers. Depending on how a given Herald rule is setup, it can
> enforce this - escape is impossible.
>
> Please see https://secure.phabricator.com/book/phabricator/article/owners/
> and https://secure.phabricator.com/book/phabricator/article/herald/
> for more detail on what they're capable of.
>
> There is a performance penalty to be paid with Herald however of
> around 1ms per rule we have, which is why access to Herald is
> restricted to Community Admins.
>
> We currently have rules to cover: Okular, Dolphin & Konqueror, All
> Games, Kile, Konsole, Simon, Documentation (all repositories),
> Frameworks, KWin, Plasma, KWayland, Kirigami, Ark, Kate, KProperty,
> KDb, Calligra, Kexi, Plasma, Marble, KDE PIM, All Utils Apps, KDevelop
> and RKWard.
>
> These rules essentially CC the mailing list for those projects, and in
> some instances add the appropriate project as a reviewer. At the very
> least it should ensure it's brought to the attention of involved
> developers.

Thanks for the info.

So, I think there are two problems we need to deal with: the internal
and the external.

Internally developers need to be pro-active in ensuring their products
have a subscriber. Which they can do with Owner packages it seems. In
fact we could perhaps automate this (initially on repo creation?). We
have a members property in our repo-metdata yamls, so we could have a
package for each repo that tracks all files and subscribes the Owners.
That way technically every repo has a reviewer. I am not sure if doing
it automatically is necessarily a good idea, perhaps only for new
repos upon initial creation? What we certainly could do is create a
package for each repo so people just need to add themselves as owners.

Externally we still need a fallback system though as to remove the
"your stuff doesn't get reviewed unless you define a reviewer"
problem. That's where I think a volunteer team of global reviewers
would come in handy. A global Herald rule acting on no-reviewers &&
no-subscribers to then add the Global Reviewers as subscriber would
make sure every diff ends up in someone's inbox. The team's action
could then be to identify the actually responsible person and make
sure they are set as owner (as per above), subscribe a more relevant
team, or do the review themselves.

With this combination, 99% diffs would have some owner(s)
automatically set as a reviewer and the remaining 1% would get handled
by a team to make sure nothing falls through the cracks.

Does that make sense?

HS