From 3f74f6fb048e78679a16fa3f6478b1b39c53d7c2 Mon Sep 17 00:00:00 2001 From: Jesse Keating Date: Thu, 6 Jul 2017 15:45:36 -0700 Subject: [PATCH] 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 --- tests/unit/test_github_requirements.py | 31 ++++++++++++++++++++++++++ zuul/driver/github/githubconnection.py | 13 ++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py index 135f7ab852..34da88fe45 100644 --- a/tests/unit/test_github_requirements.py +++ b/tests/unit/test_github_requirements.py @@ -255,6 +255,37 @@ class TestGithubRequirements(ZuulTestCase): self.assertEqual(len(self.history), 1) 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): diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 838cba5370..6e0ccde816 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -763,8 +763,19 @@ class GithubConnection(BaseConnection): # 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()