Implement Depends-On for github

This adds the capability for one github project pr to depend on another
project's PR, by marking it so in the pull request body. This means we
need to be able to react to editing of the pull request body to see if a
dependency was created or removed.

Reverse searching for a change that needs the current change is
implemented via a github search of (open) pull request bodies.

Various bits of how change objects are created and cached needed to be
fixed to support a change being added while adding another change.

Also put a try around the webhook event handling, so that a traceback,
such as when a dependency loop is detected, can be gracefully logged
instead of the whole thing shutting down.

Change-Id: Ia32e6cc70b163f7e32e74d88f0a1f9b4e3255320
Story: 2000774
Task: 4686

tests/base.py

@@ -554,7 +554,7 @@ class FakeGithubPullRequest(object):
 
     def __init__(self, github, number, project, branch,
                  subject, upstream_root, files=[], number_of_commits=1,
-                 writers=[]):
+                 writers=[], body=''):
         """Creates a new PR with several commits.
         Sends an event about opened PR."""
         self.github = github
@@ -563,6 +563,7 @@ class FakeGithubPullRequest(object):
         self.project = project
         self.branch = branch
         self.subject = subject
+        self.body = body
         self.number_of_commits = 0
         self.upstream_root = upstream_root
         self.files = []
@@ -602,6 +603,9 @@ class FakeGithubPullRequest(object):
     def getPullRequestClosedEvent(self):
         return self._getPullRequestEvent('closed')
 
+    def getPullRequestEditedEvent(self):
+        return self._getPullRequestEvent('edited')
+
     def addComment(self, message):
         self.comments.append(message)
         self._updateTimeStamp()
@@ -723,6 +727,10 @@ class FakeGithubPullRequest(object):
         }
         return (name, data)
 
+    def editBody(self, body):
+        self.body = body
+        self._updateTimeStamp()
+
     def _getRepo(self):
         repo_path = os.path.join(self.upstream_root, self.project)
         return git.Repo(repo_path)
@@ -830,7 +838,8 @@ class FakeGithubPullRequest(object):
                     'repo': {
                         'full_name': self.project
                     }
-                }
+                },
+                'body': self.body
             },
             'sender': {
                 'login': 'ghuser'
@@ -869,11 +878,12 @@ class FakeGithubConnection(githubconnection.GithubConnection):
         self.merge_failure = False
         self.merge_not_allowed_count = 0
 
-    def openFakePullRequest(self, project, branch, subject, files=[]):
+    def openFakePullRequest(self, project, branch, subject, files=[],
+                            body=''):
         self.pr_number += 1
         pull_request = FakeGithubPullRequest(
             self, self.pr_number, project, branch, subject, self.upstream_root,
-            files=files)
+            files=files, body=body)
         self.pull_requests.append(pull_request)
         return pull_request
 
@@ -934,7 +944,9 @@ class FakeGithubConnection(githubconnection.GithubConnection):
                 }
             },
             'files': pr.files,
-            'labels': pr.labels
+            'labels': pr.labels,
+            'merged': pr.is_merged,
+            'body': pr.body
         }
         return data
 
@@ -1022,6 +1034,19 @@ class FakeGithubConnection(githubconnection.GithubConnection):
         pull_request = self.pull_requests[pr_number - 1]
         pull_request.removeLabel(label)
 
+    def _getNeededByFromPR(self, change):
+        prs = []
+        pattern = re.compile(r"Depends-On.*https://%s/%s/pull/%s" %
+                             (self.git_host, change.project.name,
+                              change.number))
+        for pr in self.pull_requests:
+            if pattern.search(pr.body):
+                # Get our version of a pull so that it's a dict
+                pull = self.getPull(pr.project, pr.number)
+                prs.append(pull)
+
+        return prs
+
 
 class BuildHistory(object):
     def __init__(self, **kw):

tests/fixtures/layouts/crd-github.yaml

@@ -0,0 +1,79 @@
+- pipeline:
+    name: check
+    manager: independent
+    trigger:
+      github:
+        - event: pull_request
+          action: edited
+    start:
+      github: {}
+    success:
+      github: {}
+    failure:
+      github: {}
+
+- pipeline:
+    name: gate
+    manager: dependent
+    trigger:
+      github:
+        - event: pull_request
+          action: edited
+    start:
+      github: {}
+    success:
+      github:
+        merge: true
+    failure:
+      github: {}
+
+- job:
+    name: project1-test
+- job:
+    name: project2-test
+- job:
+    name: project3-test
+- job:
+    name: project4-test
+- job:
+    name: project5-test
+- job:
+    name: project6-test
+
+- project:
+    name: org/project1
+    check:
+      jobs:
+        - project1-test
+
+- project:
+    name: org/project2
+    check:
+      jobs:
+        - project2-test
+
+- project:
+    name: org/project3
+    gate:
+      queue: cogated
+      jobs:
+        - project3-test
+
+- project:
+    name: org/project4
+    gate:
+      queue: cogated
+      jobs:
+        - project4-test
+
+- project:
+    name: org/project5
+    gate:
+      jobs:
+        - project5-test
+
+- project:
+    name: org/project6
+    gate:
+      jobs:
+        - project6-test

tests/unit/test_github_crd.py

@@ -0,0 +1,172 @@
+#!/usr/bin/env python
+# Copyright (c) 2017 IBM Corp.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from tests.base import ZuulTestCase, simple_layout
+
+
+class TestGithubCrossRepoDeps(ZuulTestCase):
+    """Test Github cross-repo dependencies"""
+    config_file = 'zuul-github-driver.conf'
+
+    @simple_layout('layouts/crd-github.yaml', driver='github')
+    def test_crd_independent(self):
+        "Test cross-repo dependences on an independent pipeline"
+
+        # Create a change in project1 that a project2 change will depend on
+        A = self.fake_github.openFakePullRequest('org/project1', 'master', 'A')
+
+        # Create a commit in B that sets the dependency on A
+        msg = "Depends-On: https://github.com/org/project1/pull/%s" % A.number
+        B = self.fake_github.openFakePullRequest('org/project2', 'master', 'B',
+                                                 body=msg)
+
+        # Make an event to re-use
+        event = B.getPullRequestEditedEvent()
+
+        self.fake_github.emitEvent(event)
+        self.waitUntilSettled()
+
+        # The changes for the job from project2 should include the project1
+        # PR contet
+        changes = self.getJobFromHistory(
+            'project2-test', 'org/project2').changes
+
+        self.assertEqual(changes, "%s,%s %s,%s" % (A.number,
+                                                   A.head_sha,
+                                                   B.number,
+                                                   B.head_sha))
+
+        # There should be no more changes in the queue
+        tenant = self.sched.abide.tenants.get('tenant-one')
+        self.assertEqual(len(tenant.layout.pipelines['check'].queues), 0)
+
+    @simple_layout('layouts/crd-github.yaml', driver='github')
+    def test_crd_dependent(self):
+        "Test cross-repo dependences on a dependent pipeline"
+
+        # Create a change in project3 that a project4 change will depend on
+        A = self.fake_github.openFakePullRequest('org/project3', 'master', 'A')
+
+        # Create a commit in B that sets the dependency on A
+        msg = "Depends-On: https://github.com/org/project3/pull/%s" % A.number
+        B = self.fake_github.openFakePullRequest('org/project4', 'master', 'B',
+                                                 body=msg)
+
+        # Make an event to re-use
+        event = B.getPullRequestEditedEvent()
+
+        self.fake_github.emitEvent(event)
+        self.waitUntilSettled()
+
+        # The changes for the job from project4 should include the project3
+        # PR contet
+        changes = self.getJobFromHistory(
+            'project4-test', 'org/project4').changes
+
+        self.assertEqual(changes, "%s,%s %s,%s" % (A.number,
+                                                   A.head_sha,
+                                                   B.number,
+                                                   B.head_sha))
+
+        self.assertTrue(A.is_merged)
+        self.assertTrue(B.is_merged)
+
+    @simple_layout('layouts/crd-github.yaml', driver='github')
+    def test_crd_unshared_dependent(self):
+        "Test cross-repo dependences on unshared dependent pipeline"
+
+        # Create a change in project1 that a project2 change will depend on
+        A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A')
+
+        # Create a commit in B that sets the dependency on A
+        msg = "Depends-On: https://github.com/org/project5/pull/%s" % A.number
+        B = self.fake_github.openFakePullRequest('org/project6', 'master', 'B',
+                                                 body=msg)
+
+        # Make an event for B
+        event = B.getPullRequestEditedEvent()
+
+        # Emit for B, which should not enqueue A because they do not share
+        # A queue. Since B depends on A, and A isn't enqueue, B will not run
+        self.fake_github.emitEvent(event)
+        self.waitUntilSettled()
+
+        self.assertEqual(0, len(self.history))
+
+        # Enqueue A alone, let it finish
+        self.fake_github.emitEvent(A.getPullRequestEditedEvent())
+        self.waitUntilSettled()
+
+        self.assertTrue(A.is_merged)
+        self.assertFalse(B.is_merged)
+        self.assertEqual(1, len(self.history))
+
+        # With A merged, B should go through
+        self.fake_github.emitEvent(event)
+        self.waitUntilSettled()
+
+        self.assertTrue(B.is_merged)
+        self.assertEqual(2, len(self.history))
+
+    @simple_layout('layouts/crd-github.yaml', driver='github')
+    def test_crd_cycle(self):
+        "Test cross-repo dependency cycles"
+
+        # A -> B -> A
+        msg = "Depends-On: https://github.com/org/project6/pull/2"
+        A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A',
+                                                 body=msg)
+        msg = "Depends-On: https://github.com/org/project5/pull/1"
+        B = self.fake_github.openFakePullRequest('org/project6', 'master', 'B',
+                                                 body=msg)
+
+        self.fake_github.emitEvent(A.getPullRequestEditedEvent())
+        self.waitUntilSettled()
+
+        self.assertFalse(A.is_merged)
+        self.assertFalse(B.is_merged)
+        self.assertEqual(0, len(self.history))
+
+    @simple_layout('layouts/crd-github.yaml', driver='github')
+    def test_crd_needed_changes(self):
+        "Test cross-repo needed changes discovery"
+
+        # Given change A and B, where B depends on A, when A
+        # completes B should be enqueued (using a shared queue)
+
+        # Create a change in project3 that a project4 change will depend on
+        A = self.fake_github.openFakePullRequest('org/project3', 'master', 'A')
+
+        # Set B to depend on A
+        msg = "Depends-On: https://github.com/org/project3/pull/%s" % A.number
+        B = self.fake_github.openFakePullRequest('org/project4', 'master', 'B',
+                                                 body=msg)
+
+        # Enqueue A, which when finished should enqueue B
+        self.fake_github.emitEvent(A.getPullRequestEditedEvent())
+        self.waitUntilSettled()
+
+        # The changes for the job from project4 should include the project3
+        # PR contet
+        changes = self.getJobFromHistory(
+            'project4-test', 'org/project4').changes
+
+        self.assertEqual(changes, "%s,%s %s,%s" % (A.number,
+                                                   A.head_sha,
+                                                   B.number,
+                                                   B.head_sha))
+
+        self.assertTrue(A.is_merged)
+        self.assertTrue(B.is_merged)

zuul/driver/github/githubconnection.py

@@ -18,6 +18,7 @@ import logging
 import hmac
 import hashlib
 import time
+import re
 
 import cachecontrol
 from cachecontrol.cache import DictCache
@@ -72,7 +73,10 @@ class GithubWebhookListener():
 
         self._validate_signature(request)
 
-        self.__dispatch_event(request)
+        try:
+            self.__dispatch_event(request)
+        except:
+            self.log.exception("Exception handling Github event:")
 
     def __dispatch_event(self, request):
         try:
@@ -172,6 +176,8 @@ class GithubWebhookListener():
         elif action == 'unlabeled':
             event.action = 'unlabeled'
             event.label = body['label']['name']
+        elif action == 'edited':
+            event.action = 'edited'
         else:
             return None
 
@@ -353,6 +359,12 @@ class GithubConnection(BaseConnection):
             DictCache(),
             cache_etags=True)
 
+        # The regex is based on the connection host. We do not yet support
+        # cross-connection dependency gathering
+        self.depends_on_re = re.compile(
+            r"^Depends-On: https://%s/.+/.+/pull/[0-9]+$" % self.git_host,
+            re.MULTILINE | re.IGNORECASE)
+
     def onLoad(self):
         webhook_listener = GithubWebhookListener(self)
         self.registerHttpHandler(self.payload_path,
@@ -476,8 +488,6 @@ class GithubConnection(BaseConnection):
         if event.change_number:
             change = self._getChange(project, event.change_number,
                                      event.patch_number, refresh=refresh)
-            change.refspec = event.refspec
-            change.branch = event.branch
             change.url = event.change_url
             change.updated_at = self._ghTimestampToDate(event.updated_at)
             change.source_event = event
@@ -495,8 +505,9 @@ class GithubConnection(BaseConnection):
             change = Ref(project)
         return change
 
-    def _getChange(self, project, number, patchset, refresh=False):
-        key = '%s/%s/%s' % (project.name, number, patchset)
+    def _getChange(self, project, number, patchset=None, refresh=False,
+                   history=None):
+        key = (project.name, number, patchset)
         change = self._change_cache.get(key)
         if change and not refresh:
             return change
@@ -507,24 +518,122 @@ class GithubConnection(BaseConnection):
             change.patchset = patchset
         self._change_cache[key] = change
         try:
-            self._updateChange(change)
+            self._updateChange(change, history)
         except Exception:
             if key in self._change_cache:
                 del self._change_cache[key]
             raise
         return change
 
-    def _updateChange(self, change):
+    def _getDependsOnFromPR(self, body):
+        prs = []
+        seen = set()
+
+        for match in self.depends_on_re.findall(body):
+            if match in seen:
+                self.log.debug("Ignoring duplicate Depends-On: %s" % (match,))
+                continue
+            seen.add(match)
+            # Get the github url
+            url = match.rsplit()[-1]
+            # break it into the parts we need
+            _, org, proj, _, num = url.rsplit('/', 4)
+            # Get a pull object so we can get the head sha
+            pull = self.getPull('%s/%s' % (org, proj), int(num))
+            prs.append(pull)
+
+        return prs
+
+    def _getNeededByFromPR(self, change):
+        prs = []
+        seen = set()
+        # This shouldn't return duplicate issues, but code as if it could
+
+        # This leaves off the protocol, but looks for the specific GitHub
+        # hostname, the org/project, and the pull request number.
+        pattern = 'Depends-On %s/%s/pull/%s' % (self.git_host,
+                                                change.project.name,
+                                                change.number)
+        query = '%s type:pr is:open in:body' % pattern
+        github = self.getGithubClient()
+        for issue in github.search_issues(query=query):
+            pr = issue.issue.pull_request().as_dict()
+            if not pr.get('url'):
+                continue
+            if issue in seen:
+                continue
+            # the issue provides no good description of the project :\
+            org, proj, _, num = pr.get('url').split('/')[-4:]
+            self.log.debug("Found PR %s/%s/%s needs %s/%s" %
+                           (org, proj, num, change.project.name,
+                            change.number))
+            prs.append(pr)
+            seen.add(issue)
+
+        log_rate_limit(self.log, github)
+        return prs
+
+    def _updateChange(self, change, history=None):
+
+        # If this change is already in the history, we have a cyclic
+        # dependency loop and we do not need to update again, since it
+        # was done in a previous frame.
+        if history and (change.project.name, change.number) in history:
+            return change
+
         self.log.info("Updating %s" % (change,))
         change.pr = self.getPull(change.project.name, change.number)
+        change.refspec = "refs/pull/%s/head" % change.number
+        change.branch = change.pr.get('base').get('ref')
         change.files = change.pr.get('files')
         change.title = change.pr.get('title')
         change.open = change.pr.get('state') == 'open'
+        change.is_merged = change.pr.get('merged')
         change.status = self._get_statuses(change.project,
                                            change.patchset)
         change.reviews = self.getPullReviews(change.project,
                                              change.number)
         change.labels = change.pr.get('labels')
+        change.body = change.pr.get('body')
+
+        if history is None:
+            history = []
+        else:
+            history = history[:]
+        history.append((change.project.name, change.number))
+
+        needs_changes = []
+
+        # Get all the PRs this may depend on
+        for pr in self._getDependsOnFromPR(change.body):
+            proj = pr.get('base').get('repo').get('full_name')
+            pull = pr.get('number')
+            self.log.debug("Updating %s: Getting dependent "
+                           "pull request %s/%s" %
+                           (change, proj, pull))
+            project = self.source.getProject(proj)
+            dep = self._getChange(project, pull,
+                                  patchset=pr.get('head').get('sha'),
+                                  history=history)
+            if (not dep.is_merged) and dep not in needs_changes:
+                needs_changes.append(dep)
+
+        change.needs_changes = needs_changes
+
+        needed_by_changes = []
+        for pr in self._getNeededByFromPR(change):
+            proj = pr.get('base').get('repo').get('full_name')
+            pull = pr.get('number')
+            self.log.debug("Updating %s: Getting needed "
+                           "pull request %s/%s" %
+                           (change, proj, pull))
+            project = self.source.getProject(proj)
+            dep = self._getChange(project, pull,
+                                  patchset=pr.get('head').get('sha'),
+                                  history=history)
+            if not dep.is_merged:
+                needed_by_changes.append(dep)
+        change.needed_by_changes = needed_by_changes
 
         return change
 

zuul/driver/github/githubreporter.py

@@ -139,7 +139,7 @@ class GithubReporter(BaseReporter):
         if change.title:
             message += change.title
 
-        account = change.source_event.account
+        account = getattr(change.source_event, 'account', None)
         if not account:
             return message