We are now enforcing "OWNERS file" approvals in chromium.
This means that by default in order for you to land your change you
need an approval in your code review from someone listed in an OWNERS
file for every file you change, as documented here:
http://dev.chromium.org/developers/owners-files-1
You approve changes by adding a comment with the acronym "lgtm" (case
insensitive) to the issue. Eventually we'll add a check box to be
fancy :) We also plan to add a way to delegate approval.
This check is enforced by the presubmit hooks in git-cl and gcl. This
means you can bypass the checks with --force or --bypass-hooks (git
cl) or --no_presubmit (gcl), so you're on something of an honors
system.
The good news is that for now there is a wildcard OWNERS file at the
top of the tree, so, with few exceptions, any committer is an OWNER
for any file. The main exception is for stuff under the (newish)
"content" directory, which does not recurse up to the top file. We're
going to field test this with that directory first while we gradually
propagate the correct owners elsewhere.
For now we only check the owners on commit. Shortly (as early as later
today, or over the weekend), I will also enable the check on upload,
which will attempt to suggest reviewers for you based on the contents
of the OWNERS files and your change. I'm not enabling it yet because
the initial algorithm I used has turned out to be too annoyingly
stupid to be useful :)
Feedback welcome. Please let me know if there are any problems.
-- Dirk
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
We have talked about adding glob-based rules and rules that track
directory modifications
(files or directories being added or deleted, but not changes to
existing files) as other
ways to minimize the notification burden as well.
It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.
-- Dirk
Since this is a git-cl side check, will this also work for chromium-os
repos? (I am guessing we'll need one at least one OWNER file per
repo). Also, how does git-cl handle commits if no OWNERS file was
found (top level or in the subtree)?
On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
--
-g
-- Dirk
It looks like you are explicitly looking for LGTM. Many folks use LG
and OK as well. Can the script look for those as well?
-Scott
-Scott
On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:
I don't think we need to load down (or completely remove) this check
for the sake of NLP heuristics -- you'll always be able to override
with a flag for these edge case, right? (Having said that, +1 to
adding LG).
-- Dirk
-Scott
It would work for chormium-os as well, but my understanding is that
you guys are using Gerrit and hence don't need or want OWNERS files?
** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/browser/tab_contents/tab_contents.cc
I'd be more useful if it emitted list of owners that are required to
approve, so to take off burden to scan OWNERS files manually.
-- Dirk
Any given OWNERS file is simply a list of email addresses (possibly
including a wildcard).
However, if you have a change that touches files in multiple
directories with different sets
of OWNERS, you can end up needing logic like you describe.
-- Dirk
Running presubmit hooks...
** Presubmit ERRORS **
Missing LGTM from an OWNER for:
tools/heapcheck/suppressions.txt,tools/valgrind/memcheck/suppressions.txt
Any idea what went wrong?
On Fri, Mar 18, 2011 at 9:20 AM, Dirk Pranke <dpr...@chromium.org> wrote:
-- Dirk
-- Dirk
It's not the typing that is hard, it's the unforgetting OK and LG.
-- Dirk
But, if the OWNERS file in question also had "set noparent", you'd
have a problem, in which case you'd use --force or
--bypass-hooks to override the warning, and then (hopefully) have
someone check in a new OWNERS file
with more OWNERS (using the same mechanism).
It might be a good idea to make sure every OWNERS file always has at
least two owners to minimize this risk.
Let's see how often this comes up in practice before trying to
implement a solution.
-- Dirk
How do we deal with the situation that a folder has *one* entry in the OWNERS file and that person is on vacation/out of pocket ?
On Fri, Mar 18, 2011 at 2:25 PM, Dirk Pranke <dpr...@chromium.org> wrote:It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.
Has automatically adding reviewers and putting super-long R= lines in commit descriptions been added as part of this rollout? If so, can we disable that? Having these obnoxiously long R= lines just makes commit messages cluttered and doesn't reflect reality. If R= were to be automatically added, it should be at commit time, not at upload, and be the people whom issued the LGTM. I'd prefer no behavior around this, though.See http://codereview.chromium.org/6705030/ for an example of how ridiculous this is.
It is my hope that the OWNERS implementation turns out to be a net
improvement in
productivity (mostly through suggesting the right reviewers and
streamlining the approval
process). If it isn't, please let me know.
First, thanks for bearing with me over the week. The code has been
buggier than I had hoped it would be (mostly due to differences in the
way gcl and git-cl do things), and in addition, we've been changing
what we actually want the tools to do. Let me sum up where we're at:
1) There are still a couple of open bugs that may be screwing things
up. I hope to land both of them shortly, but any CLs that you've
created over the past couple days may be treated somewhat
inconsistently, so I apologize in advance. In particular, handling
TBR= is a bit broken.
2) The initial algorithm I used to find suggested reviewers was very
brain dead; it simply listed everyone it found. I disabled this for
git-cl before I enabled the checks, but missed a path though gcl, so
you gcl users have been seeing the stupidly-long R= lines. As of last
night, those should be gone now as well.
The net result is that right now both git-cl and gcl should not be
adding R= lines to your changes under any circumstances; the only
check that should be happening is the actual LGTM check on submit.
In the next couple days, I plan to modify the logic in the following ways:
1) When an issue is first created, we should suggest a list of
reviewers; that list should be kept to a maximum length of either 1-3
people (depending on how smart I can get things) or the minimal
covering set of owners needed.
2) Subsequent changes to the issue or uploads will not automatically
modify *any* part of your CL or description. Ideally we'll eventually
add an optional field suggesting additional reviewers if you modify
the fields affected by your CL, but this may take some experimentation
to make useful.
3) On submit, your description will be modified to include an R= line
with the people that actually approved your change (unless you have a
TBR in your description already).
4) There will be two new commands added to git-cl and gcl to suggest
reviewers and tell which approvals are still needed for your review.
It's pretty clear that there's no single policy that will make
everyone happy, but I'll try to get the best compromise I can. As
always, feel free to report and bugs/issues/suggestions to me. If you
are filing bugs against this, the Area is "Build" and you can use the
"Build-OWNERS" label as well.
-- Dirk
--
Alexander Potapenko
Software Engineer
Google Moscow
(There are also names in that file because there are other files in
that directory).
It won't stop you from getting prompted, but anyone can approve it. So
far we have not
agreed to have a "changes to this file don't ever need to be reviewed" concept.
-- Dirk
I think this response should be stronger: Code shouldn't be allowed
into the tree unless there are multiple people willing to be in
OWNERS. If there is literally only one person capable of
understanding the code, then the code cannot possibly be getting good
reviews.
-scott
I agree that we should eventually try to get better coverage across
the directories, though.
-- Dirk
I wouldn't sweat this too much. AFAIK, we don't have
>
> -scott