From d81dd7646f98b454eb088a516607cc77316a5a18 Mon Sep 17 00:00:00 2001 From: Adam Gandelman Date: Thu, 9 Feb 2017 15:15:49 -0800 Subject: [PATCH] Ensure PRs arent rejected for stale negative reviews This updates the github source to only use the most recent review from each user reviewing a PR. This avoids having obsolete negative reviews trump a newer positive review. I've added a new test for good measure to ensure it works for the github case, where we are doing some conversion of github formatted timestamps to internal timestamps. Change-Id: I5607901def856c9363ec786a4116bfec19c9c97c Co-Authored-By: Jesse Keating Signed-off-by: Adam Gandelman --- tests/base.py | 35 +++++-- .../fixtures/layouts/requirements-github.yaml | 62 +++++++++++++ tests/unit/test_github_requirements.py | 93 +++++++++++++++++++ zuul/driver/github/githubconnection.py | 24 +++-- zuul/driver/github/githubmodel.py | 4 +- zuul/driver/github/githubsource.py | 3 +- 6 files changed, 201 insertions(+), 20 deletions(-) diff --git a/tests/base.py b/tests/base.py index 9232d52d34..71c48aae9e 100755 --- a/tests/base.py +++ b/tests/base.py @@ -16,6 +16,7 @@ # under the License. from six.moves import configparser as ConfigParser +import datetime import gc import hashlib import json @@ -777,23 +778,37 @@ 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) + gh_time_format = '%Y-%m-%dT%H:%M:%SZ' + # convert the timestamp to a str format that would be returned + # from github as 'submitted_at' in the API response + + if granted_on: + granted_on = datetime.datetime.utcfromtimestamp(granted_on) + submitted_at = time.strftime( + gh_time_format, granted_on.timetuple()) + else: + # github timestamps only down to the second, so we need to make + # sure reviews that tests add appear to be added over a period of + # time in the past and not all at once. + if not self.reviews: + # the first review happens 10 mins ago + offset = 600 + else: + # subsequent reviews happen 1 minute closer to now + offset = 600 - (len(self.reviews) * 60) + + granted_on = datetime.datetime.utcfromtimestamp( + time.time() - offset) + submitted_at = time.strftime( + gh_time_format, granted_on.timetuple()) - if not granted_on: - granted_on = time.time() self.reviews.append({ 'state': state, 'user': { 'login': user, 'email': user + "@derp.com", }, - 'provided': int(granted_on), + 'submitted_at': submitted_at, }) def _getPRReference(self): diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml index 6fc94c2181..5b92b58ac2 100644 --- a/tests/fixtures/layouts/requirements-github.yaml +++ b/tests/fixtures/layouts/requirements-github.yaml @@ -53,6 +53,11 @@ review: - type: approved permission: write + reject: + github: + review: + - type: changes_requested + permission: write trigger: github: - event: pull_request @@ -71,6 +76,47 @@ - username: 'derp' type: approved permission: write + reject: + github: + review: + - type: changes_requested + permission: write + trigger: + github: + - event: pull_request + action: comment + comment: 'test me' + success: + github: + comment: true + +- pipeline: + name: newer_than + manager: independent + require: + github: + review: + - type: approved + permission: write + newer-than: 1d + trigger: + github: + - event: pull_request + action: comment + comment: 'test me' + success: + github: + comment: true + +- pipeline: + name: older_than + manager: independent + require: + github: + review: + - type: approved + permission: write + older-than: 1d trigger: github: - event: pull_request @@ -90,6 +136,10 @@ name: project4-reviewreq - job: name: project5-reviewuserstate +- job: + name: project6-newerthan +- job: + name: project7-olderthan - project: name: org/project1 @@ -120,3 +170,15 @@ reviewuserstate: jobs: - project5-reviewuserstate + +- project: + name: org/project6 + newer_than: + jobs: + - project6-newerthan + +- project: + name: org/project7 + older_than: + jobs: + - project7-olderthan diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py index 3f693074b0..60bcf74ce2 100644 --- a/tests/unit/test_github_requirements.py +++ b/tests/unit/test_github_requirements.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import time + from tests.base import ZuulTestCase, simple_layout @@ -171,3 +173,94 @@ class TestGithubRequirements(ZuulTestCase): self.waitUntilSettled() self.assertEqual(len(self.history), 1) self.assertEqual(self.history[0].name, 'project5-reviewuserstate') + +# TODO: Implement reject on approval username/state + + @simple_layout('layouts/requirements-github.yaml', driver='github') + def test_pipeline_require_review_latest_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) + + # The first negative review from derp should not cause it to be + # enqueued + for i in range(1, 4): + submitted_at = time.time() - 72 * 60 * 60 + A.addReview('derp', 'CHANGES_REQUESTED', + submitted_at) + 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') + + @simple_layout('layouts/requirements-github.yaml', driver='github') + def test_require_review_newer_than(self): + + A = self.fake_github.openFakePullRequest('org/project6', '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) + + # Add a too-old positive review, should not be enqueued + submitted_at = time.time() - 72 * 60 * 60 + A.addReview('derp', 'APPROVED', + submitted_at) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 0) + + # Add a recent positive review + submitted_at = time.time() - 12 * 60 * 60 + A.addReview('derp', 'APPROVED', submitted_at) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project6-newerthan') + + @simple_layout('layouts/requirements-github.yaml', driver='github') + def test_require_review_older_than(self): + + A = self.fake_github.openFakePullRequest('org/project7', '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) + + # Add a too-new positive, should not be enqueued + submitted_at = time.time() - 12 * 60 * 60 + A.addReview('derp', 'APPROVED', + submitted_at) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 0) + + # Add an old enough positive, should enqueue + submitted_at = time.time() - 72 * 60 * 60 + A.addReview('herp', 'APPROVED', submitted_at) + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project7-olderthan') diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 700f0416d2..66ffb3f992 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -434,29 +434,39 @@ class GithubConnection(BaseConnection): revs = self._getPullReviews(owner, proj, number) - reviews = [] + reviews = {} for rev in revs: + user = rev.get('user').get('login') review = { 'by': { - 'username': rev.get('user').get('login'), + 'username': user, 'email': rev.get('user').get('email'), }, - 'grantedOn': rev.get('provided'), + 'grantedOn': int(time.mktime(self._ghTimestampToDate( + rev.get('submitted_at')))), } review['type'] = rev.get('state').lower() + review['submitted_at'] = rev.get('submitted_at') # 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')) + permission = self.getRepoPermission(project.name, user) if permission == 'write': review['permission'] = 'write' if permission == 'admin': review['permission'] = 'admin' - reviews.append(review) - return reviews + if user not in reviews: + reviews[user] = review + else: + # if there are multiple reviews per user, keep the newest + # note that this breaks the ability to set the 'older-than' + # option on a review requirement. + if review['grantedOn'] > reviews[user]['grantedOn']: + reviews[user] = review + + return reviews.values() def _getPullReviews(self, owner, project, number): # make a list out of the reviews so that we complete our diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py index 27e7d39131..bbacc9b1b8 100644 --- a/zuul/driver/github/githubmodel.py +++ b/zuul/driver/github/githubmodel.py @@ -90,11 +90,11 @@ class GithubReviewFilter(object): return False elif k == 'newer-than': t = now - v - if (review['provided'] < t): + if (review['grantedOn'] < t): return False elif k == 'older-than': t = now - v - if (review['provided'] >= t): + if (review['grantedOn'] >= t): return False elif k == 'type': if review['type'] != v: diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py index ddb4aac114..0aada4dcd6 100644 --- a/zuul/driver/github/githubsource.py +++ b/zuul/driver/github/githubsource.py @@ -121,4 +121,5 @@ def getRequireSchema(): def getRejectSchema(): - return {} + reject = {'review': scalar_or_list(review)} + return reject