Merge "pagure: Improve CI status flag handling"

5 files changed, 115 insertions, 21 deletions

diff --git a/doc/source/reference/drivers/pagure.rst b/doc/source/reference/drivers/pagure.rst
index 13a7ec272..7845ae79a 100644
--- a/doc/source/reference/drivers/pagure.rst
+++ b/doc/source/reference/drivers/pagure.rst
@@ -82,6 +82,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 4a4e536a3..ba5e68342 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -1235,19 +1235,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):
@@ -1405,13 +1419,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:
@@ -1438,9 +1457,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 998fa7fdb..4a95d8f08 100644
--- a/tests/fixtures/layouts/requirements-pagure.yaml
+++ b/tests/fixtures/layouts/requirements-pagure.yaml
@@ -67,6 +67,18 @@
         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
     manager: independent
@@ -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 04daee45a..0393a13d0 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):
@@ -493,6 +492,31 @@ class TestPagureDriver(ZuulTestCase):
         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):
 
         A = self.fake_pagure.openFakePullRequest(
diff --git a/zuul/driver/pagure/pagureconnection.py b/zuul/driver/pagure/pagureconnection.py
index 5df701230..becc803c8 100644
--- a/zuul/driver/pagure/pagureconnection.py
+++ b/zuul/driver/pagure/pagureconnection.py
@@ -205,6 +205,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,
@@ -437,6 +438,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)
@@ -455,23 +462,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)
@@ -504,7 +517,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__(
@@ -517,6 +529,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(
@@ -561,9 +576,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)
@@ -695,7 +718,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):
@@ -803,14 +826,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')))