[ADD] runbot_merge: sign off commits by reviewer

closes #50
closes #54

runbot_merge/github.py

@@ -39,6 +39,11 @@ class GH(object):
             r.raise_for_status()
         return r
 
+    def user(self, username):
+        r = self._session.get("{}/users/{}".format(self._url, username))
+        r.raise_for_status()
+        return r.json()
+
     def head(self, branch):
         d = self('get', 'git/refs/heads/{}'.format(branch)).json()
 

runbot_merge/models/pull_requests.py

@@ -415,6 +415,7 @@ class PullRequests(models.Model):
     ], default=False)
     method_warned = fields.Boolean(default=False)
 
+    reviewed_by = fields.Many2one('res.partner')
     delegates = fields.Many2many('res.partner', help="Delegate reviewers, not intrinsically reviewers but can review this PR")
     priority = fields.Selection([
         (0, 'Urgent'),
@@ -583,6 +584,7 @@ class PullRequests(models.Model):
                     newstate = RPLUS.get(self.state)
                     if newstate:
                         self.state = newstate
+                        self.reviewed_by = author
                         ok = True
                     else:
                         msg = "This PR is already reviewed, reviewing it again is useless."
@@ -807,13 +809,33 @@ class PullRequests(models.Model):
                 self.env.cr.commit()
 
     def _build_merge_message(self, message):
+        # handle co-authored commits (https://help.github.com/articles/creating-a-commit-with-multiple-authors/)
+        lines = message.splitlines()
+        coauthors = []
+        for idx, line in enumerate(reversed(lines)):
+            if line.startswith('Co-authored-by:'):
+                coauthors.append(line)
+                continue
+            if not line.strip():
+                continue
+
+            if idx:
+                del lines[-idx:]
+            break
+
         m = re.search(r'( |{repository})#{pr.number}\b'.format(
             pr=self,
             repository=self.repository.name.replace('/', '\\/')
         ), message)
-        if m:
+        if not m:
-            return message
+            lines.extend(['', 'closes {pr.repository.name}#{pr.number}'.format(pr=self)])
-        return message + '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=self)
+        if self.reviewed_by:
+            lines.extend(['', 'Signed-off-by: {}'.format(self.reviewed_by.formatted_email)])
+
+        if coauthors:
+            lines.extend(['', ''])
+            lines.extend(reversed(coauthors))
+        return '\n'.join(lines)
 
     def _stage(self, gh, target):
         # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1]
@@ -825,6 +847,12 @@ class PullRequests(models.Model):
         assert commits < 250, "merging PRs of 250+ commits is not supported (https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request)"
         pr_commits = gh.commits(self.number)
 
+        if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login:
+            # XXX: find other trigger(s) to sync github name?
+            gh_name = gh.user(self.reviewed_by.github_login)['name']
+            if gh_name:
+                self.reviewed_by.name = gh_name
+
         # NOTE: lost merge v merge/copy distinction (head being
         #       a merge commit reused instead of being re-merged)
         return method, getattr(self, '_stage_' + method.replace('-', '_'))(gh, target, pr_commits)

runbot_merge/models/res_partner.py

@@ -1,4 +1,5 @@
-from odoo import fields, models, tools
+from email.utils import parseaddr, formataddr
+from odoo import fields, models, tools, api
 
 class Partner(models.Model):
     _inherit = 'res.partner'
@@ -7,9 +8,22 @@ class Partner(models.Model):
     reviewer = fields.Boolean(default=False, help="Can review PRs (maybe m2m to repos/branches?)")
     self_reviewer = fields.Boolean(default=False, help="Can review own PRs (independent from reviewer)")
     delegate_reviewer = fields.Many2many('runbot_merge.pull_requests')
+    formatted_email = fields.Char(compute='_rfc5322_formatted')
 
     def _auto_init(self):
         res = super(Partner, self)._auto_init()
         tools.create_unique_index(
             self._cr, 'runbot_merge_unique_gh_login', self._table, ['github_login'])
         return res
+
+    @api.multi
+    def _rfc5322_formatted(self):
+        # format partner's email according to RFC5322 section 3.4
+        for partner in self:
+            if partner.email:
+                email = parseaddr(partner.email)[1]
+            elif partner.github_login:
+                email = '%s@users.noreply.github.com' % partner.github_login
+            else:
+                email = ''
+            partner.formatted_email = formataddr((partner.name, email))

runbot_merge/tests/fake_github/__init__.py

@@ -16,14 +16,18 @@ from werkzeug.urls import url_parse, url_encode
 
 from . import git
 
-API_PATTERN = re.compile(
+REPOS_API_PATTERN = re.compile(
     r'https://api.github.com/repos/(?P<repo>\w+/\w+)/(?P<path>.+)'
 )
+USERS_API_PATTERN = re.compile(
+    r"https://api.github.com/users/(?P<user>\w+)"
+)
+
 class APIResponse(responses.BaseResponse):
-    def __init__(self, sim):
+    def __init__(self, sim, url):
         super(APIResponse, self).__init__(
             method=None,
-            url=API_PATTERN
+            url=url
         )
         self.sim = sim
         self.content_type = 'application/json'
@@ -35,7 +39,7 @@ class APIResponse(responses.BaseResponse):
     def get_response(self, request):
         m = self.url.match(request.url)
 
-        r = self.sim.repos[m.group('repo')].api(m.group('path'), request)
+        r = self.dispatch(request, m)
         if isinstance(r, responses.HTTPResponse):
             return r
 
@@ -52,6 +56,21 @@ class APIResponse(responses.BaseResponse):
             headers=headers,
             preload_content=False, )
 
+class ReposAPIResponse(APIResponse):
+    def __init__(self, sim):
+        super().__init__(sim, REPOS_API_PATTERN)
+
+    def dispatch(self, request, match):
+        return self.sim.repos[match.group('repo')].api(match.group('path'), request)
+
+class UsersAPIResponse(APIResponse):
+    def __init__(self, sim):
+        super().__init__(sim, url=USERS_API_PATTERN)
+
+    def dispatch(self, request, match):
+        return self.sim._read_user(request, match.group('user'))
+
+
 class Github(object):
     """ Github simulator
 
@@ -74,12 +93,22 @@ class Github(object):
     def __enter__(self):
         # otherwise swallows errors from within the test
         self._requests = responses.RequestsMock(assert_all_requests_are_fired=False).__enter__()
-        self._requests.add(APIResponse(self))
+        self._requests.add(ReposAPIResponse(self))
+        self._requests.add(UsersAPIResponse(self))
         return self
 
     def __exit__(self, *args):
         return self._requests.__exit__(*args)
 
+    def _read_user(self, _, user):
+        return (200, {
+            'id': id(user),
+            'type': 'User',
+            'login': user,
+            'name': user.capitalize(),
+        })
+
+
 class Repo(object):
     def __init__(self, name):
         self.name = name

runbot_merge/tests/local.py

@@ -77,6 +77,7 @@ def users(env):
         'name': "Reviewer",
         'github_login': 'reviewer',
         'reviewer': True,
+        'email': "reviewer@example.com",
     })
     env['res.partner'].create({
         'name': "Self Reviewer",

runbot_merge/tests/test_basic.py

@@ -10,13 +10,13 @@ from lxml import html
 
 import odoo
 
-from test_utils import re_matches, run_crons
+from test_utils import re_matches, run_crons, get_partner
 
 @pytest.fixture
 def repo(make_repo):
     return make_repo('repo')
 
-def test_trivial_flow(env, repo, page):
+def test_trivial_flow(env, repo, page, users):
     # create base branch
     m = repo.make_commit(None, "initial", None, tree={'a': 'some content'})
     repo.make_ref('heads/master', m)
@@ -91,7 +91,9 @@ def test_trivial_flow(env, repo, page):
         'a': b'some other content',
         'b': b'a second file',
     }
-    assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1".format(repo=repo)
+    assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1"\
+                             "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                             .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
 class TestCommitMessage:
     def test_commit_simple(self, env, repo, users):
@@ -113,7 +115,9 @@ class TestCommitMessage:
         env['runbot_merge.project']._check_progress()
 
         master = repo.commit('heads/master')
-        assert master.message == "simple commit message\n\ncloses {repo.name}#1".format(repo=repo)
+        assert master.message == "simple commit message\n\ncloses {repo.name}#1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_existing(self, env, repo, users):
         """ verify do not duplicate 'closes' instruction
@@ -135,7 +139,9 @@ class TestCommitMessage:
 
         master = repo.commit('heads/master')
         # closes #1 is already present, should not modify message
-        assert master.message == "simple commit message that closes #1"
+        assert master.message == "simple commit message that closes #1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 .format(reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_other(self, env, repo, users):
         """ verify do not duplicate 'closes' instruction
@@ -157,7 +163,9 @@ class TestCommitMessage:
 
         master = repo.commit('heads/master')
         # closes on another repositoy, should modify the commit message
-        assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1".format(repo=repo)
+        assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_wrong_number(self, env, repo, users):
         """ verify do not match on a wrong number
@@ -179,8 +187,57 @@ class TestCommitMessage:
 
         master = repo.commit('heads/master')
         # closes on another repositoy, should modify the commit message
-        assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1".format(repo=repo)
+        assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
+    def test_commit_delegate(self, env, repo, users):
+        """ verify 'signed-off-by ...' is correctly added in the commit message for delegated review
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen delegate=%s' % users['other'], "reviewer")
+        prx.post_comment('hansen r+', user='other')
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        assert master.message == "simple commit message\n\ncloses {repo.name}#1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 .format(repo=repo, reviewer=get_partner(env, users['other']))
+
+    def test_commit_coauthored(self, env, repo, users):
+        """ verify 'closes ...' and 'Signed-off-by' are added before co-authored-by tags.
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message\n\n\nCo-authored-by: Bob <bob@example.com>', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        assert master.message == "simple commit message\n\ncloses {repo.name}#1"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\n\nCo-authored-by: Bob <bob@example.com>"\
+                                 .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
 class TestWebhookSecurity:
     def test_no_secret(self, env, project, repo):
@@ -487,11 +544,12 @@ def test_ff_failure_batch(env, repo, users):
         c['commit']['message']
         for c in repo.log('heads/master')
     }
+    reviewer = get_partner(env, users["reviewer"]).formatted_email
     assert messages == {
         'initial', 'NO!',
-        'a1', 'a2', 'A\n\ncloses {}#{}'.format(repo.name, A.number),
+        'a1', 'a2', 'A\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, A.number, reviewer),
-        'b1', 'b2', 'B\n\ncloses {}#{}'.format(repo.name, B.number),
+        'b1', 'b2', 'B\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, B.number, reviewer),
-        'c1', 'c2', 'C\n\ncloses {}#{}'.format(repo.name, C.number),
+        'c1', 'c2', 'C\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, C.number, reviewer),
     }
 
 def test_edit(env, repo):
@@ -974,7 +1032,7 @@ class TestMergeMethod:
             (users['user'], "Merge method set to rebase and fast-forward"),
         ]
 
-    def test_pr_rebase_merge(self, repo, env):
+    def test_pr_rebase_merge(self, repo, env, users):
         """ test result on rebase-merge
 
         left: PR
@@ -1026,8 +1084,9 @@ class TestMergeMethod:
         # then compare to the dag version of the right graph
         nm2 = node('M2', node('M1', node('M0')))
         nb1 = node('B1', node('B0', nm2))
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
         merge_head = (
-            'title\n\nbody\n\ncloses {}#{}'.format(repo.name, prx.number),
+            'title\n\nbody\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer),
             frozenset([nm2, nb1])
         )
         expected = (re_matches('^force rebuild'), frozenset([merge_head]))
@@ -1048,7 +1107,7 @@ class TestMergeMethod:
         final_tree = repo.read_tree(repo.commit('heads/master'))
         assert final_tree == {'m': b'2', 'b': b'1'}, "sanity check of final tree"
 
-    def test_pr_rebase_ff(self, repo, env):
+    def test_pr_rebase_ff(self, repo, env, users):
         """ test result on rebase-merge
 
         left: PR
@@ -1090,7 +1149,9 @@ class TestMergeMethod:
         staging = log_to_node(repo.log('heads/staging.master'))
         # then compare to the dag version of the right graph
         nm2 = node('M2', node('M1', node('M0')))
-        nb1 = node('B1\n\ncloses {}#{}'.format(repo.name, prx.number), node('B0', nm2))
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
+        nb1 = node('B1\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer),
+                   node('B0', nm2))
         expected = node(re_matches('^force rebuild'), nb1)
         assert staging == expected
 
@@ -1113,7 +1174,7 @@ class TestMergeMethod:
     def test_pr_contains_merges(self, repo, env):
         pass
 
-    def test_pr_force_merge_single_commit(self, repo, env):
+    def test_pr_force_merge_single_commit(self, repo, env, users):
         """ should be possible to flag a PR as regular-merged, regardless of
         its commits count
 
@@ -1146,10 +1207,12 @@ class TestMergeMethod:
 
         m = node('M')
         c0 = node('C0', m)
-        expected = node('gibberish\n\nblahblah\n\ncloses {}#{}'.format(repo.name, prx.number), m, c0)
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
+        expected = node('gibberish\n\nblahblah\n\ncloses {}#{}'
+                        '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0)
         assert log_to_node(repo.log('heads/master')), expected
 
-    def test_unrebase_emptymessage(self, repo, env):
+    def test_unrebase_emptymessage(self, repo, env, users):
         """ When merging between master branches (e.g. forward port), the PR
         may have only a title
         """
@@ -1175,7 +1238,9 @@ class TestMergeMethod:
 
         m = node('M')
         c0 = node('C0', m)
-        expected = node('gibberish\n\ncloses {}#{}'.format(repo.name, prx.number), m, c0)
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
+        expected = node('gibberish\n\ncloses {}#{}'
+                        '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0)
         assert log_to_node(repo.log('heads/master')), expected
 
     def test_pr_mergehead(self, repo, env):
@@ -1214,7 +1279,7 @@ class TestMergeMethod:
         expected = node('C1', node('C0', m1), node('M2', m1))
         assert log_to_node(repo.log('heads/master')), expected
 
-    def test_pr_mergehead_nonmember(self, repo, env):
+    def test_pr_mergehead_nonmember(self, repo, env, users):
         """ if the head of the PR is a merge commit but none of the parents is
         in the target, merge normally
 
@@ -1253,8 +1318,9 @@ class TestMergeMethod:
         assert repo.read_tree(master) == {'a': b'1', 'b': b'2', 'bb': b'bb'}
 
         m1 = node('M1')
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
         expected = node(
-            'T\n\nTT\n\ncloses {}#{}'.format(repo.name, prx.number),
+            'T\n\nTT\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer),
             node('M2', m1),
             node('C1', node('C0', m1), node('B0', m1))
         )
@@ -1556,7 +1622,7 @@ class TestBatching(object):
             ('number', '=', number),
         ])
 
-    def test_staging_batch(self, env, repo):
+    def test_staging_batch(self, env, repo, users):
         """ If multiple PRs are ready for the same target at the same point,
         they should be staged together
         """
@@ -1576,20 +1642,21 @@ class TestBatching(object):
 
         log = list(repo.log('heads/staging.master'))
         staging = log_to_node(log)
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
         p1 = node(
-            'title PR1\n\nbody PR1\n\ncloses {}#{}'.format(repo.name, pr1.number),
+            'title PR1\n\nbody PR1\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer),
             node('initial'),
             node('commit_PR1_01', node('commit_PR1_00', node('initial')))
         )
         p2 = node(
-            'title PR2\n\nbody PR2\n\ncloses {}#{}'.format(repo.name, pr2.number),
+            'title PR2\n\nbody PR2\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer),
             p1,
             node('commit_PR2_01', node('commit_PR2_00', p1))
         )
         expected = (re_matches('^force rebuild'), frozenset([p2]))
         assert staging == expected
 
-    def test_staging_batch_norebase(self, env, repo):
+    def test_staging_batch_norebase(self, env, repo, users):
         """ If multiple PRs are ready for the same target at the same point,
         they should be staged together
         """
@@ -1614,21 +1681,22 @@ class TestBatching(object):
         log = list(repo.log('heads/staging.master'))
 
         staging = log_to_node(log)
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
 
         p1 = node(
-            'title PR1\n\nbody PR1\n\ncloses {}#{}'.format(repo.name, pr1.number),
+            'title PR1\n\nbody PR1\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer),
             node('initial'),
             node('commit_PR1_01', node('commit_PR1_00', node('initial')))
         )
         p2 = node(
-            'title PR2\n\nbody PR2\n\ncloses {}#{}'.format(repo.name, pr2.number),
+            'title PR2\n\nbody PR2\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer),
             p1,
             node('commit_PR2_01', node('commit_PR2_00', node('initial')))
         )
         expected = (re_matches('^force rebuild'), frozenset([p2]))
         assert staging == expected
 
-    def test_staging_batch_squash(self, env, repo):
+    def test_staging_batch_squash(self, env, repo, users):
         """ If multiple PRs are ready for the same target at the same point,
         they should be staged together
         """
@@ -1649,10 +1717,11 @@ class TestBatching(object):
         log = list(repo.log('heads/staging.master'))
 
         staging = log_to_node(log)
+        reviewer = get_partner(env, users["reviewer"]).formatted_email
         expected = node(
             re_matches('^force rebuild'),
-            node('commit_PR2_00\n\ncloses {}#{}'.format(repo.name, pr2.number),
+            node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer),
-                 node('commit_PR1_00\n\ncloses {}#{}'.format(repo.name, pr1.number),
+                 node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer),
                       node('initial'))))
         assert staging == expected
 
@@ -2307,3 +2376,18 @@ def log_to_node(log):
             todo.append(c)
 
     return nodes[log[0]['sha']]
+
+class TestEmailFormatting:
+    def test_simple(self, env):
+        p1 = env['res.partner'].create({
+            'name': 'Bob',
+            'email': 'bob@example.com',
+        })
+        assert p1.formatted_email == 'Bob <bob@example.com>'
+
+    def test_noemail(self, env):
+        p1 = env['res.partner'].create({
+            'name': 'Shultz',
+            'github_login': 'Osmose99',
+        })
+        assert p1.formatted_email == 'Shultz <Osmose99@users.noreply.github.com>'

runbot_merge/tests/test_multirepo.py

@@ -9,7 +9,7 @@ import json
 
 import pytest
 
-from test_utils import re_matches, run_crons
+from test_utils import re_matches, run_crons, get_partner
 
 @pytest.fixture
 def repo_a(make_repo):
@@ -201,13 +201,14 @@ def test_merge_fail(env, project, repo_a, repo_b, users):
         (users['user'], re_matches('^Unable to stage PR')),
     ]
     other = to_pr(env, pr1a)
+    reviewer = get_partner(env, users["reviewer"]).formatted_email
     assert not other.staging_id
     assert [
         c['commit']['message']
         for c in repo_a.log('heads/staging.master')
     ] == [
         re_matches('^force rebuild'),
-        'commit_A2_00\n\ncloses %s#2' % repo_a.name,
+        'commit_A2_00\n\ncloses %s#2\n\nSigned-off-by: %s' % (repo_a.name, reviewer),
         'initial'
     ], "dummy commit + squash-merged PR commit + root commit"
 

runbot_merge/tests/test_utils.py

@@ -17,3 +17,6 @@ def run_crons(env):
     env['runbot_merge.project']._check_progress()
     env['runbot_merge.pull_requests']._check_linked_prs_statuses()
     env['runbot_merge.project']._send_feedback()
+
+def get_partner(env, gh_login):
+    return env['res.partner'].search([('github_login', '=', gh_login)])