From ae4cd274b1a194999cc7feabc8d0bff6ce0eb32b Mon Sep 17 00:00:00 2001 From: Jesse Keating Date: Mon, 30 Jan 2017 17:10:44 -0800 Subject: [PATCH] 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 --- requirements.txt | 4 +- tests/base.py | 39 +++++++- .../fixtures/layouts/requirements-github.yaml | 76 +++++++++++++++ tests/unit/test_github_requirements.py | 96 ++++++++++++++++++- zuul/driver/github/githubconnection.py | 62 ++++++++++++ zuul/driver/github/githubmodel.py | 84 +++++++++++++++- zuul/driver/github/githubsource.py | 14 ++- 7 files changed, 368 insertions(+), 7 deletions(-) diff --git a/requirements.txt b/requirements.txt index 9f20458c0f..aeaabfa359 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 1d3669405d..9232d52d34 100755 --- a/tests/base.py +++ b/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)) diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml index 6bbf0c8292..6fc94c2181 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 a3831ff951..3f693074b0 100644 --- a/tests/unit/test_github_requirements.py +++ b/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') diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 7eff7bbb0e..700f0416d2 100644 --- a/zuul/driver/github/githubconnection.py +++ b/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) diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py index 98b5ee0b48..27e7d39131 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 @@ 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 diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py index c0ae33f5dd..ddb4aac114 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 @@ 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