From de281aee41f5fae08e9274341b7995d26af32a66 Mon Sep 17 00:00:00 2001 From: tandrii Date: Wed, 12 Oct 2016 06:02:30 -0700 Subject: [PATCH] git cl try: make code less Rietveld-specific. This also adds first test to cover case of custom properties. R=machenbach@chromium.org,sergiyb@chromium.org BUG=599931 Review-Url: https://codereview.chromium.org/2409223002 --- git_cl.py | 112 ++++++++++++++++++++++++++++--------------- tests/git_cl_test.py | 61 ++++++++++++++++++++++- 2 files changed, 133 insertions(+), 40 deletions(-) diff --git a/git_cl.py b/git_cl.py index 2fcde0422..174d46b3b 100755 --- a/git_cl.py +++ b/git_cl.py @@ -318,24 +318,34 @@ def _buildbucket_retry(operation_name, http, *args, **kwargs): assert False, 'unreachable' -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) +def _trigger_try_jobs(auth_config, changelist, masters, options, + category='git_cl_try', patchset=None): + 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 + authenticator = auth.get_authenticator_for_host(codereview_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() - properties = _get_properties_from_options(options) + + # TODO(tandrii): consider caching Gerrit CL details just like + # _RietveldChangelistImpl does, then caching values in these two variables + # won't be necessary. + owner_email = changelist.GetIssueOwner() + project = changelist.GetIssueProject() buildbucket_put_url = ( 'https://{hostname}/_ah/api/buildbucket/v1/builds/batch'.format( hostname=options.buildbucket_host)) - buildset = 'patch/rietveld/{hostname}/{issue}/{patch}'.format( - hostname=rietveld_host, - issue=issue, + buildset = 'patch/{codereview}/{hostname}/{issue}/{patch}'.format( + codereview='gerrit' if changelist.IsGerrit() else 'rietveld', + hostname=codereview_host, + issue=changelist.GetIssue(), patch=patchset) + extra_properties = _get_properties_from_options(options) batch_req_body = {'builds': []} print_text = [] @@ -348,26 +358,26 @@ def trigger_try_jobs(auth_config, changelist, options, masters, category): parameters = { 'builder_name': builder, 'changes': [{ - 'author': {'email': issue_props['owner_email']}, + 'author': {'email': owner_email}, 'revision': options.revision, }], 'properties': { 'category': category, - 'issue': issue, + 'issue': changelist.GetIssue(), 'master': master, - 'patch_project': issue_props['project'], + 'patch_project': project, 'patch_storage': 'rietveld', 'patchset': patchset, 'reason': options.name, - 'rietveld': rietveld_url, + 'rietveld': codereview_url, }, } if 'presubmit' in builder.lower(): parameters['properties']['dry_run'] = 'true' if tests: parameters['properties']['testfilter'] = tests - if properties: - parameters['properties'].update(properties) + if extra_properties: + parameters['properties'].update(extra_properties) if options.clobber: parameters['properties']['clobber'] = True batch_req_body['builds'].append( @@ -1548,10 +1558,6 @@ class Changelist(object): assert self.GetIssue() return self._codereview_impl.SetCQState(new_state) - def CannotTriggerTryJobReason(self): - """Returns reason (str) if unable trigger tryjobs on this CL or None.""" - return self._codereview_impl.CannotTriggerTryJobReason() - # Forward methods to codereview specific implementation. def CloseIssue(self): @@ -1563,12 +1569,26 @@ class Changelist(object): def GetCodereviewServer(self): return self._codereview_impl.GetCodereviewServer() + def GetIssueOwner(self): + """Get owner from codereview, which may differ from this checkout.""" + return self._codereview_impl.GetIssueOwner() + + def GetIssueProject(self): + """Get project from codereview, which may differ from what this + checkout's codereview.settings or gerrit project URL say. + """ + return self._codereview_impl.GetIssueProject() + def GetApprovingReviewers(self): return self._codereview_impl.GetApprovingReviewers() def GetMostRecentPatchset(self): return self._codereview_impl.GetMostRecentPatchset() + def CannotTriggerTryJobReason(self): + """Returns reason (str) if unable trigger tryjobs on this CL or None.""" + return self._codereview_impl.CannotTriggerTryJobReason() + def __getattr__(self, attr): # This is because lots of untested code accesses Rietveld-specific stuff # directly, and it's hard to fix for sure. So, just let it work, and fix @@ -1698,6 +1718,12 @@ class _ChangelistCodereviewBase(object): """Returns reason (str) if unable trigger tryjobs on this CL or None.""" raise NotImplementedError() + def GetIssueOwner(self): + raise NotImplementedError() + + def GetIssueProject(self): + raise NotImplementedError() + class _RietveldChangelistImpl(_ChangelistCodereviewBase): def __init__(self, changelist, auth_config=None, rietveld_server=None): @@ -1783,6 +1809,12 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): def GetApprovingReviewers(self): return get_approving_reviewers(self.GetIssueProperties()) + def GetIssueOwner(self): + return (self.GetIssueProperties() or {}).get('owner_email') + + def GetIssueProject(self): + return (self.GetIssueProperties() or {}).get('project') + def AddComment(self, message): return self.RpcServer().add_comment(self.GetIssue(), message) @@ -2738,6 +2770,14 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): # TODO(tandrii): implement for Gerrit. raise NotImplementedError() + def GetIssueOwner(self): + # TODO(tandrii): implement for Gerrit. + raise NotImplementedError() + + def GetIssueProject(self): + # TODO(tandrii): implement for Gerrit. + raise NotImplementedError() + _CODEREVIEW_IMPLEMENTATIONS = { 'rietveld': _RietveldChangelistImpl, @@ -4717,8 +4757,6 @@ def CMDtry(parser, args): 'Not yet supported for Gerrit (http://crbug.com/599931).\n' 'If your project has Commit Queue, dry run is a workaround:\n' ' git cl set-commit --dry-run') - # Code below assumes Rietveld issue. - # TODO(tandrii): actually implement for Gerrit http://crbug.com/599931. error_message = cl.CannotTriggerTryJobReason() if error_message: @@ -4813,27 +4851,25 @@ def CMDtry(parser, args): for builders in masters.itervalues(): if any('triggered' in b for b in builders): print('ERROR You are trying to send a job to a triggered bot. This type ' - 'of bot requires an\ninitial job from a parent (usually a builder).' - ' Instead send your job to the parent.\n' + 'of bot requires an initial job from a parent (usually a builder). ' + 'Instead send your job to the parent.\n' 'Bot list: %s' % builders, file=sys.stderr) 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) + if patchset != cl.GetPatchset(): + print('Warning: Codereview server has newer patchsets (%s) than most ' + 'recent upload from local checkout (%s). Did a previous upload ' + 'fail?\n' + 'By default, git cl try uses the latest patchset from ' + 'codereview, continuing to use patchset %s.\n' % + (patchset, cl.GetPatchset(), patchset)) try: - trigger_try_jobs(auth_config, cl, options, masters, 'git_cl_try') + _trigger_try_jobs(auth_config, cl, masters, options, 'git_cl_try', + patchset) 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 try jobs: %s\n%s' % - (e, stacktrace)) - return 1 return 0 @@ -4876,8 +4912,8 @@ def CMDtry_results(parser, args): print('Warning: Codereview server has newer patchsets (%s) than most ' 'recent upload from local checkout (%s). Did a previous upload ' 'fail?\n' - 'By default, git cl try uses latest patchset from codereview, ' - 'continuing to use patchset %s.\n' % + 'By default, git cl try-results uses the latest patchset from ' + 'codereview, continuing to use patchset %s.\n' % (patchset, cl.GetPatchset(), patchset)) try: jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 3e3b59195..90647a713 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -5,9 +5,9 @@ """Unit tests for git_cl.py.""" +import json import os import StringIO -import stat import sys import unittest import urlparse @@ -1836,7 +1836,7 @@ class TestGitCl(TestCase): ] self.assertEqual(0, git_cl.main(['issue', '--json', 'output.json'])) - def test_git_cl_try_default(self): + def test_git_cl_try_default_cq_dry_run(self): self.mock(git_cl.Changelist, 'GetChange', lambda _, *a: ( self._mocked_call(['GetChange']+list(a)))) @@ -1874,6 +1874,63 @@ class TestGitCl(TestCase): out.getvalue(), 'scheduled CQ Dry Run on https://codereview.chromium.org/123\n') + def test_git_cl_try_buildbucket_with_properties(self): + self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 20001) + self.mock(git_cl.Changelist, 'GetIssueOwner', lambda _: 'owner@e.mail') + self.mock(git_cl.Changelist, 'GetIssueProject', lambda _: 'depot_tools') + self.mock(git_cl.uuid, 'uuid4', lambda: 'uuid4') + self.calls = [ + ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), + ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), + ((['git', 'config', 'rietveld.autoupdate'],), CERR1), + ((['git', 'config', 'rietveld.server'],), + 'https://codereview.chromium.org'), + ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), + ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), + ] + + 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'issue': 123, + u'key': u'val', + u'json': [{u'a': 1}, None], + u'master': u'tryserver.chromium', + u'patch_project': u'depot_tools', + u'patch_storage': u'rietveld', + u'patchset': 20001, + u'reason': u'feature', # This is a branch name, but why? + u'rietveld': u'https://codereview.chromium.org', + } + }) + self.assertEqual(build, { + u'bucket': u'master.tryserver.chromium', + u'client_operation_id': u'uuid4', + u'tags': [u'builder:win', + u'buildset:patch/rietveld/codereview.chromium.org/123/20001', + u'master:tryserver.chromium', + 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', '-m', 'tryserver.chromium', '-b', 'win', + '-p', 'key=val', '-p', 'json=[{"a":1}, null]'])) + self.assertRegexpMatches( + git_cl.sys.stdout.getvalue(), + 'Tried jobs on:\nMaster: tryserver.chromium') + def _common_GerritCommitMsgHookCheck(self): self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) self.mock(git_cl.os.path, 'abspath',