Enforce OWNERS files in Chrome OS repositories |
|||||
Issue descriptionWe've never enforced OWNERS files for commits to Chrome OS repositories. We have a bunch of OWNERS files (although probably not the level of coverage we'd want), but they're used purely in an advisory capacity; owners need to set up notifications within Gerrit and stay on their toes if they want to sign off on everything that's going into a given repo or directory. The main source of complexity here is probably the way that we put a bunch of unrelated changes together in a single CQ run instead of testing them individual like Chrome does. I think what we'd want would be something like: a) Pre-CQ still runs without owner approval (since people often want a pre-CQ run in parallel with getting a review). b) CQ doesn't include changes that don't have owner approval. We need to drop these from the CQ instead of making the whole CQ run fail. Re b), I'm not sure whether we'd want to also automatically remove the CQ ready bit from the change when it's not yet approved. Right now, people often set that pre-review when they're pretty confident that the reviewer is going to approve it without any comments (or at least I do :-P).
,
May 14 2018
This is a Build area change that I think is worth doing. It leads to better code quality as OWNERS tend to defend their areas from lowest-common-denominator changes. The first step would be creating those OWNERS files. Over to vapier@ to triage.
,
May 14 2018
We have a bunch of OWNERS files already (68 in platform2 alone). I think that enforcing them is the next step.
,
May 15 2018
seems like we have this same discussion every year, but i can never find the previous threads, probably because we have them in different places (e-mail, docs, etc...) :p. personally, i'm not a fan of enforcing OWNERS as i find, in most cases, it adds bureaucracy that hasn't been necessary thus far in the project. if i'm reading the docs [1] correctly, we can simply add a "*" as the first line to existing OWNERS files to retain that behavior. so the file acts as documentation of who you should contact, but it's not ultimately a barrier (unless a dev wants to lock down the dir). on the Chromium side, they clear the CQ+2 bit if OWNERS is missing, so it's probably reasonable for us to do the same. otherwise, if a non-owner has given the CL a +2, people are going to ask "why hasn't the CQ picked up my CL" yet. clearing that would simplify the design greatly as we wouldn't have to maintain parallel state somewhere (e.g. cidb/equiv) like "has the CQ commented on this CL/PS combo that OWNERS is missing". imo we should add "*" to all OWNERS files unless people listed there specifically object, then we start rolling out soft warnings/notifications for a period of time to get devs used to the flow and to encourage updates of the OWNERS files, and then we start making it enforcing. i expect we'll leverage the existing Chrome tooling to do the actual checking/processing of these files rather than implement our own.
,
Jun 14 2018
,
Jun 15 2018
"The main source of complexity here is probably the way that we put a bunch of unrelated changes together in a single CQ run instead of testing them individual like Chrome does." If this the main (or one of the main) premises of this bug. If so, weren't we told that this changes with Skylab or related new infra, where the CQ runs on only one CL at a time?
,
Jun 15 2018
people asking for OWNERS support has nothing to do with how we get CLs through the CQ (bundled up or individual or otherwise). it was noted as a possible complexity point on the way to implementing OWNERS support. the topic of how the CQ tests/merges things is handled elsewhere. lets not get into in this bug.
,
Jul 13
,
Sep 19
,
Nov 8
Any ideas about how to make forward progress on this? Issue 886394 looks like it's stalled out on Gerrit plugin limitations. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by derat@chromium.org
, Jul 25 2017