>>> 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. >at the end of the day tooling needs to ensure that reviewer has the >authority to approve changes.
Well, that's an assertion. I'm not sure that the tools *needs* to do more to ensure it than today; I believe that some people believe it *should* do more. And perhaps they're right. >i don't think the issues you've raised are show stoppers; it's certainly a >system we will iterate on over time. >keep in mind _how_ we work is also something we should be iterating on too. Agreed. And if something can work better, great; but I'm also leery of adding a lot of process or gatekeepers, especially when you need to touch a module with few official peers, who might not be available when you need a rubber stamp of a nit fix. (Initial review has nits; revise, now the reviewer is on PTO or simply away for the weekend or a holiday and you have a big landing gated on this, or a sec issue, or will miss a release if you can't land before uplift/soft-freeze, etc, etc, etc) An alternative is more guidelines (and training, especially for new devs) for reviewers/peers/devs to follow, and deal with any oversteps on a case-by-case basis, and if there are continual issues then add enforced process. Right now it's kinda learn-by-watching-other-devs - which works, but makes it hard to be sure you learned the right things. >the current system is extremely coarse - everyone with scm-level-3 can >approve any change tree-wide. >there is a strong desire to make this finer grained I believe that; though I'd be interested to know with who, the reasons, and the observed problems, and what other solutions have been considered. Of course, I don't need to know these; I'm just interested as while I've seen occasional fubars, I haven't seen persistent problems that would require that (especially over improved guidelines). Also, for example, one could add these gate-checks with overrides that devs can use at their discretion (like for nits). I'm sure added checks can avoid some fubars - but I'm also sure it will cause added friction to development, extra load on some people, and slow some things down. >> [snip] >> 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. >the enforcement you're referring to is controlled by a hardcoded list of >peers inside a mercurial hook. > >this doesn't scale anywhere close to our needs, and all of the exact same >concerns you raise with regards to in-tree ownership tracking applies >(however it would be worse as it's imposed by a system separate from the >review/landing tooling). Agreed. Moving those to in-tree lists is certainly a win over current. -- 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
