set noparent and OWNERS

[Ben Goodger (Google)]

Background:

A little while ago, I realized that despite the OWNERS file in chrome/browser/ui, I wasn't seeing enough reviews. I realized the OWNERS file wasn't taking effect because the only OWNERS file above it in src/ had * as its contents. So I added set noparent and lo, began receiving reviews. Without thinking about it very hard, I also added set noparent to some subdirectories under chrome/browser/ui, so that the tooling would automatically enforce using those reviewers instead of sending extra stuff to sky and myself.

I realize now that I was over-zealous, and that having nested set noparent is a bad thing - it's making it harder to pursue refactoring changes by installing a lot of bureaucracy into the flow.

Darin and I discussed this, and after reviewing several of the discussions that have occurred on this topic between members of the team reached an agreement on the use of set noparent:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent. Doing so implies a lack of trust in reviewers at a higher level, which does not exist in the chromium project.

Coupled with this, contributors should take care to send their changes to the most tightly enclosing OWNERS where it makes sense. I am not going to start providing great feedback on Gtk or Cocoa changes for example.

With this in mind, we'll roll back my over-zealous application of set noparent shortly.

-Ben

[Peter Kasting]

On Tue, Feb 28, 2012 at 12:22 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent.

I assume this means we are abandoning the plan to explicitly or implicitly include topmost-level OWNERS in all subdirectories?  This was the previous plan to deal with excess bureaucracy caused by set noparent, based on the google3 solution.

Coupled with this, contributors should take care to send their changes to the most tightly enclosing OWNERS where it makes sense.

Are there any plans to improve our tooling to make this as easy as possible, and make it harder to accidentally send requests to the wrong owners?  In the time since we introduced OWNERS it seems like the negatives have outweighed the positives in some ways, and I wonder if a lot of that is due to us not building in more comprehensive support for them in our tooling.

With this in mind, we'll roll back my over-zealous application of set noparent shortly.

There are a large number of other set noparents in the OWNERS files.  Will you be fixing the rest as well?  If not what is the plan to bring them in line with the above decisions?

PK 

[Paweł Hajdan, Jr.]

On Tue, Feb 28, 2012 at 21:22, Ben Goodger (Google) <b...@chromium.org> wrote:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent. Doing so implies a lack of trust in reviewers at a higher level, which does not exist in the chromium project.

How about adding automated checks for those two common mistakes?

1. OWNERS file with no "set noparent" while nearest enclosing OWNERS file is *

2. OWNERS file with "set noparent" while nearest enclosing OWNERS file is not *

It could even be made completely implicit, i.e. remove all "set noparent" strings from the OWNERS file, just make the checking tool interpret it the right way.

[Ben Goodger (Google)]

+dpranke

Great ideas.

-Ben

[John Abd-El-Malek]

On Tue, Feb 28, 2012 at 12:30 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 28, 2012 at 12:22 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent.

I assume this means we are abandoning the plan to explicitly or implicitly include topmost-level OWNERS in all subdirectories?  This was the previous plan to deal with excess bureaucracy caused by set noparent, based on the google3 solution.

We still need to add top level owners. The main issue I see here is that no one will want to be the suckers in the chrome\browser directory because of the volume and disorganization. That code needs to be separated more.

 

Coupled with this, contributors should take care to send their changes to the most tightly enclosing OWNERS where it makes sense.

Are there any plans to improve our tooling to make this as easy as possible, and make it harder to accidentally send requests to the wrong owners?  In the time since we introduced OWNERS it seems like the negatives have outweighed the positives in some ways, and I wonder if a lot of that is due to us not building in more comprehensive support for them in our tooling.

I disagree with this assertion. I've seen the OWNERS system lead to better code quality in the directories where it's used, by preventing changes that are not reviewed by the folks most familiar with it.

 

With this in mind, we'll roll back my over-zealous application of set noparent shortly.

There are a large number of other set noparents in the OWNERS files.  Will you be fixing the rest as well?  If not what is the plan to bring them in line with the above decisions?

I think everyone should feel free to go ahead and fix this. I have a change for chrome\browser\ui subdirectories in http://codereview.chromium.org/9481010/

Looking at cs, most of the other "set noparent" are there because the top level directory specifies "*" as owners. So perhaps a way forward can be to add top level owners instead of everyone, and just make chrome\browser "*" for now while we figure out who should go there. The rest of the directories under top-level either all have owners, or it's mostly trivial to add them.

PK 

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Tue, Feb 28, 2012 at 12:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Feb 28, 2012 at 12:30 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 28, 2012 at 12:22 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent.

I assume this means we are abandoning the plan to explicitly or implicitly include topmost-level OWNERS in all subdirectories?  This was the previous plan to deal with excess bureaucracy caused by set noparent, based on the google3 solution.

We still need to add top level owners. The main issue I see here is that no one will want to be the suckers in the chrome\browser directory because of the volume and disorganization. That code needs to be separated more.

I don't think I understand what you're saying.  I don't know if you're talking about the same issue I am.  I'm just noting that if our plan is to ultimately remove all set noparent then that seems to accomplish the goal of making global refactorings possible with one OWNER approval from a topmost-level OWNER, because those topmost OWNERS will own everything down the tree.  So I think we don't need to build anything special like an "include directive" or something to accomplish this like we had talked about doing.  I'm making sure that's correct.

Maybe part of the confusion is that I don't know what you mean by "top level".  I try to explicitly say "topmost" to make it clear that I mean "src/OWNERS".  I think maybe you are referring to directories one level down from that or something.  I'm confused.

Are there any plans to improve our tooling to make this as easy as possible, and make it harder to accidentally send requests to the wrong owners?  In the time since we introduced OWNERS it seems like the negatives have outweighed the positives in some ways, and I wonder if a lot of that is due to us not building in more comprehensive support for them in our tooling.

I disagree with this assertion. I've seen the OWNERS system lead to better code quality in the directories where it's used, by preventing changes that are not reviewed by the folks most familiar with it.

OK, putting aside the question of precisely what good and harm has happened so far, since that wasn't my point, can I reiterate my question: are there any plans to put more work into tooling around these?  If not, can we make some?

PK

[John Abd-El-Malek]

On Tue, Feb 28, 2012 at 12:52 PM, Peter Kasting <pkas...@google.com> wrote:

On Tue, Feb 28, 2012 at 12:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Feb 28, 2012 at 12:30 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 28, 2012 at 12:22 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

set noparent should be used only when the nearest enclosing OWNERS file is *, i.e. there should never be nested OWNERS files with set noparent.

I assume this means we are abandoning the plan to explicitly or implicitly include topmost-level OWNERS in all subdirectories?  This was the previous plan to deal with excess bureaucracy caused by set noparent, based on the google3 solution.

We still need to add top level owners. The main issue I see here is that no one will want to be the suckers in the chrome\browser directory because of the volume and disorganization. That code needs to be separated more.

I don't think I understand what you're saying.  I don't know if you're talking about the same issue I am.  I'm just noting that if our plan is to ultimately remove all set noparent then that seems to accomplish the goal of making global refactorings possible with one OWNER approval from a topmost-level OWNER, because those topmost OWNERS will own everything down the tree.

The number of top level owners will be very small of course. So it'll be useful when touching all the files under chrome\browser\ui to have someone who's not in the top level owners, but still covers all the subdirectories, give one lgtm.

From what I've been told about top level owners in google3, it's used to:

-approve one-off changes that touch a lot of code, i.e. because a function in base is used all over

-temporarily add people who're doing refactorings that span many changes

It's expected that whoever uses a lgtm from the people above will make sure all their changes are properly reviewed. A nice property about this system is that the people added temporarily have a strong incentive to make sure they follow this, or else they'll be removed. This is different from the current system of forcing, because a person can keep forcing even when they shouldn't, and the only alternative is to remove their commit rights, which we have never done afaik.

 

 So I think we don't need to build anything special like an "include directive" or something to accomplish this like we had talked about doing.  I'm making sure that's correct.

Yeah I think the "include directive" won't be needed.

I think there's no valid use case for "set noparent" in the chrome code, once top level doesn't have "*" anymore. In google3 they are discouraged, but there are legitimate use cases there that don't exist in chrome (IM me for more details).

Maybe part of the confusion is that I don't know what you mean by "top level".  I try to explicitly say "topmost" to make it clear that I mean "src/OWNERS".  I think maybe you are referring to directories one level down from that or something.  I'm confused.

I'm referring to the same file as you (src/OWNERS).

[Peter Kasting]

On Tue, Feb 28, 2012 at 1:04 PM, John Abd-El-Malek <j...@chromium.org> wrote:

The number of top level owners will be very small of course. So it'll be useful when touching all the files under chrome\browser\ui to have someone who's not in the top level owners, but still covers all the subdirectories, give one lgtm.

Right, and those would be the people we'd list in chrome/browser/ui/OWNERS.  Right?

To be clear, the overall plan is:

(1) Remove all "set noparent" on everywhere except subdirs of a dir with "*" (this email thread)

(2) Replace all the "*"s with actual OWNERS (already planned, unrelated to this thread).  This consequently removes all remaining set noparents.

(3) Remove support for set noparent from the system (no one has said this but it seems obvious from the previous two)

I think you were originally saying you still wanted to see (2) happen, which I do too.  I was just saying that this new step (1) replaces what was going to be a different step (1) of "add support for #include or else manually insert a couple names into all OWNERS files with set noparent".

PK

[John Abd-El-Malek]

On Tue, Feb 28, 2012 at 1:18 PM, Peter Kasting <pkas...@google.com> wrote:

On Tue, Feb 28, 2012 at 1:04 PM, John Abd-El-Malek <j...@chromium.org> wrote:

The number of top level owners will be very small of course. So it'll be useful when touching all the files under chrome\browser\ui to have someone who's not in the top level owners, but still covers all the subdirectories, give one lgtm.

Right, and those would be the people we'd list in chrome/browser/ui/OWNERS.  Right?

Yep (just to be overly verbose, there'll be more people there than in src/OWNERS).

To be clear, the overall plan is:

(1) Remove all "set noparent" on everywhere except subdirs of a dir with "*" (this email thread)

(2) Replace all the "*"s with actual OWNERS (already planned, unrelated to this thread).  This consequently removes all remaining set noparents.

(3) Remove support for set noparent from the system (no one has said this but it seems obvious from the previous two)

I think you were originally saying you still wanted to see (2) happen, which I do too.  I was just saying that this new step (1) replaces what was going to be a different step (1) of "add support for #include or else manually insert a couple names into all OWNERS files with set noparent".

Yep, I don't see a reason why that would be needed anymore.

PK

[Ami Fischman]

While Peter's 1-3 SGTM, I'd like it if inclusion weren't taken off the table, to support feature teams whose OWNERS end up sprinkled around the codebase, with drift/rot setting in over time.  E.g. there are 7 .../media/OWNERS files in the codebase, each of which should be copies of src/media/OWNERS, but they're not.

If file: were supported all but one of these files could be a pointer to the canonical one.

But maybe media is atypical.

Cheers,

-a

[Dirk Pranke]

On Tue, Feb 28, 2012 at 12:30 PM, Peter Kasting <pkas...@chromium.org> wrote:

Clearly this whole issue is just stuck on lack of follow-through to
remove the topmost "*" and make sure the immediate subdirectories like
chrome are correctly populated.

Unless anyone objects, I will make this happen in the next few days. I
will remove the "set noparent" lines at the same time.

On Tue, Feb 28, 2012 at 12:52 PM, Peter Kasting <pkas...@google.com> wrote:
> can I reiterate my question: are there any plans to put more work into tooling
> around these?  If not, can we make some?

There are not, but this is solely because of a perceived lack of demand :)

I am planning to add better support for suggesting owners soon as a
part of this discussion, and, to speak to Ami's point, I will look
into adding #include for the media (and perhaps other) dirs as well.
It sounds like I should remove support for "set noparent" as well.

Lastly, there has also been a request to add per-file OWNERS so that
we don't have to put a file into its own directory just to get a
tighter (or looser) list of owners.

If there are other things missing or desired, please speak up :)

-- Dirk

[Rahul Chaturvedi]

Removing support for set noparent is probably not a good idea.

Consider this case,

BaseDirForFeatureX

  --SubDirForFeatureXComponentA

  --SubDirForFeatureXComponentB

  --SubDirForFeatureXComponentC

  --SubDirForFeatureXComponentD

  --SubDirForFeatureXComponentE

  --SubDirForFeatureXComponentF

If an owner (or set of owners) is familiar with components A, B, D, E and F but not C; he'd be on the OWNERS file for the base DIR. For FeatureXComponentC, we would have a separate OWNERS file with people familiar with that component and set noparent.

With support for noparent gone, we'll have to add OWNERS files for each of the components separately and 'maintain' each of them; the latter being the painful part.

/rkc

[Peter Kasting]

On Tue, Feb 28, 2012 at 3:02 PM, Rahul Chaturvedi <r...@google.com> wrote:

If an owner (or set of owners) is familiar with components A, B, D, E and F but not C; he'd be on the OWNERS file for the base DIR. For FeatureXComponentC, we would have a separate OWNERS file with people familiar with that component and set noparent.

The whole point of this thread is that we are explicitly banning this.

 

With support for noparent gone, we'll have to add OWNERS files for each of the components separately and 'maintain' each of them; the latter being the painful part.

Not necessarily.  Just have your separate OWNERS file for the component C dir without set noparent in it.  This informs anyone looking that those are the people familiar with component C.  Your unfamiliar owner is expected, as part of his responsibilities as a parent dir owner, to not approve changes to component C with which he is insufficiently familiar.  This hasn't changed; all that's changing is that we're removing the mechanism by which the tools can be used to enforce this.  (That's part of why I asked for better tooling support given that we're making this change.)

PK