Merge "Implement pipeline requirement on github reviews" into feature/zuulv3
diff --git a/requirements.txt b/requirements.txt
index 9f20458..aeaabfa 100644
--- a/requirements.txt
+++ b/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
diff --git a/tests/base.py b/tests/base.py
index 3719eb6..c68d2f9 100755
--- a/tests/base.py
+++ b/tests/base.py
@@ -542,7 +542,8 @@
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 @@
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 @@
}
}))
+ 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 @@
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 @@
}
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))
diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml
index 6bbf0c8..6fc94c2 100644
--- a/tests/fixtures/layouts/requirements-github.yaml
+++ b/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
diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py
index a3831ff..3f69307 100644
--- a/tests/unit/test_github_requirements.py
+++ b/tests/unit/test_github_requirements.py
@@ -56,7 +56,7 @@
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 @@
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 @@
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')
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index 7eff7bb..700f041 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -336,6 +336,7 @@
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 @@
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)
diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py
index 98b5ee0..27e7d39 100644
--- a/zuul/driver/github/githubmodel.py
+++ b/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 @@
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 @@
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 @@
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 @@
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 @@
if set(change.status).isdisjoint(set(self.statuses)):
return False
+ # required reviews are ANDed
+ if not self.matchesReviews(change):
+ return False
+
return True
diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py
index c0ae33f..ddb4aac 100644
--- a/zuul/driver/github/githubsource.py
+++ b/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 @@
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 @@
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