|
|
Created:
9 years, 9 months ago by Dirk Pranke Modified:
9 years, 7 months ago Reviewers:
M-A Ruel CC:
chromium-reviews Visibility:
Public. |
DescriptionClean 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
Patch Set 1 #
Total comments: 4
Patch Set 2 : update w/ review feedback from maruel #
Total comments: 1
Patch Set 3 : use owner_email instead of owner, disallow quoted lines containing 'lgtm' #Patch Set 4 : lint #Patch Set 5 : rebase to head #
Created: 9 years, 9 months ago
Messages
Total messages: 15 (0 generated)
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.
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']) ?
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.
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.
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/ >
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
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/ >
can you upload a new patchset?
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/ >
updated.
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 '>' ?
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/ >
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
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 >
lgtm |