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..065847957f9983e8d2d6dea945010203e25c03ae 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,11 @@ REFS_THAT_ALIAS_TO_OTHER_REFS = {
     'refs/remotes/origin/lkcr': 'refs/remotes/origin/master',
 }
 
+# Buildbucket-related constants
+BUILDBUCKET_PUT_URL = (

[M-A Ruel, 2015/05/12 00:46:21]

Please only code the host here. Host != API, so that it's shouldn't be an
endpoint here, same for BUILDSET_STR, which is not worth making a constant.

So

BUILDBUCKET_HOST = 'cr-buildbucket.appspot.com'

then create the URL in the function below.

[sheyang, 2015/05/12 01:37:56]

Done.

+    'https://cr-buildbucket.appspot.com/_ah/api/buildbucket/v1/builds/batch')

[M-A Ruel, 2015/05/11 22:21:28]

That URL shouldn't be hardcoded there.

[nodir, 2015/05/11 22:48:29]

I think hardcoding the default hostname is fine, but making it configurable
(read from codereview.settings or something) would be good too

[M-A Ruel, 2015/05/11 23:01:27]

Not lgtm.

It's not optional nor nice to have.

[sheyang, 2015/05/12 01:37:56]

Done.

+BUILDSET_STR = 'patch/rietveld/{hostname}/{issue}/{patch}'
+
 # Valid extensions for files we want to lint.
 DEFAULT_LINT_REGEX = r"(.*\.cpp|.*\.cc|.*\.h)"
 DEFAULT_LINT_IGNORE_REGEX = r"$^"
@@ -202,6 +210,107 @@ def add_git_similarity(parser):
   parser.parse_args = Parse
 
 
+def _prefix_master(master):

[M-A Ruel, 2015/05/12 00:46:21]

Why?

[sheyang, 2015/05/12 01:37:56]

See the comments about 'black magic' below.

+  prefix = 'master.'
+  if master.startswith(prefix):
+    return master
+  else:
+    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()
+  buildset = BUILDSET_STR.format(
+      hostname=rietveld_host,
+      issue=issue,
+      patch=patchset)
+
+  batch_req_body = {'builds': []}
+  print_text = []
+  print_text.append('Tried jobs on:')

[M-A Ruel, 2015/05/11 23:13:59]

Why not printing as it's happening? Why hang output until everything is
complete? It feels much less interactive.

[sheyang, 2015/05/12 01:37:56]

Here we're only preparing parameters for the PUT call - nothing is happening
until the one request gets sent. 

This is also consistent with the old way - send request first, and report error
or tryjob results based on the exception.

+  for master, builders_and_tests in masters.iteritems():

[M-A Ruel, 2015/05/11 23:13:59]

Please sort.

[sheyang, 2015/05/12 01:37:56]

Done.

+    print_text.append('Master: %s' % master)
+    bucket = _prefix_master(master)

[M-A Ruel, 2015/05/12 00:46:21]

This looks like black magic, explain the context.

[sheyang, 2015/05/12 01:37:56]

Buildbucket uses full master name as bucket name
'master.tryserver.chromium.linux', while the developers always use
'tryserver.chromium.linux' to schedule tryjobs by stripping off the prefix
'master.'. This "black magic" here is to convert user-specified master name to
formal master name behind the scene, so this migration will not cause any end
user workflow change.

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

Please add this to _prefix_master docstring then.

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

Done.

+    for builder, tests in builders_and_tests.iteritems():

[M-A Ruel, 2015/05/12 00:46:21]

sort this too.

[sheyang, 2015/05/12 01:37:56]

Done.

+      print_text.append('  %s: %s' % (builder, tests))
+      parameters = {
+          'builder_name': builder,
+          'changes':[
+              {'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),

[M-A Ruel, 2015/05/12 00:46:21]

What. You are encoding json inside json? Why? Because it's passed without
understanding to buildbot?

[sheyang, 2015/05/12 01:37:56]

Yes this parameter property is for buildbot about which Buildbucket has no
knowledge, so it ends up with a big json string... Nodir@ should know all
details about the design.

+              'tags': ['builder:%s' % builder,
+                       'buildset:%s' % buildset,
+                       'master:%s' % master,
+                       'user_agent:git_cl_try']
+          }
+      )
+
+  wait = 1
+  try_count = 3
+  while try_count > 0:
+    try_count -= 1

[M-A Ruel, 2015/05/11 23:13:59]

try_count--

[nodir, 2015/05/11 23:19:36]

this is invalid Python syntax

[M-A Ruel, 2015/05/11 23:20:22]

Eh, damn python.

[sheyang, 2015/05/12 01:37:56]

Acknowledged.

+    response, content = http.request(
+        BUILDBUCKET_PUT_URL,
+        'PUT',

[M-A Ruel, 2015/05/11 23:01:27]

If I understand correctly, the sole reason http is being pulled in is to allow
PUT request?

[nodir, 2015/05/11 23:19:36]

to authenticate the request.

[sheyang, 2015/05/12 01:37:56]

Discussed with maruel@ offline and the recommended request is N/A in
depot_tools.

+        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)

[Vadim Sh., 2015/05/11 22:14:01]

nit: no need to wait if it was the last attempt (i.e. try_count == 0).

[M-A Ruel, 2015/05/11 23:13:59]

Please address this.

[sheyang, 2015/05/12 01:37:56]

if try_count <=0, we'll raise an exception above so this block will not get hit.

+    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 +378,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 +2877,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 +2977,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():
+      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