Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1725053002:
git cl try-results: show buildbucket tryjobs. (Closed)
|
Created:
4 years, 10 months ago by tandrii(chromium) Modified:
4 years, 9 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptiongit cl try-results: show buildbucket tryjobs.
R=nodir@chromium.org,rmistry@chromium.org
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298989
Patch Set 1 #Patch Set 2 : colors #Patch Set 3 : fixes for special cases #
Total comments: 26
Patch Set 4 : builder-names-robust #Patch Set 5 : review #Patch Set 6 : review2 #Patch Set 7 : missing review chagnes #
Total comments: 4
Patch Set 8 : review #Messages
Total messages: 14 (4 generated)
PTAL While sending lots of tryjobs for Win machine, I got tired of having to refresh rietvled just to get URL to the build. Example output: tryjobs for the https://codereview.chromium.org/1730923003 are trying this patch on Mac/Trusty to show tryjob status of some chromium CL (example, https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Mac/buil...). https://codereview.chromium.org/1730923003 itself is has now a failure, 2 success and 1 misspelled builder. Running on it $ git cl try-results git cl try-results Wrong master/builder name: Misspelled builder Started: Try Recipe Mac https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Mac/buil... Try Recipe Trusty 64 https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Trusty%2... Try Recipe Win 64 https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Win%2064... Total: 4 tryjobs tandrii@andrii:0:/ii/build/scripts/slave$ git cl try-results Successes: Try Recipe Mac https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Mac/buil... Try Recipe Trusty 64 https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Trusty%2... Infra Failures: Try Recipe Win 64 https://build.chromium.org/p/tryserver.infra/builders/Try%20Recipe%20Win%2064... Wrong master/builder name: Misspelled builder Total: 4 tryjobs Finally, this is how above looks in color: https://plus.google.com/photos/116086596710843528905/album/625466941361825598....
tandrii@chromium.org changed reviewers: + machenbach@chromium.org
+machenbach@
lgtm https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode491 git_cl.py:491: f=lambda b: (get_name(b), 'id=%s' % b['id'])) Maybe assert len(builds) = 0 ? https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode3543 git_cl.py:3543: print 'ERROR: Exception when trying to trigger tryjobs: %s\n%s' % ( nit: trigger? Maybe adapt error message?
Note: triggered builds get name None.
sorry for the delay https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode255 git_cl.py:255: content_json['error'].get('code', ''), there is no code, only reason and message https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode267 git_cl.py:267: if response.status < 500 or try_count >= 2: you have two checks for try_count value, here and in the loop header. I recommend replacing the loop with `while True` https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode342 git_cl.py:342: 'parameters_json': json.dumps(parameters), Please set client_operation_id to a random string See https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode393 git_cl.py:393: http.force_exception_to_status_code = True From what I understand reading this code, you first try with an authenticator in case there are cached credentials. And if it fails with LoginRequiredError (which means there are no cached credentials), you try without the authenticator I'd recommend to be more explicit: use authenticator.has_cached_credentials function to check if credentials are there. If it returns true, then call authenticator.authoize. You won't have to write this catch clause https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode412 git_cl.py:412: def get_builder(b): nit: blank line before func def https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode413 git_cl.py:413: for tag in b.get('tags', []): Beware that someone may forget to set tags. The source of truth of builder is in parameters_json https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode450 git_cl.py:450: selected = True possibly, using `any` may simplify this code https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode482 git_cl.py:482: f=lambda b: (get_name(b), b.get('result'), b.get('url'))) All cancelled builds will fall into this category. Maybe add a pop statement for status='COMPLETED' result='CANCELED'? https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode3539 git_cl.py:3539: print 'ERROR: %s' % ex nit: 'Buildbucket error: %s' so users direct their complaints more precisely
Hey, thanks for review! https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (left): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#oldcode316 git_cl.py:316: for try_count in xrange(3): this is original - i didn't change this, except for the configurable request args and logging message. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode245 git_cl.py:245: for try_count in xrange(3): this is a copy of code from before. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode255 git_cl.py:255: content_json['error'].get('code', ''), On 2016/02/25 17:14:52, nodir wrote: > there is no code, only reason and message > https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... ok, ok. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode267 git_cl.py:267: if response.status < 500 or try_count >= 2: On 2016/02/25 17:14:52, nodir wrote: > you have two checks for try_count value, here and in the loop header. I > recommend replacing the loop with `while True` DOne, but see https://codereview.chromium.org/1063103002/#ps320001 :) https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode342 git_cl.py:342: 'parameters_json': json.dumps(parameters), On 2016/02/25 17:14:53, nodir wrote: > Please set client_operation_id to a random string > See > https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... outside of CL scope - this is a copy of old code. That said, how does setting it to random string different from now? If it's random, it's ~never same, hence always schedules new build. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode393 git_cl.py:393: http.force_exception_to_status_code = True On 2016/02/25 17:14:52, nodir wrote: > From what I understand reading this code, you first try with an authenticator in > case there are cached credentials. And if it fails with LoginRequiredError > (which means there are no cached credentials), you try without the authenticator > > I'd recommend to be more explicit: use authenticator.has_cached_credentials > function to check if credentials are there. If it returns true, then call > authenticator.authoize. You won't have to write this catch clause Good idea! Done. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode412 git_cl.py:412: def get_builder(b): On 2016/02/25 17:14:52, nodir wrote: > nit: blank line before func def Done. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode413 git_cl.py:413: for tag in b.get('tags', []): On 2016/02/25 17:14:52, nodir wrote: > Beware that someone may forget to set tags. The source of truth of builder is in > parameters_json Indeed, machenbach@ showed me exactly that case, and already fixed in PS5. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode450 git_cl.py:450: selected = True On 2016/02/25 17:14:53, nodir wrote: > possibly, using `any` may simplify this code s/any/all and done, good idea. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode482 git_cl.py:482: f=lambda b: (get_name(b), b.get('result'), b.get('url'))) On 2016/02/25 17:14:53, nodir wrote: > All cancelled builds will fall into this category. Maybe add a pop statement for > status='COMPLETED' result='CANCELED'? Done. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode491 git_cl.py:491: f=lambda b: (get_name(b), 'id=%s' % b['id'])) On 2016/02/25 15:20:29, Michael Achenbach wrote: > Maybe > assert len(builds) = 0 > ? Done. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode3539 git_cl.py:3539: print 'ERROR: %s' % ex On 2016/02/25 17:14:53, nodir wrote: > nit: 'Buildbucket error: %s' > so users direct their complaints more precisely Done. https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode3543 git_cl.py:3543: print 'ERROR: Exception when trying to trigger tryjobs: %s\n%s' % ( On 2016/02/25 15:20:29, Michael Achenbach wrote: > nit: trigger? Maybe adapt error message? Done.
lgtm % tags https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode342 git_cl.py:342: 'parameters_json': json.dumps(parameters), On 2016/02/25 17:59:33, tandrii(chromium) wrote: > On 2016/02/25 17:14:53, nodir wrote: > > Please set client_operation_id to a random string > > See > > > https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... > > outside of CL scope - this is a copy of old code. > > That said, how does setting it to random string different from now? If it's > random, it's ~never same, hence always schedules new build. it is the same in the retry loop, so if a build was created in datastore, but client didn't receive the response and retries the request, the server will see that client operation is the same and will not create a duplicate build https://codereview.chromium.org/1725053002/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/120001/git_cl.py#newcode423 git_cl.py:423: for tag in b.get('tags', []): I'd omit this. The user will probably believe git-cl-tryresults' output that the buildbot builder actually has the build, but it won't because it does not care about tags, so neither should we https://codereview.chromium.org/1725053002/diff/120001/git_cl.py#newcode481 git_cl.py:481: title='Canceled:', color=Fore.MAGENTA, nit: Cancelled :) don't repeat my typo :)
https://codereview.chromium.org/1725053002/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/40001/git_cl.py#newcode342 git_cl.py:342: 'parameters_json': json.dumps(parameters), On 2016/02/25 18:31:06, nodir wrote: > On 2016/02/25 17:59:33, tandrii(chromium) wrote: > > On 2016/02/25 17:14:53, nodir wrote: > > > Please set client_operation_id to a random string > > > See > > > > > > https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn... > > > > outside of CL scope - this is a copy of old code. > > > > That said, how does setting it to random string different from now? If it's > > random, it's ~never same, hence always schedules new build. > > it is the same in the retry loop, so if a build was created in datastore, but > client didn't receive the response and retries the request, the server will see > that client operation is the same and will not create a duplicate build but of course, you are perfectly right! Done. https://codereview.chromium.org/1725053002/diff/120001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1725053002/diff/120001/git_cl.py#newcode423 git_cl.py:423: for tag in b.get('tags', []): On 2016/02/25 18:31:07, nodir wrote: > I'd omit this. The user will probably believe git-cl-tryresults' output that the > buildbot builder actually has the build, but it won't because it does not care > about tags, so neither should we Yeah, i agree. I guess was too attached to my parsing of tags :) https://codereview.chromium.org/1725053002/diff/120001/git_cl.py#newcode481 git_cl.py:481: title='Canceled:', color=Fore.MAGENTA, On 2016/02/25 18:31:07, nodir wrote: > nit: Cancelled :) > don't repeat my typo :) it's not a typo, unless you have a pedantic British English teacher :) Single 'l' is recently common in US English. Since the result keyword is 'CANCELED', i'd keep using it for consistency. FTR, git grep on depot_tools shows two uses of 'll' and ~12 uses of 'l'.
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, nodir@chromium.org Link to the patchset: https://codereview.chromium.org/1725053002/#ps140001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1725053002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1725053002/140001
Message was sent while issue was closed.
Description was changed from ========== git cl try-results: show buildbucket tryjobs. R=nodir@chromium.org,rmistry@chromium.org BUG= ========== to ========== git cl try-results: show buildbucket tryjobs. R=nodir@chromium.org,rmistry@chromium.org BUG= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298989 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=298989 |
