From 5c04c782b9778300d6d16399eeb00d10ae88ff26 Mon Sep 17 00:00:00 2001 From: Fabien Boucher Date: Tue, 7 Apr 2020 11:35:46 +0200 Subject: [PATCH] pagure: Improve CI status flag handling - Ensure to consider only CI status flags set by the related token account. This is to avoid being triggered by another CI system voting on the same Pull Request. - Re-use the same status CI slot. - Allow to set the display name of the CI system in from of the CI status flag. Change-Id: I12fc761e34413f4efd7c2ad60ed04bf12d5932a3 --- doc/source/reference/drivers/pagure.rst | 6 +++ tests/base.py | 36 ++++++++++++--- .../fixtures/layouts/requirements-pagure.yaml | 20 +++++++- tests/unit/test_pagure_driver.py | 28 ++++++++++- zuul/driver/pagure/pagureconnection.py | 46 ++++++++++++++----- 5 files changed, 115 insertions(+), 21 deletions(-) diff --git a/doc/source/reference/drivers/pagure.rst b/doc/source/reference/drivers/pagure.rst index 0614ad222e..a12f77985e 100644 --- a/doc/source/reference/drivers/pagure.rst +++ b/doc/source/reference/drivers/pagure.rst @@ -81,6 +81,12 @@ The supported options in ``zuul.conf`` connections are: Path to the Pagure Git repositories. Used to clone. + .. attr:: app_name + :default: Zuul + + Display name that will appear as the application name in front + of each CI status flag. + .. attr:: source_whitelist :default: '' diff --git a/tests/base.py b/tests/base.py index e9846c641f..a163b1a19a 100644 --- a/tests/base.py +++ b/tests/base.py @@ -1232,19 +1232,33 @@ class FakePagurePullRequest(object): return self._getPullRequestEvent( 'pull-request.tag.added', pull_data_field='pull_request') - def getPullRequestStatusSetEvent(self, status): + def getPullRequestStatusSetEvent(self, status, username="zuul"): self.addFlag( - status, "https://url", "Build %s" % status) + status, "https://url", "Build %s" % status, username) return self._getPullRequestEvent('pull-request.flag.added') - def addFlag(self, status, url, comment, username="Pingou"): + def insertFlag(self, flag): + to_pop = None + for i, _flag in enumerate(self.flags): + if _flag['uid'] == flag['uid']: + to_pop = i + if to_pop is not None: + self.flags.pop(to_pop) + self.flags.insert(0, flag) + + def addFlag(self, status, url, comment, username="zuul"): + flag_uid = "%s-%s-%s" % (username, self.number, self.project) flag = { - "username": username, + "username": "Zuul CI", + "user": { + "name": username + }, + "uid": flag_uid[:32], "comment": comment, "status": status, "url": url } - self.flags.insert(0, flag) + self.insertFlag(flag) self._updateTimeStamp() def editInitialComment(self, initial_comment): @@ -1402,13 +1416,18 @@ class FakePagureAPIClient(pagureconnection.PagureAPIClient): pr.is_merged = True return {}, 200, "", "POST" + match = re.match(r'.+/api/0/-/whoami$', url) + if match: + return {"username": "zuul"}, 200, "", "POST" + if not params: return self.gen_error("POST") match = re.match(r'.+/api/0/(.+)/pull-request/(\d+)/flag$', url) if match: pr = self._get_pr(match) - pr.flags.insert(0, params) + params['user'] = {"name": "zuul"} + pr.insertFlag(params) match = re.match(r'.+/api/0/(.+)/pull-request/(\d+)/comment$', url) if match: @@ -1435,9 +1454,12 @@ class FakePagureConnection(pagureconnection.PagureConnection): self.cloneurl = self.upstream_root def get_project_api_client(self, project): - return FakePagureAPIClient( + client = FakePagureAPIClient( self.baseurl, None, project, pull_requests_db=self.pull_requests) + if not self.username: + self.set_my_username(client) + return client def get_project_webhook_token(self, project): return 'fake_webhook_token-%s' % project diff --git a/tests/fixtures/layouts/requirements-pagure.yaml b/tests/fixtures/layouts/requirements-pagure.yaml index 998fa7fdb5..4a95d8f08d 100644 --- a/tests/fixtures/layouts/requirements-pagure.yaml +++ b/tests/fixtures/layouts/requirements-pagure.yaml @@ -66,6 +66,18 @@ pagure: status: 'success' +- pipeline: + name: require-flag + manager: independent + require: + pagure: + status: success + trigger: + pagure: + - event: pg_pull_request + action: status + status: success + - pipeline: name: require-trigger-pg-closed-merged precedence: high @@ -125,4 +137,10 @@ name: org/project6 require-trigger-pg-closed-merged: jobs: - - project-test \ No newline at end of file + - project-test + +- project: + name: org/project7 + require-flag: + jobs: + - project-test diff --git a/tests/unit/test_pagure_driver.py b/tests/unit/test_pagure_driver.py index 04daee45a4..0393a13d00 100644 --- a/tests/unit/test_pagure_driver.py +++ b/tests/unit/test_pagure_driver.py @@ -61,9 +61,8 @@ class TestPagureDriver(ZuulTestCase): self.assertThat( A.comments[1]['comment'], MatchesRegex(r'.*\[project-test2 \]\(.*\).*', re.DOTALL)) - self.assertEqual(2, len(A.flags)) + self.assertEqual(1, len(A.flags)) self.assertEqual('success', A.flags[0]['status']) - self.assertEqual('pending', A.flags[1]['status']) @simple_layout('layouts/basic-pagure.yaml', driver='pagure') def test_pull_request_updated(self): @@ -492,6 +491,31 @@ class TestPagureDriver(ZuulTestCase): self.waitUntilSettled() self.assertEqual(1, len(self.history)) + @simple_layout('layouts/requirements-pagure.yaml', driver='pagure') + def test_flag_require(self): + + A = self.fake_pagure.openFakePullRequest( + 'org/project7', 'master', 'A') + + # CI status from other CIs must not be handled + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("success", username="notzuul")) + self.waitUntilSettled() + self.assertEqual(0, len(self.history)) + self.assertEqual(1, len(A.flags)) + + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("failure")) + self.waitUntilSettled() + self.assertEqual(0, len(self.history)) + self.assertEqual(2, len(A.flags)) + + self.fake_pagure.emitEvent( + A.getPullRequestStatusSetEvent("success")) + self.waitUntilSettled() + self.assertEqual(1, len(self.history)) + self.assertEqual(2, len(A.flags)) + @simple_layout('layouts/requirements-pagure.yaml', driver='pagure') def test_pull_request_closed(self): diff --git a/zuul/driver/pagure/pagureconnection.py b/zuul/driver/pagure/pagureconnection.py index 19a68ef0f8..ed0c3e7f0d 100644 --- a/zuul/driver/pagure/pagureconnection.py +++ b/zuul/driver/pagure/pagureconnection.py @@ -206,6 +206,7 @@ class PagureEventConnector(threading.Thread): 'pull-request.closed': self._event_pull_request_closed, 'pull-request.new': self._event_pull_request, 'pull-request.flag.added': self._event_flag_added, + 'pull-request.flag.updated': self._event_flag_added, 'git.receive': self._event_ref_updated, 'git.branch.creation': self._event_ref_created, 'git.branch.deletion': self._event_ref_deleted, @@ -438,6 +439,12 @@ class PagureAPIClient(): ret.status_code, ret.text)) return ret.json(), ret.status_code, ret.url, 'POST' + def whoami(self): + path = '-/whoami' + resp = self.post(self.base_url + path) + self._manage_error(*resp) + return resp[0]['username'] + def get_project_branches(self): path = '%s/git/branches' % self.project resp = self.get(self.base_url + path) @@ -456,23 +463,29 @@ class PagureAPIClient(): self._manage_error(*resp) return resp[0] - def get_pr_flags(self, number, last=False): + def get_pr_flags(self, number, owner, last=False): path = '%s/pull-request/%s/flag' % (self.project, number) resp = self.get(self.base_url + path) self._manage_error(*resp) data = resp[0] + owned_flags = [ + flag for flag in data['flags'] + if flag['user']['name'] == owner] if last: - if data['flags']: - return data['flags'][0] + if owned_flags: + return owned_flags[0] else: return {} else: - return data['flags'] + return owned_flags - def set_pr_flag(self, number, status, url, description): + def set_pr_flag( + self, number, status, url, description, app_name, username): + flag_uid = "%s-%s-%s" % (username, number, self.project) params = { - "username": "Zuul", + "username": app_name, "comment": "Jobs result is %s" % status, + "uid": flag_uid[:32], "status": status, "url": url} path = '%s/pull-request/%s/flag' % (self.project, number) @@ -505,7 +518,6 @@ class PagureAPIClient(): class PagureConnection(BaseConnection): driver_name = 'pagure' log = logging.getLogger("zuul.PagureConnection") - payload_path = 'payload' def __init__(self, driver, connection_name, connection_config): super(PagureConnection, self).__init__( @@ -518,6 +530,9 @@ class PagureConnection(BaseConnection): 'canonical_hostname', self.server) self.git_ssh_key = self.connection_config.get('sshkey') self.api_token = self.connection_config.get('api_token') + self.app_name = self.connection_config.get( + 'app_name', 'Zuul') + self.username = None self.baseurl = self.connection_config.get( 'baseurl', 'https://%s' % self.server).rstrip('/') self.cloneurl = self.connection_config.get( @@ -562,9 +577,17 @@ class PagureConnection(BaseConnection): def eventDone(self): self.event_queue.task_done() + def set_my_username(self, client): + self.log.debug("Fetching my username ...") + self.username = client.whoami() + self.log.debug("My username is %s" % self.username) + def get_project_api_client(self, project): self.log.debug("Building project %s api_client" % project) - return PagureAPIClient(self.baseurl, self.api_token, project) + client = PagureAPIClient(self.baseurl, self.api_token, project) + if not self.username: + self.set_my_username(client) + return client def get_project_webhook_token(self, project, force_refresh=False): token = self.webhook_tokens.get(project) @@ -696,7 +719,7 @@ class PagureConnection(BaseConnection): def _hasRequiredStatusChecks(self, change): pagure = self.get_project_api_client(change.project.name) - flag = pagure.get_pr_flags(change.number, last=True) + flag = pagure.get_pr_flags(change.number, self.username, last=True) return True if flag.get('status', '') == 'success' else False def canMerge(self, change, allow_needs, event=None): @@ -804,14 +827,15 @@ class PagureConnection(BaseConnection): def setCommitStatus(self, project, number, state, url='', description='', context=''): pagure = self.get_project_api_client(project) - pagure.set_pr_flag(number, state, url, description) + pagure.set_pr_flag( + number, state, url, description, self.app_name, self.username) self.log.info("Set pull-request CI flag status : %s" % description) # Wait for 1 second as flag timestamp is by second time.sleep(1) def getCommitStatus(self, project, number): pagure = self.get_project_api_client(project) - flag = pagure.get_pr_flags(number, last=True) + flag = pagure.get_pr_flags(number, self.username, last=True) self.log.info( "Got pull-request CI status for PR %s on %s status: %s" % ( number, project, flag.get('status')))