Implement pipeline requirement on github reviews

Github reviews are a new pipeline requirement that is driver specific.
Reviews can be approved, changes_requested, or comment. They can come
from people with read, write, or admin access. Access is hierarchical,
admin level includes write and read, and write access includes read.

Review requirements model loosely the gerrit approvals, allowing
filtering on username, email, newer-than, older-than, type, and
permission.

Brings in an unreleased Github3.py code. Further extends that code to
determine if a user has push rights to a repository.

Documentation is not included with this change, as the docs need
restructuring for driver specific require / reject.

Change-Id: I3ab2139c2b11b7dc8aa896a03047615bcf42adba
Signed-off-by: Jesse Keating <omgjlk@us.ibm.com>

requirements.txt

@@ -1,6 +1,8 @@
 pbr>=1.1.0
 
-Github3.py==1.0.0a2
+# pull from master until https://github.com/sigmavirus24/github3.py/pull/671
+# is in a release
+-e git://github.com/sigmavirus24/github3.py.git@develop#egg=Github3.py
 PyYAML>=3.1.0
 Paste
 WebOb>=1.2.3

tests/base.py

@@ -542,7 +542,8 @@ class GithubChangeReference(git.Reference):
 class FakeGithubPullRequest(object):
 
     def __init__(self, github, number, project, branch,
-                 subject, upstream_root, files=[], number_of_commits=1):
+                 subject, upstream_root, files=[], number_of_commits=1,
+                 writers=[]):
         """Creates a new PR with several commits.
         Sends an event about opened PR."""
         self.github = github
@@ -557,6 +558,8 @@ class FakeGithubPullRequest(object):
         self.comments = []
         self.labels = []
         self.statuses = {}
+        self.reviews = []
+        self.writers = []
         self.updated_at = None
         self.head_sha = None
         self.is_merged = False
@@ -773,6 +776,26 @@ class FakeGithubPullRequest(object):
             }
         }))
 
+    def addReview(self, user, state, granted_on=None):
+        # Each user will only have one review at a time, so replace
+        # any existing reviews
+        # FIXME(jlk): this isn't quite right, reviews stack, we only
+        # consider the latest for a user. Thanks GitHub!!
+        for review in self.reviews:
+            if review['user']['login'] == user:
+                self.reviews.remove(review)
+
+        if not granted_on:
+            granted_on = time.time()
+        self.reviews.append({
+            'state': state,
+            'user': {
+                'login': user,
+                'email': user + "@derp.com",
+            },
+            'provided': int(granted_on),
+        })
+
     def _getPRReference(self):
         return '%s/head' % self.number
 
@@ -903,6 +926,10 @@ class FakeGithubConnection(githubconnection.GithubConnection):
         pr = self.pull_requests[number - 1]
         return pr.files
 
+    def _getPullReviews(self, owner, project, number):
+        pr = self.pull_requests[number - 1]
+        return pr.reviews
+
     def getUser(self, login):
         data = {
             'username': login,
@@ -911,6 +938,16 @@ class FakeGithubConnection(githubconnection.GithubConnection):
         }
         return data
 
+    def getRepoPermission(self, project, login):
+        owner, proj = project.split('/')
+        for pr in self.pull_requests:
+            pr_owner, pr_project = pr.project.split('/')
+            if (pr_owner == owner and proj == pr_project):
+                if login in pr.writers:
+                    return 'write'
+                else:
+                    return 'read'
+
     def getGitUrl(self, project):
         return os.path.join(self.upstream_root, str(project))
 

tests/fixtures/layouts/requirements-github.yaml

@@ -28,10 +28,68 @@
       github:
         status: 'failure'
 
+- pipeline:
+    name: reviewusername
+    manager: independent
+    require:
+      github:
+        review:
+          - username: '^(herp|derp)$'
+            type: approved
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: 'test me'
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: reviewreq
+    manager: independent
+    require:
+      github:
+        review:
+          - type: approved
+            permission: write
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: 'test me'
+    success:
+      github:
+        comment: true
+
+- pipeline:
+    name: reviewuserstate
+    manager: independent
+    require:
+      github:
+        review:
+          - username: 'derp'
+            type: approved
+            permission: write
+    trigger:
+      github:
+        - event: pull_request
+          action: comment
+          comment: 'test me'
+    success:
+      github:
+        comment: true
+
 - job:
     name: project1-pipeline
 - job:
     name: project2-trigger
+- job:
+    name: project3-reviewusername
+- job:
+    name: project4-reviewreq
+- job:
+    name: project5-reviewuserstate
 
 - project:
     name: org/project1
@@ -44,3 +102,21 @@
     trigger:
       jobs:
         - project2-trigger
+
+- project:
+    name: org/project3
+    reviewusername:
+      jobs:
+        - project3-reviewusername
+
+- project:
+    name: org/project4
+    reviewreq:
+      jobs:
+        - project4-reviewreq
+
+- project:
+    name: org/project5
+    reviewuserstate:
+      jobs:
+        - project5-reviewuserstate

tests/unit/test_github_requirements.py

@@ -56,7 +56,7 @@ class TestGithubRequirements(ZuulTestCase):
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 0)
 
-        # An success status from unknown user should not cause it to be
+        # A success status from unknown user should not cause it to be
         # enqueued
         A.setStatus(A.head_sha, 'success', 'null', 'null', 'check', user='foo')
         self.fake_github.emitEvent(A.getCommitStatusEvent('check',
@@ -65,7 +65,7 @@ class TestGithubRequirements(ZuulTestCase):
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 0)
 
-        # A success status goes in
+        # A success status from zuul goes in
         A.setStatus(A.head_sha, 'success', 'null', 'null', 'check')
         self.fake_github.emitEvent(A.getCommitStatusEvent('check'))
         self.waitUntilSettled()
@@ -79,3 +79,95 @@ class TestGithubRequirements(ZuulTestCase):
                                                           state='error'))
         self.waitUntilSettled()
         self.assertEqual(len(self.history), 1)
+
+    @simple_layout('layouts/requirements-github.yaml', driver='github')
+    def test_pipeline_require_review_username(self):
+        "Test pipeline requirement: review username"
+
+        A = self.fake_github.openFakePullRequest('org/project3', 'master', 'A')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent('test me')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        # No approval from derp so should not be enqueued
+        self.assertEqual(len(self.history), 0)
+
+        # Add an approved review from derp
+        A.addReview('derp', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project3-reviewusername')
+
+    @simple_layout('layouts/requirements-github.yaml', driver='github')
+    def test_pipeline_require_review_state(self):
+        "Test pipeline requirement: review state"
+
+        A = self.fake_github.openFakePullRequest('org/project4', 'master', 'A')
+        # Add derp to writers
+        A.writers.append('derp')
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent('test me')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        # No positive review from derp so should not be enqueued
+        self.assertEqual(len(self.history), 0)
+
+        # A negative review from derp should not cause it to be enqueued
+        A.addReview('derp', 'CHANGES_REQUESTED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive from nobody should not cause it to be enqueued
+        A.addReview('nobody', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive review from derp should cause it to be enqueued
+        A.addReview('derp', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project4-reviewreq')
+
+    @simple_layout('layouts/requirements-github.yaml', driver='github')
+    def test_pipeline_require_review_user_state(self):
+        "Test pipeline requirement: review state from user"
+
+        A = self.fake_github.openFakePullRequest('org/project5', 'master', 'A')
+        # Add derp and herp to writers
+        A.writers.extend(('derp', 'herp'))
+        # A comment event that we will keep submitting to trigger
+        comment = A.getCommentAddedEvent('test me')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        # No positive review from derp so should not be enqueued
+        self.assertEqual(len(self.history), 0)
+
+        # A negative review from derp should not cause it to be enqueued
+        A.addReview('derp', 'CHANGES_REQUESTED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive from nobody should not cause it to be enqueued
+        A.addReview('nobody', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive review from herp (a writer) should not cause it to be
+        # enqueued
+        A.addReview('herp', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 0)
+
+        # A positive review from derp should cause it to be enqueued
+        A.addReview('derp', 'APPROVED')
+        self.fake_github.emitEvent(comment)
+        self.waitUntilSettled()
+        self.assertEqual(len(self.history), 1)
+        self.assertEqual(self.history[0].name, 'project5-reviewuserstate')

zuul/driver/github/githubconnection.py

@@ -336,6 +336,7 @@ class GithubConnection(BaseConnection):
             change.files = self.getPullFileNames(project, change.number)
             change.title = event.title
             change.status = self._get_statuses(project, event.patch_number)
+            change.reviews = self.getPullReviews(project, change.number)
             change.source_event = event
         elif event.ref:
             change = Ref(project)
@@ -428,12 +429,73 @@ class GithubConnection(BaseConnection):
         log_rate_limit(self.log, self.github)
         return filenames
 
+    def getPullReviews(self, project, number):
+        owner, proj = project.name.split('/')
+
+        revs = self._getPullReviews(owner, proj, number)
+
+        reviews = []
+        for rev in revs:
+            review = {
+                'by': {
+                    'username': rev.get('user').get('login'),
+                    'email': rev.get('user').get('email'),
+                },
+                'grantedOn': rev.get('provided'),
+            }
+
+            review['type'] = rev.get('state').lower()
+
+            # Get user's rights. A user always has read to leave a review
+            review['permission'] = 'read'
+            permission = self.getRepoPermission(
+                project.name, rev.get('user').get('login'))
+            if permission == 'write':
+                review['permission'] = 'write'
+            if permission == 'admin':
+                review['permission'] = 'admin'
+
+            reviews.append(review)
+        return reviews
+
+    def _getPullReviews(self, owner, project, number):
+        # make a list out of the reviews so that we complete our
+        # API transaction
+        reviews = [review.as_dict() for review in
+                   self.github.pull_request(owner, project, number).reviews()]
+
+        log_rate_limit(self.log, self.github)
+        return reviews
+
     def getUser(self, login):
         return GithubUser(self.github, login)
 
     def getUserUri(self, login):
         return 'https://%s/%s' % (self.git_host, login)
 
+    def getRepoPermission(self, project, login):
+        owner, proj = project.split('/')
+        # This gets around a missing API call
+        # need preview header
+        headers = {'Accept': 'application/vnd.github.korra-preview'}
+
+        # Create a repo object
+        repository = self.github.repository(owner, proj)
+        # Build up a URL
+        url = repository._build_url('collaborators', login, 'permission',
+                                    base_url=repository._api)
+        # Get the data
+        perms = repository._get(url, headers=headers)
+
+        log_rate_limit(self.log, self.github)
+
+        # no known user, maybe deleted since review?
+        if perms.status_code == 404:
+            return 'none'
+
+        # get permissions from the data
+        return perms.json()['permission']
+
     def commentPull(self, project, pr_number, message):
         owner, proj = project.split('/')
         repository = self.github.repository(owner, proj)

zuul/driver/github/githubmodel.py

@@ -14,9 +14,12 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import copy
 import re
+import time
 
 from zuul.model import Change, TriggerEvent, EventFilter, RefFilter
+from zuul.driver.util import time_to_seconds
 
 
 EMPTY_GIT_REF = '0' * 40  # git sha of all zeros, used during creates/deletes
@@ -27,6 +30,7 @@ class PullRequest(Change):
         super(PullRequest, self).__init__(project)
         self.updated_at = None
         self.title = None
+        self.reviews = []
 
     def isUpdateOf(self, other):
         if (hasattr(other, 'number') and self.number == other.number and
@@ -55,6 +59,74 @@ class GithubTriggerEvent(TriggerEvent):
         return False
 
 
+class GithubReviewFilter(object):
+    def __init__(self, required_reviews=[]):
+        self._required_reviews = copy.deepcopy(required_reviews)
+        self.required_reviews = self._tidy_reviews(required_reviews)
+
+    def _tidy_reviews(self, reviews):
+        for r in reviews:
+            for k, v in r.items():
+                if k == 'username':
+                    r['username'] = re.compile(v)
+                elif k == 'email':
+                    r['email'] = re.compile(v)
+                elif k == 'newer-than':
+                    r[k] = time_to_seconds(v)
+                elif k == 'older-than':
+                    r[k] = time_to_seconds(v)
+        return reviews
+
+    def _match_review_required_review(self, rreview, review):
+        # Check if the required review and review match
+        now = time.time()
+        by = review.get('by', {})
+        for k, v in rreview.items():
+            if k == 'username':
+                if (not v.search(by.get('username', ''))):
+                        return False
+            elif k == 'email':
+                if (not v.search(by.get('email', ''))):
+                        return False
+            elif k == 'newer-than':
+                t = now - v
+                if (review['provided'] < t):
+                        return False
+            elif k == 'older-than':
+                t = now - v
+                if (review['provided'] >= t):
+                    return False
+            elif k == 'type':
+                if review['type'] != v:
+                    return False
+            elif k == 'permission':
+                # If permission is read, we've matched. You must have read
+                # to provide a review. Write or admin permission is different.
+                if v != 'read':
+                    if review['permission'] != v:
+                        return False
+        return True
+
+    def matchesReviews(self, change):
+        if self.required_reviews and not change.reviews:
+            # No reviews means no matching
+            return False
+
+        return self.matchesRequiredReviews(change)
+
+    def matchesRequiredReviews(self, change):
+        for rreview in self.required_reviews:
+            matches_review = False
+            for review in change.reviews:
+                if self._match_review_required_review(rreview, review):
+                    # Consider matched if any review matches
+                    matches_review = True
+                    break
+            if not matches_review:
+                return False
+        return True
+
+
 class GithubEventFilter(EventFilter):
     def __init__(self, trigger, types=[], branches=[], refs=[],
                  comments=[], actions=[], labels=[], unlabels=[],
@@ -170,10 +242,11 @@ class GithubEventFilter(EventFilter):
         return True
 
 
-class GithubRefFilter(RefFilter):
-    def __init__(self, statuses=[]):
+class GithubRefFilter(RefFilter, GithubReviewFilter):
+    def __init__(self, statuses=[], required_reviews=[]):
         RefFilter.__init__(self)
 
+        GithubReviewFilter.__init__(self, required_reviews=required_reviews)
         self.statuses = statuses
 
     def __repr__(self):
@@ -181,6 +254,9 @@ class GithubRefFilter(RefFilter):
 
         if self.statuses:
             ret += ' statuses: %s' % ', '.join(self.statuses)
+        if self.required_reviews:
+            ret += (' required-reviews: %s' %
+                    str(self.required_reviews))
 
         ret += '>'
 
@@ -195,4 +271,8 @@ class GithubRefFilter(RefFilter):
             if set(change.status).isdisjoint(set(self.statuses)):
                 return False
 
+        # required reviews are ANDed
+        if not self.matchesReviews(change):
+            return False
+
         return True

zuul/driver/github/githubsource.py

@@ -14,6 +14,7 @@
 
 import logging
 import time
+import voluptuous as v
 
 from zuul.source import BaseSource
 from zuul.model import Project
@@ -96,6 +97,7 @@ class GithubSource(BaseSource):
     def getRequireFilters(self, config):
         f = GithubRefFilter(
             statuses=to_list(config.get('status')),
+            required_reviews=to_list(config.get('review')),
         )
         return [f]
 
@@ -103,8 +105,18 @@ class GithubSource(BaseSource):
         return []
 
 
+review = v.Schema({'username': str,
+                   'email': str,
+                   'older-than': str,
+                   'newer-than': str,
+                   'type': str,
+                   'permission': v.Any('read', 'write', 'admin'),
+                   })
+
+
 def getRequireSchema():
-    require = {'status': scalar_or_list(str)}
+    require = {'status': scalar_or_list(str),
+               'review': scalar_or_list(review)}
     return require