Actually check Rietveld for LGTMs in CheckOwners()

Actually check Rietveld for LGTMs ...

This change requires us to change the previous signature for the CheckOwners() hook to provide the server address and email regexp to use for parsing approvals from Rietveld.

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

[Dirk Pranke, 2011-03-10 03:06:17]

The presubmit hooks have no direct knowledge of the rietveld server; it appears
to normally be passed in from PRESUBMIT.py. This change updates CheckOwners() to
take that parameter and, while I'm at it, the email regexp to allow for
validating OWNERS.

[Dirk Pranke, 2011-03-10 03:50:23]

Okay, it was too complicated to juggle the changes between the two patches, so
I've merged them. I think the patch is clearer now anyway ;)

[M-A Ruel, 2011-03-10 13:37:54]

http://codereview.chromium.org/6657028/diff/4001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/6657028/diff/4001/PRESUBMIT.py#newcode43
PRESUBMIT.py:43: #     CODEREVIEW_SERVER,
I'm not sure there, git-cl and gcl already knows better what's the review url.
I'd prefer to not copy that string in many places.

http://codereview.chromium.org/6657028/diff/4001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/6657028/diff/4001/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:652: def _Approvers(input_api, host_url,
email_regexp):
I don't think we want this there. This means that *all* tbr review would now
require committing with --bypass-hooks / --no_presubmit ...

[Dirk Pranke, 2011-03-10 19:30:56]

On Thu, Mar 10, 2011 at 5:37 AM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/6657028/diff/4001/PRESUBMIT.py
> File PRESUBMIT.py (right):
>
> http://codereview.chromium.org/6657028/diff/4001/PRESUBMIT.py#newcode43
> PRESUBMIT.py:43: #     CODEREVIEW_SERVER,
> I'm not sure there, git-cl and gcl already knows better what's the
> review url. I'd prefer to not copy that string in many places.
>

Fair enough. I was attempting to follow what we did for
CheckRietveldTryJobExecution().
Perhaps the server should be a setting on the InputApi?

I'll upload a patch w/ that and we can see what you think.

> http://codereview.chromium.org/6657028/diff/4001/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
>
>
http://codereview.chromium.org/6657028/diff/4001/presubmit_canned_checks.py#n...
> presubmit_canned_checks.py:652: def _Approvers(input_api, host_url,
> email_regexp):
> I don't think we want this there. This means that *all* tbr review would
> now require committing with --bypass-hooks / --no_presubmit ...
>

Ack. Good point; I can't believe I forgot about --tbr. I think that
should be a flag
on the InputApi as well, and then it can be checked in the hook and we can
react appropriately.

-- Dirk

[Dirk Pranke, 2011-03-10 22:01:22]

Changes in the latest patch:

    1) Move CODEREVIEW_SERVER to InputApi.host_url.
    2) Implement first part of support for --tbr (InputApi.is_tbr)
    3) Make EMAIL_REGEXP optional in the CheckOwners() hook.

[Dirk Pranke, 2011-03-11 21:06:17]

ping

[M-A Ruel, 2011-03-11 21:19:09]

http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py
File presubmit_canned_checks.py (right):

http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py#n...
presubmit_canned_checks.py:635: owners_db.email_regexp =
input_api.re.compile(email_regexp)
If there is:
a/PRESUBMIT.py
a/b/PRESUBMIT.py

both with a CheckOwners() but different email_regexp, the regexp will be
overwritten with a's version, right?

http://codereview.chromium.org/6657028/diff/7001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

http://codereview.chromium.org/6657028/diff/7001/tests/presubmit_unittest.py#...
tests/presubmit_unittest.py:1219: input_api.ts_tbr = False
is_tbr?

This line is probably unneeded since it obviously didn't have any effect.

[Dirk Pranke, 2011-03-11 21:26:26]

On Fri, Mar 11, 2011 at 1:19 PM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py
> File presubmit_canned_checks.py (right):
>
>
http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py#n...
> presubmit_canned_checks.py:635: owners_db.email_regexp =
> input_api.re.compile(email_regexp)
> If there is:
> a/PRESUBMIT.py
> a/b/PRESUBMIT.py
>
> both with a CheckOwners() but different email_regexp, the regexp will be
> overwritten with a's version, right?
>

Hmm. Correct. Maybe this setting should get pulled from
codereview.settings instead so that there is only
one per repo?

> http://codereview.chromium.org/6657028/diff/7001/tests/presubmit_unittest.py
> File tests/presubmit_unittest.py (right):
>
>
http://codereview.chromium.org/6657028/diff/7001/tests/presubmit_unittest.py#...
> tests/presubmit_unittest.py:1219: input_api.ts_tbr = False
> is_tbr?
>
> This line is probably unneeded since it obviously didn't have any
> effect.
>

Actually, it did; presubmit_unittests would fail but I missing running
it prior to uploading the patch. The
subsequent changes have the fix but I'll backport it to this change to be clean.

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

[Dirk Pranke, 2011-03-11 21:27:53]

On Fri, Mar 11, 2011 at 1:26 PM, Dirk Pranke <dpranke@chromium.org> wrote:
> On Fri, Mar 11, 2011 at 1:19 PM,  <maruel@chromium.org> wrote:
>>
>> http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py
>> File presubmit_canned_checks.py (right):
>>
>>
http://codereview.chromium.org/6657028/diff/7001/presubmit_canned_checks.py#n...
>> presubmit_canned_checks.py:635: owners_db.email_regexp =
>> input_api.re.compile(email_regexp)
>> If there is:
>> a/PRESUBMIT.py
>> a/b/PRESUBMIT.py
>>
>> both with a CheckOwners() but different email_regexp, the regexp will be
>> overwritten with a's version, right?
>>
>
> Hmm. Correct. Maybe this setting should get pulled from
> codereview.settings instead so that there is only
> one per repo?
>

I think for now I'll leave this hardcoded in CheckOwners() and we can
make it configurable later
as we figure out how we want to propagate stuff from gcl/git-cl to
presubmit better. Sound good?

-- Dirk

[M-A Ruel, 2011-03-11 21:35:18]

lgtm

Probably not worth on gerrit?

[Dirk Pranke, 2011-03-11 21:47:04]

On Fri, Mar 11, 2011 at 1:35 PM,  <maruel@chromium.org> wrote:
> lgtm
>
> Probably not worth on gerrit?

I didn't follow you. You mean some aspect of this isn't worth
enforcing on gerrit? Like ensuring that approvers are from certain
domains? Does Gerrit have other ways to enforce who is approving the
changes (I don't know much about it).

-- Dirk

[M-A Ruel, 2011-03-11 22:29:04]

Le 11 mars 2011 16:46, Dirk Pranke <dpranke@chromium.org> a écrit :

> On Fri, Mar 11, 2011 at 1:35 PM,  <maruel@chromium.org> wrote:
> > lgtm
> >
> > Probably not worth on gerrit?
>
> I didn't follow you. You mean some aspect of this isn't worth
> enforcing on gerrit? Like ensuring that approvers are from certain
> domains? Does Gerrit have other ways to enforce who is approving the
> changes (I don't know much about it).
>

Yes, there's administration controls on gerrit.

M-A

[anush, 2011-03-11 22:59:52]

On Gerrit the owners file is enforced via a "+2" code review. Only certain
ACL groups have a "+2" and can approve the change. This is configurable on a
per repo basis.

On Fri, Mar 11, 2011 at 2:28 PM, Marc-Antoine Ruel <maruel@chromium.org>wrote:

> Le 11 mars 2011 16:46, Dirk Pranke <dpranke@chromium.org> a écrit :
>
> On Fri, Mar 11, 2011 at 1:35 PM,  <maruel@chromium.org> wrote:
>> > lgtm
>> >
>> > Probably not worth on gerrit?
>>
>> I didn't follow you. You mean some aspect of this isn't worth
>> enforcing on gerrit? Like ensuring that approvers are from certain
>> domains? Does Gerrit have other ways to enforce who is approving the
>> changes (I don't know much about it).
>>
>
> Yes, there's administration controls on gerrit.
>
> M-A
>