Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927773002:
Reland Implement owners check in presubmit for Gerrit. (Closed)
|
Created:
4 years, 7 months ago by tandrii(chromium) Modified:
4 years, 7 months ago CC:
chromium-reviews, tandrii+omg_git_cl_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@P300 Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionReland Implement owners check in presubmit for Gerrit.
R=machenbach@chromium.org,phajdan.jr@chromium.org
BUG=605563
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300353
Patch Set 1 #Patch Set 2 : thanks pylint #
Total comments: 25
Patch Set 3 : review #
Total comments: 1
Patch Set 4 : fix #Patch Set 5 : re-upload #
Total comments: 3
Patch Set 6 : gerrit fix #
Messages
Total messages: 46 (21 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks... presubmit_canned_checks.py:975: issue = input_api.change.issue 5 lines code dupe. Maybe you can make this common? https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:223: def GetChangeDescription(self, issue, patchset=None, force=False): Who's using force=True? https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:225: self.GetChangeInfo(issue, force=force) # Ensure details are in cache. Why not: info = self.GetChangeInfo(issue, force=force) delete next line and s/self.cache[cache_key]/info/g below https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:228: if patchset: I assume gerrit patchset numbers are not starting with 0. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:237: rev_info = self.cache[cache_key]['revisions'][rev] Can you explain of what each of the revisions is comprised? Above it looks like a rev, rev_info tuple. Now you just assign to rev_info. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:254: for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all'] Do you have a reference to a place that explains how the change info is built up? Otherwise I can only rubber stamp this. OK - later I looked at the test which explains it somewhat. So never mind. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:332: rietveld_obj: rietveld.Rietveld client object nit: Add to docu for consistency, though the rietveld line is not saying much... https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1410: dry_run=None, gerrit_obj=None): nit: Maybe keep ( alignment? Or move all into a new line. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1415: rietveld_obj: rietveld.Rietveld client object. nit: Add to docu for consistency, though the rietveld line is not saying much... https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1487: gerrit_obj=None): Just some order nit: The dry_run feature is quite new, right? Maybe you can sneak in gerrit_obj next to rietveld_obj here without breaking the world? https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1784: rietveld_obj = None So, a hybrid is not supported? :) E.g. a CL that's uploaded to rietveld and gerrit and presubmit kicks in for both? Never mind this comment.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks... presubmit_canned_checks.py:975: issue = input_api.change.issue On 2016/04/28 07:17:29, Michael Achenbach wrote: > 5 lines code dupe. Maybe you can make this common? yep, tricky part is backwards compat, but I think done. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:223: def GetChangeDescription(self, issue, patchset=None, force=False): On 2016/04/28 07:17:30, Michael Achenbach wrote: > Who's using force=True? nobody any more! Removed. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:225: self.GetChangeInfo(issue, force=force) # Ensure details are in cache. On 2016/04/28 07:17:30, Michael Achenbach wrote: > Why not: > info = self.GetChangeInfo(issue, force=force) > > delete next line and s/self.cache[cache_key]/info/g below good question. I thought modifying cache explicitly, rather than implicitly, is better. But, your suggestion + comment is even better, i think. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:228: if patchset: On 2016/04/28 07:17:30, Michael Achenbach wrote: > I assume gerrit patchset numbers are not starting with 0. yes, they don't. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:237: rev_info = self.cache[cache_key]['revisions'][rev] On 2016/04/28 07:17:30, Michael Achenbach wrote: > Can you explain of what each of the revisions is comprised? Above it looks like > a rev, rev_info tuple. Now you just assign to rev_info. rev_info is dict; rev, rev_info is a tuple, but that's from iteritems(). actual example is in test. Should a comment here be useful? https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:254: for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all'] On 2016/04/28 07:17:30, Michael Achenbach wrote: > Do you have a reference to a place that explains how the change info is built > up? Otherwise I can only rubber stamp this. > > OK - later I looked at the test which explains it somewhat. So never mind. https://gerrit-review.googlesource.com/Documentation/rest-api.html is the sorta-reference, but it's not clear from it how 'labels' section looks like. Still, it's better than rietveld API :) https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:332: rietveld_obj: rietveld.Rietveld client object On 2016/04/28 07:17:30, Michael Achenbach wrote: > nit: Add to docu for consistency, though the rietveld line is not saying much... Done. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1410: dry_run=None, gerrit_obj=None): On 2016/04/28 07:17:30, Michael Achenbach wrote: > nit: Maybe keep ( alignment? Or move all into a new line. Done. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1415: rietveld_obj: rietveld.Rietveld client object. On 2016/04/28 07:17:30, Michael Achenbach wrote: > nit: Add to docu for consistency, though the rietveld line is not saying much... Done. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1487: gerrit_obj=None): On 2016/04/28 07:17:30, Michael Achenbach wrote: > Just some order nit: The dry_run feature is quite new, right? Maybe you can > sneak in gerrit_obj next to rietveld_obj here without breaking the world? Done. https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:1784: rietveld_obj = None On 2016/04/28 07:17:29, Michael Achenbach wrote: > So, a hybrid is not supported? :) > > E.g. a CL that's uploaded to rietveld and gerrit and presubmit kicks in for > both? Well, just call it twice with two sets of args :) > > Never mind this comment. too late :D
lgtm - I assume you also rebased in your "review" patchset? https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks... presubmit_canned_checks.py:975: issue = input_api.change.issue On 2016/04/28 19:31:50, tandrii(chromium) wrote: > On 2016/04/28 07:17:29, Michael Achenbach wrote: > > 5 lines code dupe. Maybe you can make this common? > > yep, tricky part is backwards compat, but I think done. Ok - I just meant to share the duplicated code in a separate method. Deleting it is certainly even shorter if you're sure that it'll still work... https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_support.py#ne... presubmit_support.py:237: rev_info = self.cache[cache_key]['revisions'][rev] On 2016/04/28 19:31:50, tandrii(chromium) wrote: > On 2016/04/28 07:17:30, Michael Achenbach wrote: > > Can you explain of what each of the revisions is comprised? Above it looks > like > > a rev, rev_info tuple. Now you just assign to rev_info. > > rev_info is dict; rev, rev_info is a tuple, but that's from iteritems(). > actual example is in test. Should a comment here be useful? No, just me being stupid. Got it now.
On 2016/04/29 08:10:19, Michael Achenbach wrote: > lgtm - I assume you also rebased in your "review" patchset? Yes, sadly, as I was creating second patch on my laptop.
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/40001
Message was sent while issue was closed.
Description was changed from ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 ========== to ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300320 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300320
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1928343002/ by machenbach@chromium.org. The reason for reverting is: Breaks presubmit: https://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presu....
Message was sent while issue was closed.
https://codereview.chromium.org/1927773002/diff/40001/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://codereview.chromium.org/1927773002/diff/40001/presubmit_canned_checks... presubmit_canned_checks.py:985: ok - input_api.gerrit is a bool in production
Message was sent while issue was closed.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Message was sent while issue was closed.
Description was changed from ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300320 ========== to ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 ==========
The CQ bit was unchecked by tandrii@chromium.org
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/60001
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/80001
The CQ bit was unchecked by tandrii@chromium.org
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1927773002/#ps80001 (title: "re-upload")
https://codereview.chromium.org/1927773002/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1927773002/diff/80001/git_cl.py#newcode98 git_cl.py:98: def RunCommand(args, error_ok=False, error_message=None, shell=False, **kwargs): rebase artifact :( https://codereview.chromium.org/1927773002/diff/80001/git_cl.py#newcode4760 git_cl.py:4760: shell=sys.platform == 'win32', rebase artifact :( https://codereview.chromium.org/1927773002/diff/80001/presubmit_support.py File presubmit_support.py (right): https://codereview.chromium.org/1927773002/diff/80001/presubmit_support.py#ne... presubmit_support.py:1457: gerrit_obj=self.gerrit, dry_run=self.dry_run) actual fix.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/80001
Message was sent while issue was closed.
Description was changed from ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 ========== to ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300350 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300350
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1935563002/ by tandrii@chromium.org. The reason for reverting is: now it doesn't work for gerrit. Damn it..
Message was sent while issue was closed.
Description was changed from ========== Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300350 ========== to ========== Reland Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 ==========
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1927773002/#ps100001 (title: "gerrit fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927773002/100001
Message was sent while issue was closed.
Description was changed from ========== Reland Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 ========== to ========== Reland Implement owners check in presubmit for Gerrit. R=machenbach@chromium.org,phajdan.jr@chromium.org BUG=605563 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300353 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=300353
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks.py File presubmit_canned_checks.py (right): https://codereview.chromium.org/1927773002/diff/20001/presubmit_canned_checks... presubmit_canned_checks.py:975: issue = input_api.change.issue On 2016/04/29 08:10:18, Michael Achenbach wrote: > On 2016/04/28 19:31:50, tandrii(chromium) wrote: > > On 2016/04/28 07:17:29, Michael Achenbach wrote: > > > 5 lines code dupe. Maybe you can make this common? > > > > yep, tricky part is backwards compat, but I think done. > > Ok - I just meant to share the duplicated code in a separate method. Deleting it > is certainly even shorter if you're sure that it'll still work... Guess this caused https://bugs.chromium.org/p/chromium/issues/detail?id=609832 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
