Add first changes needed for OWNERS file support.

Add first changes needed for OWNERS file support.

This changes adds the first pass of code needed for OWNERS files.
In total there should probably be maybe four user-visible changes:
  * new gcl/git-cl "suggest-reviewers" command
  * a presubmit hook on upload to automatically add the reviewers
  * an addition to gcl/git-cl status command that tells you
    which files still need review/approval.
  * a presubmit hook on commit to ensure all of the needed reviewers
    have approved the file.

This change implements a core "owners Database" object with the
dumbest possible algorithm for determining a covering set of reviewers,
and the skeleton of the presubmit hooks. This code will not be
used by anything yet, and is also missing unit tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76342

[M-A Ruel, 2011-02-24 16:25:57]

This change will break tests.

http://codereview.chromium.org/6581030/diff/1/owners.py
File owners.py (right):

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode1
owners.py:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode8
owners.py:8: import pdb
remove

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode15
owners.py:15: def __init__(self, root, all_owners=None,
Either all align one per line or all packed at 80 cols.

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode18
owners.py:18: """Initialize the database of owners for a repository.
Initializes

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode52
owners.py:52: return len(self.not_covered(files, reviewers)) == 0
return not self.not_covered(files, reviewers)

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode82
owners.py:82: raise Assertion('No OWNERS file found for %s' % f)
Is this really an exceptional event? I think it should just return None. If so,
you can remove the class Assertion.

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode86
owners.py:86: for owner in self.open(owner_file).readlines():
.readlines() is implied.

http://codereview.chromium.org/6581030/diff/1/owners.py#newcode98
owners.py:98: return [all_owners]
Did you mean:
return list(all_owners)
?

http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py#newc...
presubmit_canned_checks.py:648: return ['maruel']
I know it's an example but it should use fully qualified email?

http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py#newc...
presubmit_canned_checks.py:650: def _AddReviewers(input_api):
This will need to be in presubmit_support.Change.

http://codereview.chromium.org/6581030/diff/1/presubmit_support.py
File presubmit_support.py (right):

http://codereview.chromium.org/6581030/diff/1/presubmit_support.py#newcode259
presubmit_support.py:259: # TODO(dpranke): figure out a list of all approved
owners for a repo
I think owners are to respect to a specific Change. I don't think creating the
db is slow enough to warrant caching it in InputApi.

[Dirk Pranke, 2011-02-25 01:17:39]

On 2011/02/24 16:25:57, Marc-Antoine Ruel wrote:
> This change will break tests.
> 

I will attempt to fix them :) Is running the presubmit hooks sufficient to run
the tests?

> http://codereview.chromium.org/6581030/diff/1/owners.py
> File owners.py (right):
> 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode1
> owners.py:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
> 2011
> 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode8
> owners.py:8: import pdb
> remove
>

Done. whoops ;)
 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode15
> owners.py:15: def __init__(self, root, all_owners=None,
> Either all align one per line or all packed at 80 cols.
>

Done.
 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode18
> owners.py:18: """Initialize the database of owners for a repository.
> Initializes
>

Done.
 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode52
> owners.py:52: return len(self.not_covered(files, reviewers)) == 0
> return not self.not_covered(files, reviewers)
>

Done.
 
> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode82
> owners.py:82: raise Assertion('No OWNERS file found for %s' % f)
> Is this really an exceptional event? I think it should just return None. If
so,
> you can remove the class Assertion.
> 

It should be exceptional; it's not clear what we're supposed to do if we don't
find a top-level OWNERS file. We can change this down the road if we need to,
though.

> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode86
> owners.py:86: for owner in self.open(owner_file).readlines():
> .readlines() is implied.
> 

Okay.

> http://codereview.chromium.org/6581030/diff/1/owners.py#newcode98
> owners.py:98: return [all_owners]
> Did you mean:
> return list(all_owners)
> ?
> 

No. In theory there could be multiple combinations of reviewers that would be
sufficient to approve a change, and this routine will return some number of
combinations. For now, we'll only return one combination that lists *every
owner*, which is sufficient but more coverage than we'll likely need. 

Coding it this way allows me to not have to change the client's code when I
implement a smarter algorithm here.

> http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
> 
>
http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py#newc...
> presubmit_canned_checks.py:648: return ['maruel']
> I know it's an example but it should use fully qualified email?
> 

Good question. When I wrote this, I wasn't sure how we would qualify the names,
but it does look like they should be full email addresses.

>
http://codereview.chromium.org/6581030/diff/1/presubmit_canned_checks.py#newc...
> presubmit_canned_checks.py:650: def _AddReviewers(input_api):
> This will need to be in presubmit_support.Change.
>

Okay.
 
> http://codereview.chromium.org/6581030/diff/1/presubmit_support.py
> File presubmit_support.py (right):
> 
> http://codereview.chromium.org/6581030/diff/1/presubmit_support.py#newcode259
> presubmit_support.py:259: # TODO(dpranke): figure out a list of all approved
> owners for a repo
> I think owners are to respect to a specific Change. I don't think creating the
> db is slow enough to warrant caching it in InputApi.

I'm not sure what you were getting at here. The "all_owners" idea was intended
to capture the idea of a "*" wildcard that indicated that anyone was allowed to
approve it. However, this concept was only half-thought out so far, and I wasn't
sure that pulling a full list of committers from somewhere was the right way to
try and do this. Currently, I don't think it is, so I'll rework this.

If that's not what you were getting at, let me know.

[M-A Ruel, 2011-02-25 20:41:54]

On 2011/02/25 01:17:39, dpranke wrote:
> On 2011/02/24 16:25:57, Marc-Antoine Ruel wrote:
> > http://codereview.chromium.org/6581030/diff/1/owners.py#newcode82
> > owners.py:82: raise Assertion('No OWNERS file found for %s' % f)
> > Is this really an exceptional event? I think it should just return None. If
> so,
> > you can remove the class Assertion.
> > 
> 
> It should be exceptional; it's not clear what we're supposed to do if we don't
> find a top-level OWNERS file. We can change this down the road if we need to,
> though.

In practice you are right, since the check should only be run for repositories
already containing OWNERS files.


> > http://codereview.chromium.org/6581030/diff/1/owners.py#newcode98
> > owners.py:98: return [all_owners]
> > Did you mean:
> > return list(all_owners)
> > ?
> > 
> 
> No. In theory there could be multiple combinations of reviewers that would be
> sufficient to approve a change, and this routine will return some number of
> combinations. For now, we'll only return one combination that lists *every
> owner*, which is sufficient but more coverage than we'll likely need. 

Ok, I had misunderstood.


> > http://codereview.chromium.org/6581030/diff/1/presubmit_support.py
> > File presubmit_support.py (right):
> > 
> >
http://codereview.chromium.org/6581030/diff/1/presubmit_support.py#newcode259
> > presubmit_support.py:259: # TODO(dpranke): figure out a list of all approved
> > owners for a repo
> > I think owners are to respect to a specific Change. I don't think creating
the
> > db is slow enough to warrant caching it in InputApi.
> 
> I'm not sure what you were getting at here. The "all_owners" idea was intended
> to capture the idea of a "*" wildcard that indicated that anyone was allowed
to
> approve it. However, this concept was only half-thought out so far, and I
wasn't
> sure that pulling a full list of committers from somewhere was the right way
to
> try and do this. Currently, I don't think it is, so I'll rework this.
> 
> If that's not what you were getting at, let me know.

Don't bother, I don't really have an opinion on that.

---

lgtm

http://codereview.chromium.org/6581030/diff/12001/owners.py
File owners.py (right):

http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode1
owners.py:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
2011

http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode12
owners.py:12: """Initializes the database of owners for a repository.
Personal nit: constructor docstring are usually useless, if one it needed, it's
usually at the class level. Args are fine to document but saying that an
__init__ function initializes is a pleonasm, but I digress.

http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode57
owners.py:57: def _LoadOwnersFor(self, f):
Is this function really needed? It's called only once so it's probably better to
inline since both functions are rather short. It's fine if you prefer it this
way.

[Dirk Pranke, 2011-03-01 00:22:21]

On 2011/02/25 20:41:54, Marc-Antoine Ruel wrote:
> On 2011/02/25 01:17:39, dpranke wrote:
> > On 2011/02/24 16:25:57, Marc-Antoine Ruel wrote:
> > > http://codereview.chromium.org/6581030/diff/1/owners.py#newcode82
> > > owners.py:82: raise Assertion('No OWNERS file found for %s' % f)
> > > Is this really an exceptional event? I think it should just return None.
If
> > so,
> > > you can remove the class Assertion.
> > > 
> > 
> > It should be exceptional; it's not clear what we're supposed to do if we
don't
> > find a top-level OWNERS file. We can change this down the road if we need
to,
> > though.
> 
> In practice you are right, since the check should only be run for repositories
> already containing OWNERS files.
> 
> 
> > > http://codereview.chromium.org/6581030/diff/1/owners.py#newcode98
> > > owners.py:98: return [all_owners]
> > > Did you mean:
> > > return list(all_owners)
> > > ?
> > > 
> > 
> > No. In theory there could be multiple combinations of reviewers that would
be
> > sufficient to approve a change, and this routine will return some number of
> > combinations. For now, we'll only return one combination that lists *every
> > owner*, which is sufficient but more coverage than we'll likely need. 
> 
> Ok, I had misunderstood.
> 

Actually, it was confusing. I changed it to just return a single set in the most
recent rev. I'll change the code again when we support multiple sets.

> 
> > > http://codereview.chromium.org/6581030/diff/1/presubmit_support.py
> > > File presubmit_support.py (right):
> > > 
> > >
> http://codereview.chromium.org/6581030/diff/1/presubmit_support.py#newcode259
> > > presubmit_support.py:259: # TODO(dpranke): figure out a list of all
approved
> > > owners for a repo
> > > I think owners are to respect to a specific Change. I don't think creating
> the
> > > db is slow enough to warrant caching it in InputApi.
> > 
> > I'm not sure what you were getting at here. The "all_owners" idea was
intended
> > to capture the idea of a "*" wildcard that indicated that anyone was allowed
> to
> > approve it. However, this concept was only half-thought out so far, and I
> wasn't
> > sure that pulling a full list of committers from somewhere was the right way
> to
> > try and do this. Currently, I don't think it is, so I'll rework this.
> > 
> > If that's not what you were getting at, let me know.
> 
> Don't bother, I don't really have an opinion on that.
> 
> ---
> 
> lgtm
> 
> http://codereview.chromium.org/6581030/diff/12001/owners.py
> File owners.py (right):
> 
> http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode1
> owners.py:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved.
> 2011
> 
> http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode12
> owners.py:12: """Initializes the database of owners for a repository.
> Personal nit: constructor docstring are usually useless, if one it needed,
it's
> usually at the class level. Args are fine to document but saying that an
> __init__ function initializes is a pleonasm, but I digress.
>

True enough.
 
> http://codereview.chromium.org/6581030/diff/12001/owners.py#newcode57
> owners.py:57: def _LoadOwnersFor(self, f):
> Is this function really needed? It's called only once so it's probably better
to
> inline since both functions are rather short. It's fine if you prefer it this
> way.

Yeah, you could inline most of the functions in that class with no downside;
I've gotten used to the webkit python style of lots of little functions.

[M-A Ruel, 2011-03-01 16:40:56]

lgtm

[M-A Ruel, 2011-03-01 16:41:07]

On 2011/03/01 16:40:56, Marc-Antoine Ruel wrote:
> lgtm

(err, ignore me)