Clean up the parsing of approvals for OWNERS checks.

Clean up the parsing of approvals for OWNERS checks.

This fixes bugs 76724. We will now allow approvals from
only non-owners to be sufficient if the patch is from an OWNER.

Also, this strips out the code for suggesting reviewers during upload completely. I've come to believe that this should be done by gcl and git-cl directly rather than through a presubmit hook, when the change is being initially created. CheckOwners() is now a no-op on upload.

(We might need to add a new value to the codereview.settings file instead to indicate if we want this to happen).

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

[Dirk Pranke, 2011-03-24 03:46:38]

Marc-Antoine,

Among other things, this change restructures the parsing logic for LGTM
approval. I've split out the code so that we can re-use it in the CQ verifier if
you so desire, but I"m not sure where to put the code so that it is more easily
sharable.

[M-A Ruel, 2011-03-24 15:09:23]

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

http://codereview.chromium.org/6730020/diff/1/presubmit_canned_checks.py#newc...
presubmit_canned_checks.py:653: return [output_api.PresubmitError('Missing LGTM
from someone other than '
nit: I prefer this style
return [output_api.PresubmitError(
    'Missing LGTM from someone other than %s' % owner)]

First, it doesn't throw is owner is a list, a None or Whatever else error could
happen.
Then it's easier to read the string if it's on one line.

http://codereview.chromium.org/6730020/diff/1/presubmit_canned_checks.py#newc...
presubmit_canned_checks.py:688: if (l.startswith('lgtm') or l.startswith('lg')
or
I disagree in two ways:

I prefer to accept lgtm anywhere in the sentence. The only exception would be to
filter out lines starting with '>'.

I prefer to not accept lg, looks good or whatever else.

Follow up comments already remove the commit bit, I'm not sure it's useful to
affect owners check in any way.

http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py#new...
tests/presubmit_unittest.py:1886: def AssertOwnersWorks(self, tbr=False,
issue='1', approvers=set(),
(even if it's just a unit test, I don't want to encourage this problem and in
practice pylint should fail on that)

approvers=None

and then:

approvers = approvers or set()

same for uncovered_files.

http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py#new...
tests/presubmit_unittest.py:1932: if approvers is None:
approvers = approvers or set(['ben@example.com'])
?

[M-A Ruel, 2011-03-24 15:10:00]

And the reason I don't like the lg & looks good is that the check is done client
side and not server side. If this was implemented on Rietveld I wouldn't mind.

[M-A Ruel, 2011-03-24 15:10:16]

On 2011/03/24 15:10:00, Marc-Antoine Ruel wrote:
> And the reason I don't like the lg & looks good is that the check is done
client
> side and not server side. If this was implemented on Rietveld I wouldn't mind.

-> this would be easy to expose from the json api.

[Dirk Pranke, 2011-03-24 19:22:09]

On Thu, Mar 24, 2011 at 8:10 AM,  <maruel@chromium.org> wrote:
> On 2011/03/24 15:10:00, Marc-Antoine Ruel wrote:
>>
>> And the reason I don't like the lg & looks good is that the check is done
>
> client
>>
>> side and not server side. If this was implemented on Rietveld I wouldn't
>> mind.
>
> -> this would be easy to expose from the json api.
>

Why is server-side better in your opinion?



> http://codereview.chromium.org/6730020/
>

[M-A Ruel, 2011-03-24 19:24:25]

On 2011/03/24 19:22:09, dpranke wrote:
> On Thu, Mar 24, 2011 at 8:10 AM,  <mailto:maruel@chromium.org> wrote:
> > On 2011/03/24 15:10:00, Marc-Antoine Ruel wrote:
> >>
> >> And the reason I don't like the lg & looks good is that the check is done
> >
> > client
> >>
> >> side and not server side. If this was implemented on Rietveld I wouldn't
> >> mind.
> >
> > -> this would be easy to expose from the json api.
> >
> 
> Why is server-side better in your opinion?

Because there isn't a copy of the brittle regexps on every dev's workstation.
Also because conceptually, the review website is the place to declare if a
review was completed or not, mostly by definition.

M-A

[Dirk Pranke, 2011-03-24 19:28:01]

On Thu, Mar 24, 2011 at 8:09 AM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/6730020/diff/1/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
>
>
http://codereview.chromium.org/6730020/diff/1/presubmit_canned_checks.py#newc...
> presubmit_canned_checks.py:653: return
> [output_api.PresubmitError('Missing LGTM from someone other than '
> nit: I prefer this style
> return [output_api.PresubmitError(
>    'Missing LGTM from someone other than %s' % owner)]
>
> First, it doesn't throw is owner is a list, a None or Whatever else
> error could happen.
> Then it's easier to read the string if it's on one line.
>

Fair enough.

>
http://codereview.chromium.org/6730020/diff/1/presubmit_canned_checks.py#newc...
> presubmit_canned_checks.py:688: if (l.startswith('lgtm') or
> l.startswith('lg') or
> I disagree in two ways:
>
> I prefer to accept lgtm anywhere in the sentence. The only exception
> would be to filter out lines starting with '>'.
>
> I prefer to not accept lg, looks good or whatever else.
>

This is not-consistent with what is done inside the GOOG, but I can
leave it the way it is (as checked-in) for now and we can always add
this later if there's demand.

> Follow up comments already remove the commit bit, I'm not sure it's
> useful to affect owners check in any way.
>
> http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py
> File tests/presubmit_unittest.py (right):
>
>
http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py#new...
> tests/presubmit_unittest.py:1886: def AssertOwnersWorks(self, tbr=False,
> issue='1', approvers=set(),
> (even if it's just a unit test, I don't want to encourage this problem
> and in practice pylint should fail on that)
>
> approvers=None
>
> and then:
>
> approvers = approvers or set()
>
> same for uncovered_files.
>

Sure.

>
http://codereview.chromium.org/6730020/diff/1/tests/presubmit_unittest.py#new...
> tests/presubmit_unittest.py:1932: if approvers is None:
> approvers = approvers or set(['ben@example.com'])
> ?

No, this will not work if you intentionally want an empty set.

>
> http://codereview.chromium.org/6730020/
>

[M-A Ruel, 2011-03-24 19:46:37]

can you upload a new patchset?

[Dirk Pranke, 2011-03-24 19:49:07]

Yup, just haven't gotten to it yet :)

-- Dirk

On Thu, Mar 24, 2011 at 12:46 PM,  <maruel@chromium.org> wrote:
> can you upload a new patchset?
>
> http://codereview.chromium.org/6730020/
>

[Dirk Pranke, 2011-03-24 20:23:56]

updated.

[M-A Ruel, 2011-03-24 20:27:19]

lgtm

http://codereview.chromium.org/6730020/diff/9/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/6730020/diff/9/presubmit_canned_checks.py#newc...
presubmit_canned_checks.py:693: if 'lgtm' in l:
nit: Should we strip lines starting with '>' ?

[Dirk Pranke, 2011-03-24 20:30:50]

That would be fine.

Also, there's gonna be at least one more patch for this; it looks like
rietveld doesn't always return a full email address for the 'owner'
field. Is it safe to assume that a bare userid maps to
userid@chromium.org?

-- Dirk

On Thu, Mar 24, 2011 at 1:27 PM,  <maruel@chromium.org> wrote:
> lgtm
>
>
>
> http://codereview.chromium.org/6730020/diff/9/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
>
>
http://codereview.chromium.org/6730020/diff/9/presubmit_canned_checks.py#newc...
> presubmit_canned_checks.py:693: if 'lgtm' in l:
> nit: Should we strip lines starting with '>' ?
>
> http://codereview.chromium.org/6730020/
>

[Dirk Pranke, 2011-03-24 20:32:36]

On Thu, Mar 24, 2011 at 1:30 PM, Dirk Pranke <dpranke@chromium.org> wrote:
> That would be fine.
>
> Also, there's gonna be at least one more patch for this; it looks like
> rietveld doesn't always return a full email address for the 'owner'
> field. Is it safe to assume that a bare userid maps to
> userid@chromium.org?
>

Hm. Answering my own question, no. How can I get the email address for the user?

-- Dirk

[Dirk Pranke, 2011-03-24 20:33:14]

On Thu, Mar 24, 2011 at 1:32 PM, Dirk Pranke <dpranke@chromium.org> wrote:
> On Thu, Mar 24, 2011 at 1:30 PM, Dirk Pranke <dpranke@chromium.org> wrote:
>> That would be fine.
>>
>> Also, there's gonna be at least one more patch for this; it looks like
>> rietveld doesn't always return a full email address for the 'owner'
>> field. Is it safe to assume that a bare userid maps to
>> userid@chromium.org?
>>
>
> Hm. Answering my own question, no. How can I get the email address for the
user?
>

nm ... I'll use owner_email.

> -- Dirk
>

[M-A Ruel, 2011-03-24 22:50:06]

lgtm