Re: phab reviews

[Ben Cooksley]

On Tue, Aug 29, 2017 at 8:52 PM, Harald Sitter  wrote:
> On Sat, Aug 26, 2017 at 10:17 PM, Ben Cooksley  wrote:
>> On Sun, Aug 27, 2017 at 1:15 AM, Adriaan de Groot  wrote:
>>> On Saturday 26 August 2017 23:06:29 Ben Cooksley wrote:
 > 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.

 Ideally you wouldn't need to set a reviewer at all - it should be
 handled for you automatically.
>>>
>>> Would it make sense to add this information -- possibly just as a comment, 
>>> but
>>> it'd be better as real data -- in the .arcconfig of each repo? That would
>>> require work from Phab upstream to do right, but it would be useful so that
>>> individual repo's can say
>>>
>>> reviewers: [ tom, dick, harry ]
>>>
>>> or whatever the right JSON formatting is for that.
>>
>> That would potentially be another way of doing it yes. However if
>> someone isn't using Arcanist to make their submission then they won't
>> be affected by this (they could manually check it I guess though).
>
> I rather liked this bit of reviewboard TBH, alas, won't fix for phab:
> https://secure.phabricator.com/T6927
>
> (also Owners is actually way more versatile and works for both arc and
> manual diff upload, so I think I like that actually better and so
> should everyone else ;))

In terms of getting started with this, how would you like to proceed?
I'd suggest we start with a couple of small Extragear projects to
confirm Owners is working as we expect before we bulk out the
implementation to cover all projects.

>
> HS

Cheers,
Ben

Re: phab reviews

[Harald Sitter]

On Sat, Aug 26, 2017 at 1:06 PM, Ben Cooksley  wrote:
>> 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.
>
> I haven't checked to see if there are any scalability limitations
> within the Owner packages system, so it may be worth keeping that in
> mind.
>
> For repositories which are maintained by a single person (primarily
> Extragear and Playground, and to a lesser extent Applications) this
> should be fine. If there are 2-3 repositories which make up the same
> logical group (such as Atcore and Atelier to use one example) then it
> would probably be better to cover them with the same rule (as they're
> one "package")

Organizationally that would make sense, from an implementation POV it
depends... The question is if we want to automate this or not. If it's
fine for a sysadmin to create the Package line-up manually upon repo
creation that'd be the time to group things, which makes a lot of
sense and is how Ownership is meant to be used anyway. If we want this
to happen automatically by inferring data from repo-metadata we'll
currently not have the "grouping" context information to do that
(which could, of course, be added if need be but generally increases
complexity).

> For large groupings of repositories, where the review is by a group of
> people (like for Frameworks and Plasma for instance) we probably want
> to continue using Herald rules for those, as then we don't have to
> replicate the project to repository mapping.

Given frameworks have dedicated maintainers I think there it would
also make sense to have them always set as reviewers (in addition to
having the ML subscribed I guess). Overall I'd say that's a matter of
individual policy of the teams/individuals though?
The problem with mass subscription is that the better part of ML
subscribers will outright ignore/filter the review mails rendering
them largely moot. I recently had an encounter where someone
complained about me not putting a reviewer on a Plasma diff when in
fact the auto-subscription of Plasma was active (the diff hadn't
gotten a single comment). With ML subscriptions it is really easy to
ignore them, not just technically but also socially aka "someone else
from the ML will deal with this so I don't need to".

I really wonder if diffs with ML subscriber shouldn't also end up in
the no-reviewer bucket via Herald. At the end of the day they still
need someone to make sure the diffs get a review, and from personal
experience, I am not convinced the MLs are necessarily able to take
care of that, whereas a dedicated global reviewer team might.

> I'm not sure if Owner packages have the power to subscribe mailing
> lists or projects to reviews - this is something Herald is certainly
> able to do though.

>From a bit of playing around it seems to me that the same way it is
done for Herald will work for Owners. Any user/project entity can be
owner, if a user has a ML address the ML will get the mail.

>>
>> 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.
>
> The only potential issue I see with that is i'm not sure what the
> order of evaluation is here. It is possible that this ends up being
> executed before other rules which would have added reviewers so you
> may end up with false positives here. Is that something the global
> reviewers folks would be happy to handle?

Can we test this? Supposedly phab would be able to figure out that a
rule without repo restriction is more generic and thus ought to be
applied later, whether it does that or not is a different story xD

Ideally, I'd say reviewers should only be added via Owners, and Herald
should only add subscribers. And then my thought from above would
apply that a diff without reviewer is always in need of shepherding
from the team even if it has a ML subscriber. Whether that is
sustainable is something we'll have to try and see I guess.

HS

Re: phab reviews

[Ben Cooksley]

On Sun, Aug 27, 2017 at 1:15 AM, Adriaan de Groot  wrote:
> On Saturday 26 August 2017 23:06:29 Ben Cooksley wrote:
>> > 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.
>>
>> Ideally you wouldn't need to set a reviewer at all - it should be
>> handled for you automatically.
>
> Would it make sense to add this information -- possibly just as a comment, but
> it'd be better as real data -- in the .arcconfig of each repo? That would
> require work from Phab upstream to do right, but it would be useful so that
> individual repo's can say
>
> reviewers: [ tom, dick, harry ]
>
> or whatever the right JSON formatting is for that.

That would potentially be another way of doing it yes. However if
someone isn't using Arcanist to make their submission then they won't
be affected by this (they could manually check it I guess though).

>
> [ade]

Cheers,
Ben

Re: phab reviews

[Adriaan de Groot]

On Saturday 26 August 2017 23:06:29 Ben Cooksley wrote:
> > 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.
> 
> Ideally you wouldn't need to set a reviewer at all - it should be
> handled for you automatically.

Would it make sense to add this information -- possibly just as a comment, but 
it'd be better as real data -- in the .arcconfig of each repo? That would 
require work from Phab upstream to do right, but it would be useful so that 
individual repo's can say

reviewers: [ tom, dick, harry ]

or whatever the right JSON formatting is for that.

[ade]

signature.asc
Description: This is a digitally signed message part.

Re: phab reviews

[Ben Cooksley]

On Sat, Aug 26, 2017 at 1:11 AM, Harald Sitter  wrote:
> On Fri, Aug 25, 2017 at 2:00 PM, Ben Cooksley  wrote:
>> On Fri, Aug 25, 2017 at 8:59 PM, Harald Sitter  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.

Ideally you wouldn't need to set a reviewer at all - it should be
handled for you automatically.

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

Yes.

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

I haven't checked to see if there are any scalability limitations
within the Owner packages system, so it may be worth keeping that in
mind.

For repositories which are maintained by a single person (primarily
Extragear and Playground, and to a lesser extent Applications) this
should be fine. If there are 2-3 repositories which make up the same
logical group (such as Atcore and Atelier to use one example) then it
would probably be better to cover them with the same rule (as they're
one "package")

For large groupings of repositories, where the review is by a group of
people (like for Frameworks and Plasma for instance) we probably want
to continue using Herald rules for those, as then we don't have to
replicate the project to repository mapping.

I'm not sure if Owner packages have the power to subscribe mailing
lists or projects to reviews - this is something Herald is certainly
able to do 

Re: phab reviews

[Harald Sitter]

On Fri, Aug 25, 2017 at 2:00 PM, Ben Cooksley  wrote:
> On Fri, Aug 25, 2017 at 8:59 PM, Harald Sitter  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

Re: phab reviews

[Ben Cooksley]

On Fri, Aug 25, 2017 at 8:59 PM, Harald Sitter  wrote:
> Hola!

Hi Harald,

>
> Our phabricator guide says that at least one person needs to be set as
> reviewer to get any review.
> https://community.kde.org/Infrastructure/Phabricator#Posting_a_Patch
>
> This sucks.

That is correct. Reviewboard had a similar issue, albeit with the lack
of any ability to autosubscribe people.

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

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

>
> HS

Regards,
Ben