zuul
/
zuul
Code Issues Proposed changes

GitHub Reporter: Fix `Reviewed-by` in Merge Commit Message 

Currently, the user stated in the `Reviewed-by` clause of a merge commit
message is the last user that commented on a pull request. This is only
by chance a user that reviewed a pull request.

This change makes sure we append the correct list of GitHub users to the
`Reviewed-by` clause, i.e., those who are in the list of reviews of the
current change/pull request.

Change-Id: Ic1c8187fce2206607789549f02a1d9576818b23e
This commit is contained in:
Benjamin Schanzel 2020-06-09 12:51:08 +02:00
parent f6e4d30ea1
commit 71d4be1f62
2 changed files with 51 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
tests/unit/test_github_driver.py
View File

@ -14,7 +14,7 @@
import os
import re
from testtools.matchers import MatchesRegex, StartsWith
from testtools.matchers import MatchesRegex, Not, StartsWith
import urllib
import socket
import time
@ -635,7 +635,7 @@ class TestGithubDriver(ZuulTestCase):
self.waitUntilSettled()
self.assertTrue(A.is_merged)
self.assertThat(A.merge_message,
MatchesRegex(r'.*PR title.*Reviewed-by.*', re.DOTALL))
MatchesRegex(r'.*PR title.*', re.DOTALL))
self.assertEqual(len(A.comments), 0)
# pipeline merges the pull request on success after failure
@ -670,6 +670,47 @@ class TestGithubDriver(ZuulTestCase):
self.assertEqual(D.comments[0],
'Pull request merge failed: 403 Merge not allowed')
@simple_layout('layouts/merging-github.yaml', driver='github')
def test_report_pull_merge_message_reviewed_by(self):
# pipeline merges the pull request on success
A = self.fake_github.openFakePullRequest('org/project', 'master', 'A')
self.fake_github.emitEvent(A.getCommentAddedEvent('merge me'))
self.waitUntilSettled()
self.assertTrue(A.is_merged)
# assert that no 'Reviewed-By' is in merge commit message
self.assertThat(A.merge_message,
Not(MatchesRegex(r'.*Reviewed-By.*', re.DOTALL)))
B = self.fake_github.openFakePullRequest('org/project', 'master', 'B')
B.addReview('derp', 'APPROVED')
self.fake_github.emitEvent(B.getCommentAddedEvent('merge me'))
self.waitUntilSettled()
self.assertTrue(B.is_merged)
# assert that single 'Reviewed-By' is in merge commit message
self.assertThat(B.merge_message,
MatchesRegex(
r'.*Reviewed-by: derp <derp@derp.com>.*',
re.DOTALL))
C = self.fake_github.openFakePullRequest('org/project', 'master', 'C')
C.addReview('derp', 'APPROVED')
C.addReview('herp', 'COMMENTED')
self.fake_github.emitEvent(C.getCommentAddedEvent('merge me'))
self.waitUntilSettled()
self.assertTrue(C.is_merged)
# assert that multiple 'Reviewed-By's are in merge commit message
self.assertThat(C.merge_message,
MatchesRegex(
r'.*Reviewed-by: derp <derp@derp.com>.*',
re.DOTALL))
self.assertThat(C.merge_message,
MatchesRegex(
r'.*Reviewed-by: herp <herp@derp.com>.*',
re.DOTALL))
@simple_layout('layouts/dependent-github.yaml', driver='github')
def test_draft_pr(self):
# pipeline merges the pull request on success

25
zuul/driver/github/githubreporter.py
View File

@ -270,23 +270,14 @@ class GithubReporter(BaseReporter):
if change.title:
message += change.title
account = getattr(change.source_event, 'account', None)
if not account:
return message
name = account['name']
email = account['email']
message += '\n\nReviewed-by: '
if name:
message += name
if email:
if name:
message += ' '
message += '<' + email + '>'
if name or email:
message += '\n '
message += account['html_url']
if change.reviews:
review_users = []
for r in change.reviews:
name = r['by']['username']
email = r['by']['email']
review_users.append('Reviewed-by: {} <{}>'.format(name, email))
message += '\n\n'
message += '\n'.join(review_users)
return message