Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442153002:
Refactoring: Extract helper functions from CMDtry in git_cl.py. (Closed)
|
Created:
4 years, 2 months ago by qyearsley Modified:
4 years, 1 month ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, tandrii+omg_git_cl_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionRefactoring: Extract helper functions from CMDtry in git_cl.py.
The purpose of this change is to prepare for modifying git cl try
so that builders on multiple masters can be triggered in one invocation
(http://crbug.com/640740).
This should not affect behavior; this CL makes several non-essential
changes to formatting and comments.
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/1fdfcb63166412adae9361acf85705c5290523c6
Patch Set 1 #Patch Set 2 : - #
Total comments: 10
Patch Set 3 : Rebase #Patch Set 4 : Move TriggerDryRun to be a method of Changelist #Patch Set 5 : Added a TODO to maybe reuse TriggerDryRun. #
Total comments: 2
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
qyearsley@chromium.org changed reviewers: + tandrii@chromium.org
https://codereview.chromium.org/2442153002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode406 git_cl.py:406: '"-m tryserver.chromium.linux".' % error_message) Note: This block is moved from lines 4810-4815 and the condition is changed to just `if not options.master` because at this point in the code, given the conditions above, we can assume that --bot is given and --bucket is not given. https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode450 git_cl.py:450: return result_master, None This function is moved from below and renamed; in another CL, I'd like to change this to return a dict mapping masters to builders. https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode575 git_cl.py:575: raise This block is extracted from below; potentially, it could be reused in other places here, or this change could be removed from this CL (and this block could be put back in CMDtry).
LGTM % suggestion, thanks for this! https://codereview.chromium.org/2442153002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode363 git_cl.py:363: # Fall back to deprecated method: get try slaves from PRESUBMIT.py we should kill this, it has lived for too long, but since this is refactoring, can you just add TODO for now? https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode399 git_cl.py:399: # TODO(crbug.com/640740): git cl try should be able to trigger lol :) I think styleguide mandates TODO(qyearsley): bugurl ... that said, i completely agree it's a bug. https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode450 git_cl.py:450: return result_master, None On 2016/10/22 19:14:31, qyearsley wrote: > This function is moved from below and renamed; in another CL, I'd like to change > this to return a dict mapping masters to builders. SGTM. +1 for "_" name. https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode575 git_cl.py:575: raise On 2016/10/22 19:14:31, qyearsley wrote: > This block is extracted from below; potentially, it could be reused in other > places here, or this change could be removed from this CL (and this block could > be put back in CMDtry). SGTM, how about making it method of ChangeList itself? It'd make sense later to replace SetCQState() in CMDset_commit && CMDUpload to make use of this function, maybe by refactoring it to be dry-run independent.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2442153002/diff/20001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode363 git_cl.py:363: # Fall back to deprecated method: get try slaves from PRESUBMIT.py On 2016/10/23 at 16:55:55, tandrii(chromium) wrote: > we should kill this, it has lived for too long, but since this is refactoring, can you just add TODO for now? Sounds good! https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode399 git_cl.py:399: # TODO(crbug.com/640740): git cl try should be able to trigger On 2016/10/23 at 16:55:55, tandrii(chromium) wrote: > lol :) I think styleguide mandates TODO(qyearsley): bugurl ... > > that said, i completely agree it's a bug. Ah, makes sense; done. https://codereview.chromium.org/2442153002/diff/20001/git_cl.py#newcode575 git_cl.py:575: raise On 2016/10/23 at 16:55:55, tandrii(chromium) wrote: > On 2016/10/22 19:14:31, qyearsley wrote: > > This block is extracted from below; potentially, it could be reused in other > > places here, or this change could be removed from this CL (and this block could > > be put back in CMDtry). > > SGTM, how about making it method of ChangeList itself? > It'd make sense later to replace SetCQState() in CMDset_commit && CMDUpload to make use of this function, maybe by refactoring it to be dry-run independent. Done, and added TODO note to make use of this function in either place (or use SetCQState here and change SetCQState to possibly print messages and catch errors).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2442153002/#ps80001 (title: "Added a TODO to maybe reuse TriggerDryRun.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Extract helper functions from CMDtry in git_cl.py. The purpose of this change is to prepare for modifying git cl try so that builders on multiple masters can be triggered in one invocation (http://crbug.com/640740). This should not affect behavior; this CL makes several non-essential changes to formatting and comments. ========== to ========== Refactoring: Extract helper functions from CMDtry in git_cl.py. The purpose of this change is to prepare for modifying git cl try so that builders on multiple masters can be triggered in one invocation (http://crbug.com/640740). This should not affect behavior; this CL makes several non-essential changes to formatting and comments. Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/1fdfcb63166412... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/1fdfcb63166412...
Message was sent while issue was closed.
nodir@chromium.org changed reviewers: + nodir@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2442153002/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2442153002/diff/80001/git_cl.py#newcode362 git_cl.py:362: return masters this returns masters, not buckets. Caused https://bugs.chromium.org/p/chromium/issues/detail?id=661580
Message was sent while issue was closed.
https://codereview.chromium.org/2442153002/diff/80001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2442153002/diff/80001/git_cl.py#newcode362 git_cl.py:362: return masters On 2016/11/04 21:49:40, nodir wrote: > this returns masters, not buckets. Caused > https://bugs.chromium.org/p/chromium/issues/detail?id=661580 https://codereview.chromium.org/2482523002
Message was sent while issue was closed.
On 2016/11/04 at 21:55:42, nodir wrote: > https://codereview.chromium.org/2442153002/diff/80001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/2442153002/diff/80001/git_cl.py#newcode362 > git_cl.py:362: return masters > On 2016/11/04 21:49:40, nodir wrote: > > this returns masters, not buckets. Caused > > https://bugs.chromium.org/p/chromium/issues/detail?id=661580 > > https://codereview.chromium.org/2482523002 Ah, right -- thanks for finding this and uploading a fix. |
