Handle GitHub comment reviews carefully
A comment review should not mask a previously provided approval or
changes_requested review from a given user. The GitHub UX follows the
same logic. If an approval or a changes_requested review was already
provided from a user, that user can issue future comment reviews and it
will not change their vote. Our algorithm needed to be updated to handle
this scenario, and a test case was added to validate it.
Change-Id: Iac10544a1f24362c27220a44edbdb988b7caa989
diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py
index 135f7ab..34da88f 100644
--- a/tests/unit/test_github_requirements.py
+++ b/tests/unit/test_github_requirements.py
@@ -256,6 +256,37 @@
self.assertEqual(self.history[0].name, 'project5-reviewuserstate')
@simple_layout('layouts/requirements-github.yaml', driver='github')
+ def test_pipeline_require_review_comment_masked(self):
+ "Test pipeline requirement: review comments on top of votes"
+
+ A = self.fake_github.openFakePullRequest('org/project5', '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)
+
+ # The first 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 review is required, so provide it
+ A.addReview('derp', 'APPROVED')
+
+ # Add a comment review on top to make sure we can still enqueue
+ A.addReview('derp', 'COMMENTED')
+ 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')
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index 838cba5..6e0ccde 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -763,8 +763,19 @@
# 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.
+ # BUT do not keep the latest if it's a 'commented' type and the
+ # previous review was 'approved' or 'changes_requested', as
+ # the GitHub model does not change the vote if a comment is
+ # added after the fact. THANKS GITHUB!
if review['grantedOn'] > reviews[user]['grantedOn']:
- reviews[user] = review
+ if (review['type'] == 'commented' and reviews[user]['type']
+ in ('approved', 'changes_requested')):
+ self.log.debug("Discarding comment review %s due to "
+ "an existing vote %s" % (review,
+ reviews[user]))
+ pass
+ else:
+ reviews[user] = review
return reviews.values()