From 2c210a490857380a93f8308dd504aeb1ef759d38 Mon Sep 17 00:00:00 2001 From: Edward Lemur Date: Mon, 16 Sep 2019 23:58:35 +0000 Subject: [PATCH] Revert "git-cl: Use bb to schedule try jobs." This reverts commit 5b6ae8bc74d18033d8880e6183efa9ef6ca71de3. Reason for revert: Suspected breakage of wpt-importer (https://ci.chromium.org/p/infra/builders/cron/wpt-importer) Original change's description: > git-cl: Use bb to schedule try jobs. > > Bug: 976104 > Change-Id: I3423667f1ed9edfc5fa17842932de7704951fc62 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1801799 > Commit-Queue: Edward Lesmes > Reviewed-by: Andrii Shyshkalov TBR=nodir@chromium.org,tandrii@google.com,ehmaldonado@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 976104 Change-Id: I885c1e71b34928c402d3375f820b28f3d6535c54 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1806148 Reviewed-by: Edward Lesmes Reviewed-by: Andrii Shyshkalov Commit-Queue: Edward Lesmes --- git_cl.py | 142 ++++++++++++++++++++++--------------------- tests/git_cl_test.py | 78 ++++++++++++------------ 2 files changed, 112 insertions(+), 108 deletions(-) diff --git a/git_cl.py b/git_cl.py index 3c3bc4895..19b7c952b 100755 --- a/git_cl.py +++ b/git_cl.py @@ -416,80 +416,82 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, patchset): buckets: A nested dict mapping bucket names to builders to tests. options: Command-line options. """ - _, ret_code = subprocess2.communicate( - ['bb', 'auth-info'], stdout=subprocess2.VOID, stderr=subprocess2.VOID) - if ret_code: - DieWithError('"bb auth-login" must be executed before scheduling try jobs.') - - gerrit_change = changelist.GetGerritChange() - shared_properties = { - 'category': options.category, - } + assert changelist.GetIssue(), 'CL must be uploaded first' + codereview_url = changelist.GetCodereviewServer() + assert codereview_url, 'CL must be uploaded first' + patchset = patchset or changelist.GetMostRecentPatchset() + assert patchset, 'CL must be uploaded first' + + codereview_host = urlparse.urlparse(codereview_url).hostname + # Cache the buildbucket credentials under the codereview host key, so that + # users can use different credentials for different buckets. + authenticator = auth.get_authenticator_for_host(codereview_host, auth_config) + http = authenticator.authorize(httplib2.Http()) + http.force_exception_to_status_code = True + + buildbucket_put_url = ( + 'https://{hostname}/_ah/api/buildbucket/v1/builds/batch'.format( + hostname=options.buildbucket_host)) + buildset = 'patch/gerrit/{hostname}/{issue}/{patch}'.format( + hostname=codereview_host, + issue=changelist.GetIssue(), + patch=patchset) + + shared_parameters_properties = changelist.GetTryJobProperties(patchset) + shared_parameters_properties['category'] = options.category if options.clobber: - shared_properties['clobber'] = True + shared_parameters_properties['clobber'] = True extra_properties = _get_properties_from_options(options) if extra_properties: - shared_properties.update(extra_properties) + shared_parameters_properties.update(extra_properties) - batch_request = {'requests': []} + batch_req_body = {'builds': []} print_text = [] print_text.append('Tried jobs on:') for bucket, builders_and_tests in sorted(buckets.iteritems()): print_text.append('Bucket: %s' % bucket) - project, bucket = bucket.split('.', 1) for builder, tests in sorted(builders_and_tests.iteritems()): print_text.append(' %s: %s' % (builder, tests)) - properties = shared_properties.copy() + parameters = { + 'builder_name': builder, + 'changes': [{ + 'author': {'email': changelist.GetIssueOwner()}, + 'revision': options.revision, + }], + 'properties': shared_parameters_properties.copy(), + } if 'presubmit' in builder.lower(): - properties['dry_run'] = 'true' + parameters['properties']['dry_run'] = 'true' if tests: - properties['testfilter'] = tests + parameters['properties']['testfilter'] = tests tags = [ - { - 'key': 'builder', - 'value': builder, - }, - { - 'key': 'user_agent', - 'value': 'git_cl_try', - }, + 'builder:%s' % builder, + 'buildset:%s' % buildset, + 'user_agent:git_cl_try', ] - batch_request['requests'].append( + batch_req_body['builds'].append( { - 'scheduleBuild': { - 'builder': { - 'project': options.project or project, - 'bucket': bucket, - 'builder': builder, - }, - 'properties': properties, - 'requestId': str(uuid.uuid4()), - 'gerritChanges': [gerrit_change], - 'tags': tags, - } + 'bucket': bucket, + 'parameters_json': json.dumps(parameters), + 'client_operation_id': str(uuid.uuid4()), + 'tags': tags, } ) - (stdout, _), ret_code = subprocess2.communicate( - ['bb', 'batch', '-host', options.buildbucket_host], - stdin=json.dumps(batch_request, sort_keys=True), - stdout=subprocess2.PIPE) - if ret_code: - print_text.append('Failed to schedule builds for some bots:') - if stdout: - responses = json.loads(stdout) - print_text.extend( - ' ' + response['error']['message'] - for response in responses['responses'] if 'error' in response) - + _buildbucket_retry( + 'triggering tryjobs', + http, + buildbucket_put_url, + 'PUT', + body=json.dumps(batch_req_body), + headers={'Content-Type': 'application/json'} + ) print_text.append('To see results here, run: git cl try-results') print_text.append('To see results in browser, run: git cl web') print('\n'.join(print_text)) - return ret_code - def fetch_try_jobs(auth_config, changelist, buildbucket_host, patchset=None): @@ -2688,27 +2690,26 @@ class Changelist(object): if data['status'] in ('ABANDONED', 'MERGED'): return 'CL %s is closed' % self.GetIssue() - def GetGerritChange(self, patchset=None): - """Returns a buildbucket.v2.GerritChange message for the current issue.""" - host = urlparse.urlparse(self.GetCodereviewServer()).hostname - issue = self.GetIssue() - patchset = int(patchset or self.GetPatchset()) + def GetTryJobProperties(self, patchset=None): + """Returns dictionary of properties to launch a tryjob.""" data = self._GetChangeDetail(['ALL_REVISIONS']) - - assert host and issue and patchset, 'CL must be uploaded first' - - has_patchset = any( - int(revision_data['_number']) == patchset - for revision_data in data['revisions'].itervalues()) - if not has_patchset: + patchset = int(patchset or self.GetPatchset()) + assert patchset + revision_data = None # Pylint wants it to be defined. + for revision_data in data['revisions'].itervalues(): + if int(revision_data['_number']) == patchset: + break + else: raise Exception('Patchset %d is not known in Gerrit change %d' % (patchset, self.GetIssue())) - return { - 'host': host, - 'change': issue, - 'project': data['project'], - 'patchset': patchset, + 'patch_issue': self.GetIssue(), + 'patch_set': patchset or self.GetPatchset(), + 'patch_project': data['project'], + 'patch_storage': 'gerrit', + 'patch_ref': revision_data['fetch']['http']['ref'], + 'patch_repository_url': revision_data['fetch']['http']['url'], + 'patch_gerrit_url': self.GetCodereviewServer(), } def GetIssueOwner(self): @@ -4739,7 +4740,12 @@ def CMDtry(parser, args): return 1 patchset = cl.GetMostRecentPatchset() - return _trigger_try_jobs(auth_config, cl, buckets, options, patchset) + try: + _trigger_try_jobs(auth_config, cl, buckets, options, patchset) + except BuildbucketResponseException as ex: + print('ERROR: %s' % ex) + return 1 + return 0 @metrics.collector.collect_metrics('git cl try-results') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index ce67b884b..cceb3ebf4 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2381,42 +2381,6 @@ class TestGitCl(TestCase): def test_git_cl_try_buildbucket_with_properties_gerrit(self): self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 7) self.mock(git_cl.uuid, 'uuid4', lambda: 'uuid4') - def mocked_call_with_stdin(*a, **kw): - args = list(a) - if kw.get('stdin'): - args.append(kw['stdin']) - return ([self._mocked_call(*args), ''], 0) - - self.mock(subprocess2, 'communicate', mocked_call_with_stdin) - - bb_request = json.dumps({ - "requests": [{ - "scheduleBuild": { - "requestId": "uuid4", - "builder": { - "project": "luci", - "builder": "win", - "bucket": "chromium.try", - }, - "gerritChanges": [{ - "project": "depot_tools", - "host": "chromium-review.googlesource.com", - "patchset": 7, - "change": 123456, - }], - "properties": { - "category": "git_cl_try", - "json": [{"a": 1}, None], - "key": "val", - }, - "tags": [ - {"value": "win", "key": "builder"}, - {"value": "git_cl_try", "key": "user_agent"}, - ], - }, - }], - }, sort_keys=True) - self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), @@ -2446,12 +2410,46 @@ class TestGitCl(TestCase): }, }, }), - ((['bb', 'auth-info'],), ''), - ((['git', 'config', 'branch.feature.gerritpatchset'],), '7'), - ((['bb', 'batch', '-host', 'cr-buildbucket.appspot.com'], - bb_request,), ''), ] + def _buildbucket_retry(*_, **kw): + # self.maxDiff = 10000 + body = json.loads(kw['body']) + self.assertEqual(len(body['builds']), 1) + build = body['builds'][0] + params = json.loads(build.pop('parameters_json')) + self.assertEqual(params, { + u'builder_name': u'win', + u'changes': [{u'author': {u'email': u'owner@e.mail'}, + u'revision': None}], + u'properties': { + u'category': u'git_cl_try', + u'key': u'val', + u'json': [{u'a': 1}, None], + + u'patch_gerrit_url': + u'https://chromium-review.googlesource.com', + u'patch_issue': 123456, + u'patch_project': u'depot_tools', + u'patch_ref': u'refs/changes/56/123456/7', + u'patch_repository_url': + u'https://chromium.googlesource.com/depot_tools', + u'patch_set': 7, + u'patch_storage': u'gerrit', + } + }) + self.assertEqual(build, { + u'bucket': u'luci.chromium.try', + u'client_operation_id': u'uuid4', + u'tags': [ + u'builder:win', + u'buildset:patch/gerrit/chromium-review.googlesource.com/123456/7', + u'user_agent:git_cl_try', + ], + }) + + self.mock(git_cl, '_buildbucket_retry', _buildbucket_retry) + self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self.assertEqual(0, git_cl.main([ 'try', '-B', 'luci.chromium.try', '-b', 'win',