git cl try uses buildbucket via OAuth2

git_cl.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import glob
+import httplib
 import json
 import logging
 import optparse
 import os
 import Queue
 import re
 import stat
 import sys
 import tempfile
 import textwrap
+import time
+import traceback
 import urllib2
 import urlparse
 import webbrowser
 import zlib
 
 try:
   import readline  # pylint: disable=F0401,W0611
 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
 import clang_format
 import dart_format
 import fix_encoding
 import gclient_utils
 import git_common
 import owners
 import owners_finder
 import presubmit_support
 import rietveld
 import scm
 import subcommand
 import subprocess2
 import watchlists
 
 __version__ = '1.0'
 
 DEFAULT_SERVER = 'https://codereview.appspot.com'
 POSTUPSTREAM_HOOK_PATTERN = '.git/hooks/post-cl-%s'
 DESCRIPTION_BACKUP_FILE = '~/.git_cl_description_backup'
 GIT_INSTRUCTIONS_URL = 'http://code.google.com/p/chromium/wiki/UsingGit'
 CHANGE_ID = 'Change-Id:'
 REFS_THAT_ALIAS_TO_OTHER_REFS = {
     'refs/remotes/origin/lkgr': 'refs/remotes/origin/master',
     '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"$^"
 
 # Shortcut since it quickly becomes redundant.
 Fore = colorama.Fore
 
 # Initialized in main()
 settings = None
 
@@ skipping 120 matching lines @@
     else:
       git_set_branch_value('git-find-copies', int(options.find_copies))
 
     print('Using %d%% similarity for rename/copy detection. '
           'Override with --similarity.' % options.similarity)
 
     return options, args
   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|.
 
   If |allow_wildcards| is true, |glob_spec| can contain wildcards (see below).
   """
   fetch_suburl, as_ref = glob_spec.split(':')
   if allow_wildcards:
     glob_match = re.match('(.+/)?(\*|{[^/]*})(/.+)?', fetch_suburl)
     if glob_match:
@@ skipping 47 matching lines @@
   try:
     stdout = sys.stdout.fileno()
   except AttributeError:
     stdout = None
   return subprocess2.call(
       ['git',
        'diff', '--no-ext-diff', '--stat'] + similarity_options + args,
       stdout=stdout, env=env)
 
 
+class BuildbucketResponseException(Exception):
+  pass
+
+
 class Settings(object):
   def __init__(self):
     self.default_server = None
     self.cc = None
     self.root = None
     self.is_git_svn = None
     self.svn_branch = None
     self.tree_status_url = None
     self.viewvc_url = None
     self.updated = False
@@ skipping 2475 matching lines @@
   group.add_option(
       "-c", "--clobber", action="store_true", default=False,
       help="Force a clobber before building; e.g. don't do an "
            "incremental build")
   group.add_option(
       "--project",
       help="Override which project to use. Projects are defined "
            "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)
   auth_config = auth.extract_auth_config_from_options(options)
 
   if args:
     parser.error('Unknown arguments: %s' % args)
 
   cl = Changelist(auth_config=auth_config)
   if not cl.GetIssue():
@@ skipping 77 matching lines @@
           'Instead send your job to the parent.\n'
           'Bot list: %s' % builders)
       return 1
 
   patchset = cl.GetMostRecentPatchset()
   if patchset and 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' % 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
-  print('Tried jobs on:')
+    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
+  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]))
+    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
 
 
 @subcommand.usage('[new upstream branch]')
 def CMDupstream(parser, args):
   """Prints or sets the name of the upstream branch, if any."""
   _, args = parser.parse_args(args)
   if len(args) > 1:
     parser.error('Unrecognized args: %s' % ' '.join(args))
 
@@ skipping 314 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   colorama.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)