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