git cl try uses buildbucket via OAuth2

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 1f7854551cb112dfc9647896ad28089f874036c2..f1276255dcd3246dc7a0eaa783d81bdd71f79ed4 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -11,6 +11,7 @@ from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import glob
+import httplib
 import json
 import logging
 import optparse
@@ -21,6 +22,8 @@ import stat
 import sys
 import tempfile
 import textwrap
+import time
+import traceback
 import urllib2
 import urlparse
 import webbrowser
@@ -31,8 +34,8 @@ try:
 except ImportError:
   pass
 
-
 from third_party import colorama
+from third_party import httplib2
 from third_party import upload
 import auth
 import breakpad  # pylint: disable=W0611
@@ -62,6 +65,9 @@ REFS_THAT_ALIAS_TO_OTHER_REFS = {
     'refs/remotes/origin/lkcr': 'refs/remotes/origin/master',
 }
 
+# Buildbucket-related constants
+BUILDBUCKET_HOST = 'cr-buildbucket.appspot.com'
+
 # Valid extensions for files we want to lint.
 DEFAULT_LINT_REGEX = r"(.*\.cpp|.*\.cc|.*\.h)"
 DEFAULT_LINT_IGNORE_REGEX = r"$^"
@@ -202,6 +208,113 @@ def add_git_similarity(parser):
   parser.parse_args = Parse
 
 
+def _prefix_master(master):
+  prefix = 'master.'
+  if master.startswith(prefix):
+    return master
+  else:

[M-A Ruel, 2015/05/12 13:05:27]

No need for else, just return

[sheyang, 2015/05/12 17:40:39]

Done.

+    return '%s%s' % (prefix, master)
+
+
+def trigger_try_jobs(
+    auth_config, changelist, options, masters, category):
+  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
+  issue_props = changelist.GetIssueProperties()
+  issue = changelist.GetIssue()
+  patchset = changelist.GetMostRecentPatchset()
+
+  buildbucket_url_template = (
+      'https://{hostname}/_ah/api/buildbucket/v1/builds/batch')
+  buildset_template = 'patch/rietveld/{hostname}/{issue}/{patch}'

[M-A Ruel, 2015/05/12 13:05:28]

I don't see the value to use named variable for these, they are part of the API
and this whole function is part of the API client code, so:

buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format(
   ....)

[sheyang, 2015/05/12 17:40:39]

Done.

+  buildbucket_put_url = buildbucket_url_template.format(
+      hostname=BUILDBUCKET_HOST)
+  buildset = buildset_template.format(
+      hostname=rietveld_host,
+      issue=issue,
+      patch=patchset)
+
+  batch_req_body = {'builds': []}
+  print_text = []
+  print_text.append('Tried jobs on:')
+  for master, builders_and_tests in sorted(masters.iteritems()):
+    print_text.append('Master: %s' % master)
+    bucket = _prefix_master(master)
+    for builder, tests in sorted(builders_and_tests.iteritems()):
+      print_text.append('  %s: %s' % (builder, tests))
+      parameters = {
+          'builder_name': builder,

[M-A Ruel, 2015/05/12 13:05:27]

FYI, I always use +2 indentation for structures in new code.

[sheyang, 2015/05/12 17:40:39]

I thought +4 is for line break?

+          'changes':[

[M-A Ruel, 2015/05/12 13:05:27]

'changes': [

[sheyang, 2015/05/12 17:40:38]

Eagle eyes...Done.

+              {'author': {'email': issue_props['owner_email']}},
+          ],
+          'properties': {
+              'category': category,
+              'issue': issue,
+              'master': master,
+              'patch_project': issue_props['project'],
+              'patch_storage': 'rietveld',
+              'patchset': patchset,
+              'reason': options.name,
+              'revision': options.revision,
+              'rietveld': rietveld_url,
+              'testfilter': tests,
+          },
+      }
+      if options.clobber:
+        parameters['properties']['clobber'] = True
+      batch_req_body['builds'].append(
+          {
+              'bucket': bucket,
+              'parameters_json': json.dumps(parameters),
+              'tags': ['builder:%s' % builder,
+                       'buildset:%s' % buildset,
+                       'master:%s' % master,
+                       'user_agent:git_cl_try']
+          }
+      )
+
+  wait = 1

[M-A Ruel, 2015/05/12 13:05:27]

In general we do:

for try_count in xrange(3):


then at line you do something like:

  time.sleep(0.5 + i*1.5)

- you have one variable instead of 2
- the relation between try count and sleep duration is clear, it's a formula
- you save 4 lines of code leading to more readable loop.

[sheyang, 2015/05/12 17:40:38]

Done.

+  try_count = 3
+  while try_count > 0:
+    try_count -= 1
+    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 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:
+      break
+
+    if response.status < 500 or try_count <= 0:
+      raise httplib2.HttpLib2Error(content)
+
+    # status >= 500 means transient failures.
+    logging.debug('Transient errors when triggering tryjobs. '
+                  'Will retry in %d seconds.', wait)
+    time.sleep(wait)
+    wait *= 2
+
+  print '\n'.join(print_text)
+      
+
 def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards):
   """Return the corresponding git ref if |base_url| together with |glob_spec|
   matches the full |url|.
@@ -269,6 +382,10 @@ def print_stats(similarity, find_copies, args):
       stdout=stdout, env=env)
 
 
+class BuildbucketResponseException(Exception):
+  pass
+
+
 class Settings(object):
   def __init__(self):
     self.default_server = None
@@ -2764,6 +2881,9 @@ def CMDtry(parser, args):
            "server-side to define what default bot set to use")
   group.add_option(
       "-n", "--name", help="Try job name; default to current branch name")
+  group.add_option(
+      "--use-buildbucket", action="store_true", default=False,
+      help="Use buildbucket to trigger try jobs.")
   parser.add_option_group(group)
   auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
@@ -2861,23 +2981,36 @@ def CMDtry(parser, args):
         '\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' % patchset)
-  try:
-    cl.RpcServer().trigger_distributed_try_jobs(
-        cl.GetIssue(), patchset, options.name, options.clobber,
-        options.revision, masters)
-  except urllib2.HTTPError, e:
-    if e.code == 404:
-      print('404 from rietveld; '
-            'did you mean to use "git try" instead of "git cl try"?')
+  if options.use_buildbucket:
+    try:
+      trigger_try_jobs(
+          auth_config, cl, options, masters, 'git_cl_try')
+    except BuildbucketResponseException as ex:
+      print 'ERROR: %s' % ex
+      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' % (
+          e, stacktrace)
       return 1
-  print('Tried jobs on:')
-
-  for (master, builders) in masters.iteritems():
-    if master:
-      print 'Master: %s' % master
-    length = max(len(builder) for builder in builders)
-    for builder in sorted(builders):
-      print '  %*s: %s' % (length, builder, ','.join(builders[builder]))
+  else:
+    try:
+      cl.RpcServer().trigger_distributed_try_jobs(
+          cl.GetIssue(), patchset, options.name, options.clobber,
+          options.revision, masters)
+    except urllib2.HTTPError, e:
+      if e.code == 404:
+        print('404 from rietveld; '
+              'did you mean to use "git try" instead of "git cl try"?')
+        return 1
+    print('Tried jobs on:')
+
+    for (master, builders) in masters.iteritems():

[M-A Ruel, 2015/05/12 13:05:28]

sort here while at it, it always bothered me that the ordering was random.

[sheyang, 2015/05/12 17:40:38]

Done.

+      if master:
+        print 'Master: %s' % master
+      length = max(len(builder) for builder in builders)
+      for builder in sorted(builders):
+        print '  %*s: %s' % (length, builder, ','.join(builders[builder]))
   return 0