git cl try-results: show buildbucket tryjobs.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 3023213ddd903a9d696f48eea7c79f04964f079e..b5c0bde62dece186c962f3c31b8f235280ab0a9d 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -25,6 +25,7 @@ import tempfile
 import textwrap
 import time
 import traceback
+import urllib
 import urllib2
 import urlparse
 import webbrowser
@@ -239,6 +240,39 @@ def _prefix_master(master):
   return '%s%s' % (prefix, master)
 
 
+def _buildbucket_retry(operation_name, http, *args, **kwargs):
+  """Retries requests to buildbucket service and returns parsed json content."""
+  for try_count in xrange(3):

[tandrii(chromium), 2016/02/25 17:59:33]

this is a copy of code from before.

+    response, content = http.request(*args, **kwargs)
+    try:
+      content_json = json.loads(content)
+    except ValueError:
+      content_json = None
+
+    # Buildbucket could return an error even if status==200.
+    if content_json and content_json.get('error'):
+      msg = 'Error in response. Code: %s. Reason: %s. Message: %s.' % (
+          content_json['error'].get('code', ''),

[nodir, 2016/02/25 17:14:52]

there is no code, only reason and message
https://docs.google.com/document/d/10BH6oSq_UEtdSdhrguEuV4hx-JxY7jzFcgs1OCHcn...

[tandrii(chromium), 2016/02/25 17:59:33]

ok, ok.

+          content_json['error'].get('reason', ''),
+          content_json['error'].get('message', ''))
+      raise BuildbucketResponseException(msg)
+
+    if response.status == 200:
+      if not content_json:
+        raise BuildbucketResponseException(
+            'Buildbucket returns invalid json content: %s.\n'
+            'Please file bugs at http://crbug.com, label "Infra-BuildBucket".' %
+            content)
+      return content_json
+    if response.status < 500 or try_count >= 2:

[nodir, 2016/02/25 17:14:52]

you have two checks for try_count value, here and in the loop header. I
recommend replacing the loop with `while True`

[tandrii(chromium), 2016/02/25 17:59:33]

DOne, but see https://codereview.chromium.org/1063103002/#ps320001 :)

+      raise httplib2.HttpLib2Error(content)
+
+    # status >= 500 means transient failures.
+    logging.debug('Transient errors when %s. Will retry.', operation_name)
+    time.sleep(0.5 + 1.5*try_count)
+  assert False, 'unreachable'
+
+
 def trigger_luci_job(changelist, masters, options):
   """Send a job to run on LUCI."""
   issue_props = changelist.GetIssueProperties()
@@ -313,42 +347,149 @@ def trigger_try_jobs(auth_config, changelist, options, masters, category):
           }
       )
 
-  for try_count in xrange(3):

[tandrii(chromium), 2016/02/25 17:59:33]

this is original - i didn't change this, except for the configurable request
args and logging message.

-    response, content = http.request(
-        buildbucket_put_url,
-        'PUT',
-        body=json.dumps(batch_req_body),
-        headers={'Content-Type': 'application/json'},
-    )
-    content_json = None
-    try:
-      content_json = json.loads(content)
-    except ValueError:
-      pass
+  _buildbucket_retry(
+      'triggering tryjobs',
+      http,
+      buildbucket_put_url,
+      'PUT',
+      body=json.dumps(batch_req_body),
+      headers={'Content-Type': 'application/json'}
+  )
+  print '\n'.join(print_text)
 
-    # Buildbucket could return an error even if status==200.
-    if content_json and content_json.get('error'):
-      msg = 'Error in response. Code: %d. Reason: %s. Message: %s.' % (
-          content_json['error'].get('code', ''),
-          content_json['error'].get('reason', ''),
-          content_json['error'].get('message', ''))
-      raise BuildbucketResponseException(msg)
 
-    if response.status == 200:
-      if not content_json:
-        raise BuildbucketResponseException(
-            'Buildbucket returns invalid json content: %s.\n'
-            'Please file bugs at crbug.com, label "Infra-BuildBucket".' %
-            content)
+def fetch_try_jobs(auth_config, changelist, options):
+  """Fetches tryjobs from buildbucket.
+
+  Returns a map from build id to build info as json dictionary.
+  """
+  rietveld_url = settings.GetDefaultServerUrl()
+  rietveld_host = urlparse.urlparse(rietveld_url).hostname
+  authenticator = auth.get_authenticator_for_host(rietveld_host, auth_config)
+  http = authenticator.authorize(httplib2.Http())
+  http.force_exception_to_status_code = True
+
+  buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format(
+      hostname=rietveld_host,
+      issue=changelist.GetIssue(),
+      patch=options.patchset)
+  params = {'tag': 'buildset:%s' % buildset}
+
+  login_required_error = None
+  builds = {}
+  while True:
+    url = 'https://{hostname}/_ah/api/buildbucket/v1/search?{params}'.format(
+        hostname=options.buildbucket_host,
+        params=urllib.urlencode(params))
+    try:
+      content = _buildbucket_retry('fetching tryjobs', http, url, 'GET')
+    except auth.LoginRequiredError as e:
+      if login_required_error:
+        # Avoid looping forever if login is required on this buildbucket host.
+        raise
+      print 'Warning: Some results might be missing because %s' % e
+      login_required_error = e
+      http = httplib2.Http()
+      http.force_exception_to_status_code = True

[nodir, 2016/02/25 17:14:52]

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

[tandrii(chromium), 2016/02/25 17:59:33]

Good idea! Done.

+      continue
+    for build in content.get('builds', []):
+      builds[build['id']] = build
+    if 'next_cursor' in content:
+      params['start_cursor'] = content['next_cursor']
+    else:
       break
-    if response.status < 500 or try_count >= 2:
-      raise httplib2.HttpLib2Error(content)
+  return builds
 
-    # status >= 500 means transient failures.
-    logging.debug('Transient errors when triggering tryjobs. Will retry.')
-    time.sleep(0.5 + 1.5*try_count)
 
-  print '\n'.join(print_text)
+def print_tryjobs(options, builds):
+  """Prints nicely result of fetch_try_jobs."""
+  if not builds:
+    print 'No tryjobs scheduled'
+    return
+
+  # Make a copy, because we'll be modifying builds dictionary.
+  builds = builds.copy()
+  def get_builder(b):

[nodir, 2016/02/25 17:14:52]

nit: blank line before func def

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+    for tag in b.get('tags', []):

[nodir, 2016/02/25 17:14:52]

Beware that someone may forget to set tags. The source of truth of builder is in
parameters_json

[tandrii(chromium), 2016/02/25 17:59:33]

Indeed, machenbach@ showed me exactly that case, and already fixed in PS5.

+      name_value = tag.split(':', 1)
+      if len(name_value) == 2 and name_value[0] == 'builder':
+        return name_value[1]
+    return None
+
+  def get_bucket(b):
+    bucket = b['bucket']
+    if bucket.startswith('master.'):
+      return bucket[len('master.'):]
+    return bucket
+
+  if options.print_master:
+    name_fmt = '%%-%ds %%-%ds' % (
+        max(len(str(get_bucket(b))) for b in builds.itervalues()),
+        max(len(str(get_builder(b))) for b in builds.itervalues()))
+    def get_name(b):
+      return name_fmt % (get_bucket(b), get_builder(b))
+  else:
+    name_fmt = '%%-%ds' % (
+        max(len(str(get_builder(b))) for b in builds.itervalues()))
+    def get_name(b):
+      return name_fmt % get_builder(b)
+
+  def sort_key(b):
+    return b['status'], b.get('result'), get_name(b), b.get('url')
+
+  def pop(title, f, color=None, **kwargs):
+    """Pop matching builds from `builds` dict and print them."""
+
+    if not sys.stdout.isatty() or color is None:
+      colorize = str
+    else:
+      colorize = lambda x: '%s%s%s' % (color, x, Fore.RESET)
+
+    result = []
+    for b in builds.values():
+      selected = True

[nodir, 2016/02/25 17:14:53]

possibly, using `any` may simplify this code

[tandrii(chromium), 2016/02/25 17:59:33]

s/any/all and done, good idea.

+      for k, v in kwargs.iteritems():
+        if b.get(k) != v:
+          selected = False
+          break
+      if selected:
+        builds.pop(b['id'])
+        result.append(b)
+    if result:
+      print colorize(title)
+      for b in sorted(result, key=sort_key):
+        print ' ', colorize('\t'.join(map(str, f(b))))
+
+  total = len(builds)
+  pop(status='COMPLETED', result='SUCCESS',
+      title='Successes:', color=Fore.GREEN,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE', failure_reason='INFRA_FAILURE',
+      title='Infra Failures:', color=Fore.MAGENTA,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE', failure_reason='BUILD_FAILURE',
+      title='Failures:', color=Fore.RED,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='COMPLETED', result='FAILURE',
+      failure_reason='INVALID_BUILD_DEFINITION',
+      title='Wrong master/builder name:', color=Fore.MAGENTA,
+      f=lambda b: (get_name(b),))
+  pop(status='COMPLETED', result='FAILURE',
+      title='Other failures:',
+      f=lambda b: (get_name(b), b.get('failure_reason'), b.get('url')))
+  pop(status='COMPLETED',
+      title='Other finished:',
+      f=lambda b: (get_name(b), b.get('result'), b.get('url')))

[nodir, 2016/02/25 17:14:53]

All cancelled builds will fall into this category. Maybe add a pop statement for
status='COMPLETED' result='CANCELED'?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+  pop(status='STARTED',
+      title='Started:', color=Fore.YELLOW,
+      f=lambda b: (get_name(b), b.get('url')))
+  pop(status='SCHEDULED',
+      title='Scheduled:',
+      f=lambda b: (get_name(b), 'id=%s' % b['id']))
+  # The last section is just in case buildbucket API changes OR there is a bug.
+  pop(title='Other:',
+      f=lambda b: (get_name(b), 'id=%s' % b['id']))

[Michael Achenbach, 2016/02/25 15:20:29]

Maybe
assert len(builds) = 0
?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+  print 'Total: %d tryjobs' % total
 
 
 def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards):
@@ -3365,6 +3506,47 @@ def CMDtry(parser, args):
   return 0
 
 
+def CMDtry_results(parser, args):
+  group = optparse.OptionGroup(parser, "Try job results options")
+  group.add_option(
+      "-p", "--patchset", type=int, help="patchset number if not current.")
+  group.add_option(
+      "--print-master", action='store_true', help="print master name as well")
+  group.add_option(
+      "--buildbucket-host", default='cr-buildbucket.appspot.com',
+      help="Host of buildbucket. The default host is %default.")
+  parser.add_option_group(group)
+  auth.add_auth_options(parser)
+  options, args = parser.parse_args(args)
+  if args:
+    parser.error('Unrecognized args: %s' % ' '.join(args))
+
+  auth_config = auth.extract_auth_config_from_options(options)
+  cl = Changelist(auth_config=auth_config)
+  if not cl.GetIssue():
+    parser.error('Need to upload first')
+
+  if not options.patchset:
+    options.patchset = cl.GetMostRecentPatchset()
+    if options.patchset and options.patchset != cl.GetPatchset():
+      print(
+          '\nWARNING Mismatch between local config and server. Did a previous '
+          'upload fail?\ngit-cl try always uses latest patchset from rietveld. '
+          'Continuing using\npatchset %s.\n' % options.patchset)
+  try:
+    jobs = fetch_try_jobs(auth_config, cl, options)
+  except BuildbucketResponseException as ex:
+    print 'ERROR: %s' % ex

[nodir, 2016/02/25 17:14:53]

nit: 'Buildbucket error: %s'
so users direct their complaints more precisely

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+    return 1
+  except Exception as e:
+    stacktrace = (''.join(traceback.format_stack()) + traceback.format_exc())
+    print 'ERROR: Exception when trying to trigger tryjobs: %s\n%s' % (

[Michael Achenbach, 2016/02/25 15:20:29]

nit: trigger? Maybe adapt error message?

[tandrii(chromium), 2016/02/25 17:59:33]

Done.

+        e, stacktrace)
+    return 1
+  print_tryjobs(options, jobs)
+  return 0
+
+
 @subcommand.usage('[new upstream branch]')
 def CMDupstream(parser, args):
   """Prints or sets the name of the upstream branch, if any."""