TBR makes it too easy to skips owner checks. Proposal: TBR[content/foo] = owner |
|||||
Issue descriptionForking this form Issue 736215 comment #24 and adding some extra thoughts I had recently on the topic Problem ------- I see the practical need for TBR but there are two general problems: 1) It's not immediate for reviewers getting TBR'd to figure out what are they TBR'd for. Not every author is nice enough to write "TBR foo@ for content/browser/bar.cc". And even when they do, it's quite hard to scrape these in CLs that have too many comments. 2) TBR completely bypasses owners check, even when the author doesn't mean it. Concrete example: in [1] I'm TBR-ing two people because I am doing silly mechanical changes in chrome/browser/metrics/* and net/url_request/url_request_quic_perftest.cc. The rest of the CL is not mechanical though and I don't want to accidentally bypass the actual reviewers. For instance in that CL I am touching a mojom file that requires a SECURITY_OWNER stamp. I really don't want to accidentally skip that. Proposal -------- Rework the TBR format to be very explicit. For instance in that CL I would have loved to: TBR[content/browser/metrics/*] = asvitkine TBR[net/url_request/url_request_quic_perftest.cc] = xunjieli And leaving the possibility to do TBR[*] = foo In cases of urgent things (e.g. reverts) And, of course, TBR[*] should be extremely socially unacceptable for anything other than reverts. [1] https://chromium-review.googlesource.com/c/544976/
,
Jun 27 2017
I think it's just a bug that TBR is bypassing owners checks. I don't remember offhand if I did that intentionally and, if so, why.
,
Jun 27 2017
It's a "feature" that TBR bypasses OWNERS checks, and it has been that way for as long as I can remember. But yes, the system should be revamped to simply include anyone on the TBR line in the list of reviewers for the sake of computing OWNERS coverage, and if the set still isn't covering, reject the CL.
,
Jul 3 2017
Gerrit CQ will no longer care about TBR. presubmit support which is part of depot_tools does.
,
Jul 4
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 10
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by thakis@chromium.org
, Jun 27 2017