Merge "Ensure PRs arent rejected for stale negative reviews" into feature/zuulv3

This commit is contained in:
Jenkins 2017-05-25 17:12:21 +00:00 committed by Gerrit Code Review
commit 207fa7a045
6 changed files with 201 additions and 20 deletions

View File

@ -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):

View File

@ -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

View File

@ -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')

View File

@ -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

View File

@ -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:

View File

@ -121,4 +121,5 @@ def getRequireSchema():
def getRejectSchema():
return {}
reject = {'review': scalar_or_list(review)}
return reject