Merge ~ilasc/turnip:add-mp-url-to-git-push into turnip:master

[Ioana Lasc]

Add MP URL to git push

[Ioana Lasc (ilasc)]

This still needs Unit Tests.

A sample output from local envs looks like this:

ilasc@thinkpad:~/repo$ git push
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 343 bytes | 343.00 KiB/s, done.
Total 3 (delta 1), reused 0 (delta 0)
remote:
remote: Create a merge proposal for branch 'branch1' on Launchpad by visiting:
remote: https://code.launchpad.test/~ilasc/+git/repo/+ref/branch1/+register-merge
remote:
To git+ssh://git.launchpad.test:9422/~ilasc/+git/repo
   1715591..38a497b branch1 -> branch1
ilasc@thinkpad:~/repo$

e7ad2c0... by Ioana Lasc on 2020-01-08

Addressed review comments.

[Ioana Lasc (ilasc)]

Addressed most code review comments apart from moving the check for the default branch here.

Also needs Unit Tests added.

c1238fe... by Ioana Lasc on 2020-01-23

Move check for default branch from LP to turnip

[Ioana Lasc (ilasc)]

Moved the check for default branch to Turnip per our conversation.

Goes hand in hand with this on the Turnip side:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/377044

Both now ready for review.

I'm not happy with the Unit Tests elements I added here - needs more time to understand what and how we currently test - tried to replicate what we do for checkRefPermissions but in that case we store ref_permissions on the HookRPCHandler for caching purposes I assume, we're currently not doing that for the branch name, so I can't use it in a similar manner in Unit Tests and I don't think we should add it... any comment / suggestion in terms of direction on the Unit Tests would help.

Thanks Colin!

[Colin Watson (cjwatson)]

review: Needs Fixing

6c906f3... by Ioana Lasc on 2020-02-06

Refactor the check for default branch

Refactored the check for default branch into a standalone
method and modified unit tests to pass for current code.

7100052... by Ioana Lasc on 2020-02-06

Add Unit Test for send_mp_url

d274c3f... by Ioana Lasc on 2020-02-06

Cleanup with make lint

0721778... by Ioana Lasc on 2020-02-25

Refactor the check for default branch

[Ioana Lasc (ilasc)]

Thanks Colin! I'm stuck again on the Unit Tests.

This all works when performing functional tests with real repos.

However I can't figure out how to test the refactored send_mp_url in the TestPostReceiveHook, the test method "test_no_merge_proposal_URL" at the bottom is commented out. It's not similar to match_update_rules in that it invokes and rpc call.

Current failure for commented out test_no_merge_proposal_URL is:

File "turnip/pack/hooks/hook.py", line 154, in send_mp_url
    sock, b'get_mp_url',
NameError: global name 'sock' is not defined

I tried a number of things including using the MockHookRPCHandler with a mocked sock but went down a rabbit hole.

[Colin Watson (cjwatson)]

OK, so the NameError is straightforward enough: your send_mp_url function has the wrong signature. It should be send_mp_url(sock, rpc_key, received_lines), and the caller should change appropriately. Without this it doesn't have the right context to be able to make an RPC call.

For the test side, yes, this gets awkward because you need to both mock is_default_branch (not get_default_branch, as your test currently has it) and allow the hook to make an RPC call; so the invokeHook strategy doesn't work easily because you can't mock something in a subprocess without a lot of extra work, and the obvious in-process strategy doesn't work easily because MockSocket is currently one-way.

I can see two approaches to dealing with this. One would be to create an actual git repository on disk with the right structure on the fly. You can change HookTestMixin.setUp to do most of this for all the hook tests: change dir to self.dir, have it call "git init" on self.dir, and then create hookrpc_sock and hooks in os.path.join(self.dir, '.git') rather than directly in self.dir. Individual tests can fiddle with self.dir as needed. Then you don't need to mock is_default_branch; you can let the hook subprocess call it for real. This might actually be the best approach, because it allows the tests to be more realistic and robust: for instance, it would catch any implementation mistakes in is_default_branch, which would otherwise be difficult to find.

If you take this approach, you should use the invokeHook helper to construct your test. You'll also need to make MockHookRPCHandler.getMergeProposalURL be a more useful mock: in particular, it'll need to actually return (or raise) something so that the hook will print it.

If and only if the above approach doesn't work, the other possibility would be to do everything in-process but fix it up to actually work. For this, aside from fixing the signature of send_mp_url as mentioned above, I think all you'd need to do would be to give MockSocket a send method so that it can handle both sides of the connection. I don't recommend this approach because it'll make for a less comprehensive test, but I've tried out enough of it locally to be convinced that it can work.

8307231... by Ioana Lasc on 2020-03-12

Add Unit Test for PostReceiveHook

de10d1e... by Ioana Lasc on 2020-03-13

Add Test to PostReceiveHook

Test that we get an MP URL back when pushing
non default git branch.

[Ioana Lasc (ilasc)]

Thank you Colin! Coded both approaches and committed the first approach, agreed on the fact the second approach leads to confusing tests. MP is now ready for another look.

[Colin Watson (cjwatson)]

I think this looks like basically the right shape now, thanks! Just a few minor comments. Also note that this can't be landed until https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/377044 has been deployed to production, in order to avoid a situation where we have undeployable commits on turnip's master branch.

review: Approve

d5c9cdf... by Ioana Lasc on 2020-03-30

Address code review comments

Added minor changes to unit tests suggested in code review
for the add-mp-url-to-git-push branch.

[Ioana Lasc (ilasc)]

Thanks Colin, last round of updates done and noted that we don't land this until the launchpad side makes it to Prod.

0ed3642... by Ioana Lasc on 2020-05-04

Merge branch 'master' into add-mp-url-to-git-push

6913d7f... by Ioana Lasc on 2020-05-04

Compare full ref paths in check for default branch

diff --git a/turnip/pack/hookrpc.py b/turnip/pack/hookrpc.py
index c5b0d68..bcdcfd1 100644
--- a/turnip/pack/hookrpc.py
+++ b/turnip/pack/hookrpc.py
@@ -179,7 +179,7 @@ class HookRPCHandler(object):
                         TurnipFaultCode.NOT_FOUND,
                         TurnipFaultCode.UNAUTHORIZED,
                         ):
-                    # These faults can happen with unlucky timing: a NOT_FAULT
+                    # These faults can happen with unlucky timing: a NOT_FOUND
                     # fault can happen if the repository was removed from disk
                     # between translatePath and checkRefPermissions (although
                     # that's impossible in practice with Launchpad's
@@ -246,6 +246,53 @@ class HookRPCHandler(object):
             raise
         log_context.log.info("notifyPush done: ref_path={path}", path=path)
 
+    @defer.inlineCallbacks
+    def getMergeProposalURL(self, proto, args):
+        log_context = HookRPCLogContext(self.auth_params[args['key']])
+        path = self.ref_paths[args['key']]
+        auth_params = self.auth_params[args['key']]
+        branch = args['branch']
+        log_context.log.info(
+            "getMergeProposalURL request received: ref_path={path}", path=path)
+        mp_url = None
+        try:
+            proxy = xmlrpc.Proxy(self.virtinfo_url, allowNone=True)
+            mp_url = yield proxy.callRemote(
+                b'getMergeProposalURL', path, branch, auth_params).addTimeout(
+                self.virtinfo_timeout, self.reactor)
+        except xmlrpc.Fault as e:
+            code = translate_xmlrpc_fault(e.faultCode)
+            if code in (
+                    TurnipFaultCode.NOT_FOUND,
+                    TurnipFaultCode.UNAUTHORIZED,
+                    ):
+                # These faults can happen with unlucky timing: a NOT_FOUND
+                # fault can happen if the repository was removed from disk
+                # between translatePath and checkRefPermissions (although
+                # that's impossible in practice with Launchpad's
+                # implementation); similarly, an UNAUTHORIZED fault can
+                # happen if the user's access to the repository was removed
+                # between translatePath and checkRefPermissions.  Just
+                # treat this as if the user has no permissions on any ref.
+                log_context.log.info(
+                    "getMergeProposalURL virtinfo raised Unauthorized: "
+                    "auth_params={auth_params}, ref_path={ref_path}",
+                    auth_params=auth_params, branch=branch)
+            else:
+                log_context.log.info(
+                    "getMergeProposalURL virtinfo raised "
+                    "{fault_code} {fault_string}: "
+                    "auth_params={auth_params}, ref_path={ref_path}",
+                    fault_code=code.name, fault_string=e.faultString,
+                    auth_params=auth_params, branch=branch)
+        except defer.TimeoutError:
+            log_context.log.info(
+                "getMergeProposalURL timed out: ref_path={path}", path=path)
+            raise
+        log_context.log.info("getMergeProposalURL done: ref_path={path}",
+                             path=path)
+        defer.returnValue(mp_url)
+
 
 class HookRPCServerFactory(RPCServerFactory):
     """A JSON netstring RPC interface to a HookRPCHandler."""
@@ -255,4 +302,5 @@ class HookRPCServerFactory(RPCServerFactory):
         self.methods = {
             'check_ref_permissions': self.hookrpc_handler.checkRefPermissions,
             'notify_push': self.hookrpc_handler.notifyPush,
+            'get_mp_url': self.hookrpc_handler.getMergeProposalURL,
             }
diff --git a/turnip/pack/hooks/hook.py b/turnip/pack/hooks/hook.py
index 30189b1..e4b6886 100755
--- a/turnip/pack/hooks/hook.py
+++ b/turnip/pack/hooks/hook.py
@@ -16,7 +16,6 @@ import socket
 import subprocess
 import sys
 
-
 # XXX twom 2018-10-23 This should be a pygit2 import, but
 # that currently causes CFFI warnings to be returned to the client.
 GIT_OID_HEX_ZERO = '0'*40
@@ -32,6 +31,12 @@ def check_ancestor(old, new):
     return return_code == 0
 
 
+def is_default_branch(pushed_branch):
+    default_branch = subprocess.check_output(
+        ['git', 'symbolic-ref', 'HEAD']).rstrip(b'\n')
+    return pushed_branch == default_branch
+
+
 def determine_permissions_outcome(old, ref, rule_lines):
     rule = rule_lines.get(ref, [])
     if old == GIT_OID_HEX_ZERO:
@@ -133,13 +138,32 @@ def check_ref_permissions(sock, rpc_key, ref_paths):
         for path, permissions in rule_lines.items()}
 
 
+def send_mp_url(received_line):
+    ref = received_line.rstrip(b'\n').split(b' ', 2)[2]
+
+    # check for branch ref here (we're interested in
+    # heads and not tags)
+
+    if ref.startswith(b'refs/heads/') and not is_default_branch(ref):
+        pushed_branch = ref[len(b'refs/heads/'):]
+        if not is_default_branch(pushed_branch):
+            mp_url = rpc_invoke(
+                sock, b'get_mp_url',
+                {'key': rpc_key, 'branch': pushed_branch})
+            if mp_url is not None:
+                sys.stdout.write(b'      \n')
+                sys.stdout.write(
+                    b"Create a merge proposal for '%s' on Launchpad by"
+                    b" visiting:\n" % pushed_branch)
+                sys.stdout.write(b'      %s\n' % mp_url)
+                sys.stdout.write(b'      \n')
+
 if __name__ == '__main__':
     # Connect to the RPC server, authenticating using the random key
     # from the environment.
     rpc_key = os.environ['TURNIP_HOOK_RPC_KEY']
     sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
     sock.connect(os.environ['TURNIP_HOOK_RPC_SOCK'])
-
     hook = os.path.basename(sys.argv[0])
     if hook == 'pre-receive':
         # Verify the proposed changes against rules from the server.
@@ -153,8 +177,11 @@ if __name__ == '__main__':
     elif hook == 'post-receive':
         # Notify the server about the push if there were any changes.
         # Details of the changes aren't currently included.
-        if sys.stdin.readlines():
+        lines = sys.stdin.readlines()
+        if lines:
             rpc_invoke(sock, b'notify_push', {'key': rpc_key})
+        if len(lines) == 1:
+            send_mp_url(lines[0])
         sys.exit(0)
     elif hook == 'update':
         ref = sys.argv[1]
diff --git a/turnip/pack/tests/fake_servers.py b/turnip/pack/tests/fake_servers.py
index 0914637..c0c4015 100644
--- a/turnip/pack/tests/fake_servers.py
+++ b/turnip/pack/tests/fake_servers.py
@@ -62,6 +62,8 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
         self.translations = []
         self.authentications = []
         self.push_notifications = []
+        self.merge_proposal_url = 'http://bogus.test'
+        self.merge_proposal_url_fault = None
         self.ref_permissions_checks = []
         self.ref_permissions = {}
         self.ref_permissions_fault = None
@@ -94,3 +96,9 @@ class FakeVirtInfoService(xmlrpc.XMLRPC):
         return [
             (xmlrpc_client.Binary(ref), permissions)
             for ref, permissions in self.ref_permissions.items()]
+
+    def xmlrpc_getMergeProposalURL(self, path, branch, auth_params):
+        if self.merge_proposal_url_fault is not None:
+            raise self.merge_proposal_url_fault
+        else:
+            return self.merge_proposal_url
diff --git a/turnip/pack/tests/test_hookrpc.py b/turnip/pack/tests/test_hookrpc.py
index e0344f5..a4b0d19 100644
--- a/turnip/pack/tests/test_hookrpc.py
+++ b/turnip/pack/tests/test_hookrpc.py
@@ -341,3 +341,46 @@ class TestHookRPCHandler(TestCase):
             clock.advance(15)
             self.assertTrue(d.called)
             return assert_fails_with(d, defer.TimeoutError)
+
+    @defer.inlineCallbacks
+    def test_getMergeProposalURL(self):
+        clock = task.Clock()
+        self.hookrpc_handler = hookrpc.HookRPCHandler(
+            self.virtinfo_url, 15, reactor=clock)
+
+        with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
+            mp_url = yield self.hookrpc_handler.getMergeProposalURL(
+                None, {'key': key, 'branch': 'master', 'uid': 4})
+        self.assertIsNotNone(mp_url)
+
+    def test_getMergeProposalURL_timeout(self):
+        clock = task.Clock()
+        self.hookrpc_handler = hookrpc.HookRPCHandler(
+            self.virtinfo_url, 15, reactor=clock)
+
+        with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
+            d = self.hookrpc_handler.getMergeProposalURL(
+                None, {'key': key, 'branch': 'master', 'uid': 4})
+            clock.advance(1)
+            self.assertFalse(d.called)
+            clock.advance(15)
+            self.assertTrue(d.called)
+            return assert_fails_with(d, defer.TimeoutError)
+
+    @defer.inlineCallbacks
+    def test_getMergeProposalURL_unauthorized(self):
+        # we return None for the merge proposal URL
+        # when catching and UNAUTHORIZED and NOT_FOUND exception
+        self.virtinfo.merge_proposal_url_fault = xmlrpc.Fault(
+            3, 'Unauthorized')
+        with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
+            mp_url = yield self.hookrpc_handler.getMergeProposalURL(
+                None, {'key': key, 'branch': 'master', 'uid': 4})
+        self.assertIsNone(mp_url)
+
+        self.virtinfo.merge_proposal_url_fault = xmlrpc.Fault(
+            1, 'NOT_FOUND')
+        with self.registeredKey('/translated', auth_params={'uid': 42}) as key:
+            mp_url = yield self.hookrpc_handler.getMergeProposalURL(
+                None, {'key': key, 'branch': 'master', 'uid': 4})
+        self.assertIsNone(mp_url)
diff --git a/turnip/pack/tests/test_hooks.py b/turnip/pack/tests/test_hooks.py
index ce92c3a..4e793da 100644
--- a/turnip/pack/tests/test_hooks.py
+++ b/turnip/pack/tests/test_hooks.py
@@ -10,6 +10,7 @@ from __future__ import (
 
 import base64
 import os.path
+import subprocess
 import uuid
 
 from fixtures import (
@@ -67,6 +68,9 @@ class MockHookRPCHandler(hookrpc.HookRPCHandler):
             base64.b64encode(ref).decode('UTF-8'): permissions
             for ref, permissions in self.ref_permissions[args['key']].items()}
 
+    def getMergeProposalURL(self, proto, args):
+        return 'http://mp-url.test'
+
 
 class MockRef(object):
 
@@ -141,14 +145,16 @@ class HookTestMixin(object):
         super(HookTestMixin, self).setUp()
         self.hookrpc_handler = MockHookRPCHandler()
         self.hookrpc = hookrpc.HookRPCServerFactory(self.hookrpc_handler)
-        dir = self.useFixture(TempDir()).path
-        self.hookrpc_sock_path = os.path.join(dir, 'hookrpc_sock')
+        self.repo_dir = os.path.join(self.useFixture(TempDir()).path, '.git')
+        os.mkdir(self.repo_dir)
+        subprocess.check_output(['git', 'init', self.repo_dir])
+        self.hookrpc_sock_path = os.path.join(self.repo_dir, 'hookrpc_sock')
         self.hookrpc_port = reactor.listenUNIX(
             self.hookrpc_sock_path, self.hookrpc)
         self.addCleanup(self.hookrpc_port.stopListening)
-        hooks_dir = os.path.join(dir, 'hooks')
+        hooks_dir = os.path.join(self.repo_dir, 'hooks')
         os.mkdir(hooks_dir)
-        ensure_hooks(dir)
+        ensure_hooks(self.repo_dir)
         self.hook_path = os.path.join(hooks_dir, self.hook_name)
 
     def encodeRefs(self, updates):
@@ -185,6 +191,16 @@ class HookTestMixin(object):
             self.encodeRefs(updates), permissions)
         self.assertEqual((1, message, b''), (code, out, err))
 
+    @defer.inlineCallbacks
+    def assertMergeProposalURLReceived(self, updates, permissions):
+        mp_url_message = (
+            b"Create a merge proposal for '%s' on Launchpad by"
+            b" visiting:\n" % updates[0][0].split(b'/', 2)[2])
+        code, out, err = yield self.invokeHook(
+            self.encodeRefs(updates), permissions)
+        self.assertEqual((0, b''), (code, err))
+        self.assertIn(mp_url_message, out)
+
 
 class TestPreReceiveHook(HookTestMixin, TestCase):
     """Tests for the git pre-receive hook."""
@@ -247,7 +263,7 @@ class TestPostReceiveHook(HookTestMixin, TestCase):
     @defer.inlineCallbacks
     def test_notifies(self):
         # The notification callback is invoked with the storage path.
-        yield self.assertAccepted(
+        yield self.assertMergeProposalURLReceived(
             [(b'refs/heads/foo', self.old_sha1, self.new_sha1)], [])
         self.assertEqual(['/translated'], self.hookrpc_handler.notifications)
 
@@ -257,6 +273,39 @@ class TestPostReceiveHook(HookTestMixin, TestCase):
         yield self.assertAccepted([], [])
         self.assertEqual([], self.hookrpc_handler.notifications)
 
+    @defer.inlineCallbacks
+    def test_merge_proposal_URL(self):
+        # MP URL received for push of non-default branch
+        curdir = os.getcwd()
+        try:
+            os.chdir(self.repo_dir)
+            default_branch = subprocess.check_output(
+                ['git', 'symbolic-ref', 'HEAD']
+                ).rstrip(b'\n')
+            pushed_branch = str(default_branch + 'notdefault')
+            yield self.assertMergeProposalURLReceived([(
+                b'%s' % pushed_branch,
+                self.old_sha1, self.new_sha1)],
+                {b'%s' % pushed_branch: ['push']})
+        finally:
+            os.chdir(curdir)
+
+    @defer.inlineCallbacks
+    def test_no_merge_proposal_URL(self):
+        # No MP URL received for push of default branch
+        curdir = os.getcwd()
+        try:
+            os.chdir(self.repo_dir)
+            default_branch = subprocess.check_output(
+                ['git', 'symbolic-ref', 'HEAD']
+                ).rstrip(b'\n')
+            yield self.assertAccepted([(
+                b'%s' % default_branch,
+                self.old_sha1, self.new_sha1)],
+                {b'%s' % default_branch: ['push']})
+        finally:
+            os.chdir(curdir)
+
 
 class TestUpdateHook(TestCase):
     """Tests for the git update hook"""