Remove all the "set noparent" directives

Remove all the "set noparent" directives

Now that OWNERS supports per-file owners, we can limit the scope of the
top-level wildcard to just DEPS, and make darin and ben owners for
everything else and remove the broad use of "set noparent".

R=ben@chromium.org, darin@chromium.org
BUG=88315
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163069

[Dirk Pranke, 2012-10-17 21:41:34]

Let me know if you want anyone else in the top-level OWNERS file.

[Peter Beverloo, 2012-10-17 22:35:30]

https://codereview.chromium.org/11191038/diff/1/OWNERS
File OWNERS (right):

https://codereview.chromium.org/11191038/diff/1/OWNERS#newcode3
OWNERS:3: per-file DEPS=*
We'll probably want AUTHORS and WATCHLISTS to be editable by everyone as well.

[Peter Beverloo, 2012-10-17 22:35:59]

That should read "approvable"..

[Dirk Pranke, 2012-10-17 22:44:05]

On 2012/10/17 22:35:59, Peter Beverloo wrote:
> That should read "approvable"..

Sure, seems reasonable.

[Isaac (away), 2012-10-18 06:53:58]

https://codereview.chromium.org/11191038/diff/5001/build/android/buildbot/OWNERS
File build/android/buildbot/OWNERS (left):

https://codereview.chromium.org/11191038/diff/5001/build/android/buildbot/OWN...
build/android/buildbot/OWNERS:1: set noparent
Will this patch change the behavior of owners on this directory?

[darin (slow to review), 2012-10-18 17:08:35]

LGTM

[Ben Goodger (Google), 2012-10-18 17:33:07]

On 2012/10/18 17:08:35, darin wrote:
> LGTM

LGTM2

[Isaac (away), 2012-10-18 17:54:39]

-1.  We want set noparent or equivalent for build/android/buildbot

[darin (slow to review), 2012-10-18 17:56:02]

Why?
-Darin

On Thu, Oct 18, 2012 at 10:54 AM, <ilevy@chromium.org> wrote:

> -1.  We want set noparent or equivalent for build/android/buildbot
>
>
https://chromiumcodereview.**appspot.com/11191038/<https://chromiumcodereview...
>

[Isaac (away), 2012-10-18 18:15:22]

Because that folder controls core logic for android buildbots on the main
waterfall & CQ.  The equivalent logic for other chromium bots is in the build
repo which is similarly locked down.

[Dirk Pranke, 2012-10-18 18:25:22]

On 2012/10/18 18:15:22, Isaac wrote:
> Because that folder controls core logic for android buildbots on the main
> waterfall & CQ.  The equivalent logic for other chromium bots is in the build
> repo which is similarly locked down.

Perhaps part of the problem here is that src/build currently has a wildcard
OWNERS file, and it probably shouldn't now that we have the per-file support. If
we changed that, so that the only owners that you inherited would be those in
the top-level file (ben and darin) would that be okay? 

Also, I'll be removing the "set noparent"s from the build/ tree as well; they're
not, as far as I'm concerned, special in this regard.

All that said, I don't really care about this. If Ben and Darin don't mind
leaving a few "set noparents" around, I don't either.

[Isaac (away), 2012-10-18 18:34:51]

My preference would be to limit the owners to just the ones in the file.

Feel free to open a bug against android for us to move this folder into the
build repo, which would resolve this OWNERS file issue.

[darin (slow to review), 2012-10-18 19:08:45]

I believe there was a thread about why we are removing "set noparent."  The
reason for "set noparent" was to preclude the possibility of anyone outside
of a trusted set from LGTM'ing changes in a subdirectory.  Now that the
toplevel OWNERS file does not list "*", we no longer have that problem.
 You only have to trust OWNERS and parent OWNERS, which is a small number
of individuals.  I can't imagine what problem would be solved by using "set
noparent" now that we no longer have "*" in the toplevel OWNERS file.

-Darin


On Thu, Oct 18, 2012 at 11:34 AM, <ilevy@chromium.org> wrote:

> My preference would be to limit the owners to just the ones in the file.
>
> Feel free to open a bug against android for us to move this folder into the
> build repo, which would resolve this OWNERS file issue.
>
>
https://chromiumcodereview.**appspot.com/11191038/<https://chromiumcodereview...
>

[Isaac (away), 2012-10-18 19:23:04]

On 2012/10/18 19:08:45, darin wrote:
> I believe there was a thread about why we are removing "set noparent."  The
> reason for "set noparent" was to preclude the possibility of anyone outside
> of a trusted set from LGTM'ing changes in a subdirectory.  Now that the
> toplevel OWNERS file does not list "*", we no longer have that problem.
>  You only have to trust OWNERS and parent OWNERS, which is a small number
> of individuals.  I can't imagine what problem would be solved by using "set
> noparent" now that we no longer have "*" in the toplevel OWNERS file.
> 
> -Darin
> 
> 
> On Thu, Oct 18, 2012 at 11:34 AM, <mailto:ilevy@chromium.org> wrote:
> 
> > My preference would be to limit the owners to just the ones in the file.
> >
> > Feel free to open a bug against android for us to move this folder into the
> > build repo, which would resolve this OWNERS file issue.
> >
> >
>
https://chromiumcodereview.**appspot.com/11191038/%3Chttps://chromiumcoderevi...>
> >

This code is more part of buildbot than chromium, and requires close integration
with the infrastructure team.  My preference is to keep noparent on this
directory.

[darin (slow to review), 2012-10-18 19:24:44]

On Thu, Oct 18, 2012 at 12:23 PM, <ilevy@chromium.org> wrote:

> On 2012/10/18 19:08:45, darin wrote:
>
>> I believe there was a thread about why we are removing "set noparent."
>>  The
>> reason for "set noparent" was to preclude the possibility of anyone
>> outside
>> of a trusted set from LGTM'ing changes in a subdirectory.  Now that the
>> toplevel OWNERS file does not list "*", we no longer have that problem.
>>   You only have to trust OWNERS and parent OWNERS, which is a small number
>> of individuals.  I can't imagine what problem would be solved by using
>> "set
>> noparent" now that we no longer have "*" in the toplevel OWNERS file.
>>
>
>  -Darin
>>
>
>
>  On Thu, Oct 18, 2012 at 11:34 AM, <mailto:ilevy@chromium.org> wrote:
>>
>
>  > My preference would be to limit the owners to just the ones in the file.
>> >
>> > Feel free to open a bug against android for us to move this folder into
>> the
>> > build repo, which would resolve this OWNERS file issue.
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/11191038/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/11191038/<http://appspot.com/11191038/%3Chttps://chromiumcodereview.appspot.com/11191038/>
> >
>
>> >
>>
>
> This code is more part of buildbot than chromium, and requires close
> integration
> with the infrastructure team.  My preference is to keep noparent on this
> directory.
>
>
Please see this thread:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bUjhUgrr_Uw/Jt5T5...

I'm still confused about what issue you are worried about.  I believe that
everyone who is capable of LGTM'ing a change in that directory will consult
with the OWNERS in that directory before approving such a change.  You
don't?

-Darin



>
https://chromiumcodereview.**appspot.com/11191038/<https://chromiumcodereview...
>

[darin (slow to review), 2012-10-18 19:26:22]

On Thu, Oct 18, 2012 at 12:24 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Thu, Oct 18, 2012 at 12:23 PM, <ilevy@chromium.org> wrote:
>
>> On 2012/10/18 19:08:45, darin wrote:
>>
>>> I believe there was a thread about why we are removing "set noparent."
>>>  The
>>> reason for "set noparent" was to preclude the possibility of anyone
>>> outside
>>> of a trusted set from LGTM'ing changes in a subdirectory.  Now that the
>>> toplevel OWNERS file does not list "*", we no longer have that problem.
>>>   You only have to trust OWNERS and parent OWNERS, which is a small
>>> number
>>> of individuals.  I can't imagine what problem would be solved by using
>>> "set
>>> noparent" now that we no longer have "*" in the toplevel OWNERS file.
>>>
>>
>>  -Darin
>>>
>>
>>
>>  On Thu, Oct 18, 2012 at 11:34 AM, <mailto:ilevy@chromium.org> wrote:
>>>
>>
>>  > My preference would be to limit the owners to just the ones in the
>>> file.
>>> >
>>> > Feel free to open a bug against android for us to move this folder
>>> into the
>>> > build repo, which would resolve this OWNERS file issue.
>>> >
>>> >
>>>
>>
>> https://chromiumcodereview.**a**ppspot.com/11191038/%3Chttps:/**
>>
/chromiumcodereview.appspot.**com/11191038/<http://appspot.com/11191038/%3Chttps://chromiumcodereview.appspot.com/11191038/>
>> >
>>
>>> >
>>>
>>
>> This code is more part of buildbot than chromium, and requires close
>> integration
>> with the infrastructure team.  My preference is to keep noparent on this
>> directory.
>>
>>
> Please see this thread:
>
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/bUjhUgrr_Uw/Jt5T5...
>
> I'm still confused about what issue you are worried about.  I believe that
> everyone who is capable of LGTM'ing a change in that directory will consult
> with the OWNERS in that directory before approving such a change.  You
> don't?
>
> -Darin
>

Scratch that, I think I see the problem.  src/build/OWNERS lists "*", so
indeed we are not ready to strip "set noparent" from subdirectories of
src/build/.  Sorry, I missed that detail!

-Darin


>
>
>
>>
https://chromiumcodereview.**appspot.com/11191038/<https://chromiumcodereview...
>>
>
>

[darin (slow to review), 2012-10-18 19:27:48]

Dirk, the same problem appears to happen with subdirectories of src/tools.  This
CL should only strip 'set noparent' from OWNERS files, for which all parent
OWNERS do not include "*".

[darin (slow to review), 2012-10-18 19:28:42]

On 2012/10/18 19:27:48, darin wrote:
> Dirk, the same problem appears to happen with subdirectories of src/tools. 
This
> CL should only strip 'set noparent' from OWNERS files, for which all parent
> OWNERS do not include "*".

Ditto for subdirectories of chrome/test/data/

[Dirk Pranke, 2012-10-18 19:36:02]

On 2012/10/18 19:28:42, darin wrote:
> On 2012/10/18 19:27:48, darin wrote:
> > Dirk, the same problem appears to happen with subdirectories of src/tools. 
> This
> > CL should only strip 'set noparent' from OWNERS files, for which all parent
> > OWNERS do not include "*".
> 
> Ditto for subdirectories of chrome/test/data/

Yeah, that's what I was talking about in comment #11 re: src/build. I think
src/build probably shouldn't have a "*" at all, but it probably makes sense for
src/tools and chrome/test/data to keep the "*".  

I'll update the change to put the "set noparent" back in for the directories in
question for now.

[Dirk Pranke, 2012-10-18 19:39:00]

On 2012/10/18 19:36:02, Dirk Pranke wrote:
> I'll update the change to put the "set noparent" back in for the directories
in
> question for now.

Updated. All good?

[Isaac (away), 2012-10-18 19:50:33]

On 2012/10/18 19:39:00, Dirk Pranke wrote:
> On 2012/10/18 19:36:02, Dirk Pranke wrote:
> > I'll update the change to put the "set noparent" back in for the directories
> in
> > question for now.
> 
> Updated. All good?

Lgtm, thanks

[commit-bot: I haz the power, 2012-10-18 19:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/11191038/10002

[commit-bot: I haz the power, 2012-10-18 19:57:06]

Failed to apply patch for ui/views/OWNERS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/OWNERS
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file ui/views/OWNERS.rej

Patch:       ui/views/OWNERS
Index: ui/views/OWNERS
diff --git a/ui/views/OWNERS b/ui/views/OWNERS
index
3bd2917703da09d972147187973480c3391d1bba..8e95571acbe5c3a16e8b2e4a690da69e05e9f47a
100644
--- a/ui/views/OWNERS
+++ b/ui/views/OWNERS
@@ -1,4 +1,3 @@
-set noparent
 ben@chromium.org
 sadrul@chromium.org
 sky@chromium.org

[darin (slow to review), 2012-10-18 20:09:41]

LGTM

[commit-bot: I haz the power, 2012-10-18 22:23:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/11191038/2006

[commit-bot: I haz the power, 2012-10-18 22:23:41]

Failed to apply patch for ui/views/OWNERS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/OWNERS
  Hunk #1 FAILED at 1.
  1 out of 1 hunk FAILED -- saving rejects to file ui/views/OWNERS.rej

Patch:       ui/views/OWNERS
Index: ui/views/OWNERS
diff --git a/ui/views/OWNERS b/ui/views/OWNERS
index
3bd2917703da09d972147187973480c3391d1bba..8e95571acbe5c3a16e8b2e4a690da69e05e9f47a
100644
--- a/ui/views/OWNERS
+++ b/ui/views/OWNERS
@@ -1,4 +1,3 @@
-set noparent
 ben@chromium.org
 sadrul@chromium.org
 sky@chromium.org

[joth, 2012-10-20 19:08:47]

https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS
File android_webview/lib/OWNERS (left):

https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS...
android_webview/lib/OWNERS:3: set noparent
this one was intentional (as per the comment above it :)

[Dirk Pranke, 2012-10-20 21:05:17]

On 2012/10/20 19:08:47, joth wrote:
> https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS
> File android_webview/lib/OWNERS (left):
> 
>
https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS...
> android_webview/lib/OWNERS:3: set noparent
> this one was intentional (as per the comment above it :)

The previous discussions on chromium-dev have concluded that we don't want to
encourage this kind of restriction. there's only four other people listed as
owners in android_webview; they don't know to defer reviews to one of you two to
approve?

[joth, 2012-10-21 01:50:07]

On 20 October 2012 14:05, <dpranke@chromium.org> wrote:

> On 2012/10/20 19:08:47, joth wrote:
>
>> https://codereview.chromium.**org/11191038/diff/2006/**
>>
android_webview/lib/OWNERS<https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS>
>> File android_webview/lib/OWNERS (left):
>>
>
>
> https://codereview.chromium.**org/11191038/diff/2006/**
>
android_webview/lib/OWNERS#**oldcode3<https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS#oldcode3>
>
>> android_webview/lib/OWNERS:3: set noparent
>> this one was intentional (as per the comment above it :)
>>
>
> The previous discussions on chromium-dev have concluded that we don't want
> to
> encourage this kind of restriction. there's only four other people listed
> as
> owners in android_webview; they don't know to defer reviews to one of you
> two to
> approve?
>
>
when the top-level folder was setup, beng & darin specifically requested
(required) we use OWNERS on the small piece with chrome dependency (this
folder) to set a very restricted set of approvers compared to the rest of
the code in the module.
we're aiming to remove the chrome dep soon anyway, at which point this will
be moot.

[Dirk Pranke, 2012-10-21 03:42:18]

On Sat, Oct 20, 2012 at 6:49 PM, Jonathan Dixon <joth@chromium.org> wrote:
>
>
>
> On 20 October 2012 14:05, <dpranke@chromium.org> wrote:
>>
>> On 2012/10/20 19:08:47, joth wrote:
>>>
>>>
>>>
https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS
>>> File android_webview/lib/OWNERS (left):
>>
>>
>>
>>
>>
https://codereview.chromium.org/11191038/diff/2006/android_webview/lib/OWNERS...
>>>
>>> android_webview/lib/OWNERS:3: set noparent
>>> this one was intentional (as per the comment above it :)
>>
>>
>> The previous discussions on chromium-dev have concluded that we don't want
>> to
>> encourage this kind of restriction. there's only four other people listed
>> as
>> owners in android_webview; they don't know to defer reviews to one of you
>> two to
>> approve?
>>
>
> when the top-level folder was setup, beng & darin specifically requested
> (required) we use OWNERS on the small piece with chrome dependency (this
> folder) to set a very restricted set of approvers compared to the rest of
> the code in the module.
> we're aiming to remove the chrome dep soon anyway, at which point this will
> be moot.
>

alrighty then :).

-- Dirk