phab reviews

[Harald Sitter]

On Sat, Aug 26, 2017 at 1:06 PM, Ben Cooksley <bcooksley at kde.org> 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