Merge "Implement pipeline requirement on github reviews" into feature/zuulv3

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