>On 05/07/2018 18:19, Mark Côté wrote: >> I sympathize with the concerns here; however, changing the default would >> be a very invasive change to Phabricator, which would not only be complex >> to implement but troublesome to maintain, as we upgrade Phabricator every >> week or two.
Makes sense. >> This is, however, something we can address with our new custom >> commit-series-friendly command-line tool. We are also working towards the >> superior solution of automatically selecting reviewers based on module >> owners and peers and enforcing this in Lando. > >Automatically selecting reviewers sounds like a huge improvement, >particularly for people making changes who haven't yet internalised the >ownership status of the files they are touching (notably any kind of >first-time or otherwise infrequent contributor to a specific piece of >code). So I'm very excited about this change. Agreed that this is an improvement for those who are new to mozilla development, though the bugzilla reviewer suggestions are also useful today. This would be the equivalent of adding "Select for me" in Bugzilla. >That said, basing it on the list of module owners & peers seems like it may >not be the right decision for a number of reasons: > >* The number of reviews for a given module can be very large and being >unconditionally selected for every review in a module may be overwhelming. > >* The list of module owners and peers is not uniformly well maintained (in >at least some cases it suggests that components are owned by people who >have not been involved with the project for several years). Although this >should certainly be cleaned up, the fact is that the current data is not >reliable in many cases. This can be cleaned up (and is being so), but will never be perfect. Also, unless you're amazingly careful, this will often route reviews to people on PTO, sick, in the "wrong" time zone, on a holiday day, or simply too busy to review in a reasonable period due to whatever. Some of these are known ahead of time, others aren't. And then there are the "I need a review now to fix something" where the automatic selection might be someone who won't be available to review it for 12 hours, or 3 days - or 3 weeks, requiring noticing this and having someone manually correct it. >* Oftentimes there is substructure within a module that means that some >people should be reviewers in certain files/directories but have no >knowledge of other parts. Or you spend a lot of time deciding and updating who can review what bits - and it doesn't always match the directory heirarchy! >* It usually desirable to have people perform code review for some time as >part of the process of becoming a module owner or peer. Absolutely. >A better solution would be to have in-tree metadata files providing >subscription rules for code review (e.g. a mapping of usernames to a list >of patterns matching files). Module owners would be responsible for >reviewing changes to these rules to ensure that automatic delegation >happens to the correct people. I still don't believe this would work well in practice. It would work, but would have frequent problem cases and often require a lot of updating. In-tree metadata to support suggestions (and to support "select for me" when someone explicitly asks) is good, I think. Note that not all modules are directly related to directories in the tree, so something out-of-tree or some escape valve in-tree is needed to record those. I think automatically selecting reviewers always (as is implied, but it isn't clear that this is the plan - Mark?) is quite problematic, and enforcing reviewers-are-listed-in-tree also has real problems (and may require too-frequent manual intervention) such as not supporting peers assigning reviews to non-peers to train them (to be peers), for cases where a the best person to review a patch isn't a full module peer but an SME on the issue/code in question, etc. These problems can be gamed/wallpapered (assign a review to both the non-peer and a peer who doesn't really review or reviews-the-review, etc), but that adds pain and time and unneeded process. Some modules (i.e. DOM) already to have a hard requirement for peer review. That should be continued and should be enforced as it is now, and it sounds like Lando will (does?) support that. If another module wants to enforce it we can flip a bit in the list of peers and have it enforced. -- Randell Jesup, Mozilla Corp remove "news" for personal email _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
