Merge pull request #1038 from Aflynn50/fix-consume

#1038

#### Description

`consume` takes a `controller_name` argument. This is not needed and should be automatically generated from the offer url. A deprecation warning is logged and the variable is not used.

Also update offer and consume integration tests to no longer be skipped.

This helps avoid #1031 

#### QA Steps
```
juju bootstrap lxd c1
juju add-model offerer
juju deploy juju-qa-dummy-source dummy-source
juju offer dummy-source:sink
juju bootstrap lxd c2
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("c1:admin/offerer.dummy-source")

# Exit REPL
juju status
# Confirm SAAS exists
juju remove-saas dummy-source
# Confirm removal
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("admin/offerer.dummy-source", controller_name="c1")

# Exit REPL
juju status
# Confirm SAAS exists
juju remove-saas dummy-source
# Confirm removal
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("c1:admin/offerer.dummy-source", controller_name="c1")

```
All CI tests need to pass.

Author: jujubot

juju/model.py

@@ -1526,8 +1526,7 @@ async def integrate(self, relation1, relation2):
                     remote_endpoint.source = current.controller_name
             # consume the remote endpoint
             await self.consume(remote_endpoint.string(),
-                               application_alias=remote_endpoint.application,
-                               controller_name=remote_endpoint.source)
+                               application_alias=remote_endpoint.application)
 
         log.debug(
             'Adding relation %s <-> %s', endpoints[0], endpoints[1])
@@ -2535,7 +2534,12 @@ async def consume(self, endpoint, application_alias="", controller_name=None, co
         """
         Adds a remote offer to the model. Relations can be created later using
         "juju relate".
+
+        If consuming a relation from a model on different controller the
+        controller name must be included in the endpoint. The controller_name
+        argument is being deprecated.
         """
+
         if controller and controller_name:
             raise JujuError("cannot set both controller_name and controller")
         try:
@@ -2549,9 +2553,15 @@ async def consume(self, endpoint, application_alias="", controller_name=None, co
             offer.user = self.info.username
             endpoint = offer.string()
 
-        source = None
         if controller_name:
-            source = await self._get_source_api(offer, controller_name=controller_name)
+            log.warning(
+                'controller_name argument will soon be deprecated, controller '
+                'should be specified in endpoint url')
+            if offer.source == "":
+                offer.source = controller_name
+
+        if offer.source:
+            source = await self._get_source_api(offer)
         else:
             if controller:
                 source = controller
@@ -2766,12 +2776,15 @@ async def revoke_secret(self, secret_name, application, *applications):
         if result_error.error is not None:
             raise JujuAPIError(result_error.error)
 
-    async def _get_source_api(self, url, controller_name=None):
+    async def _get_source_api(self, url):
         controller = Controller()
         if url.has_empty_source():
             async with ConnectedController(self.connection()) as current:
                 if current.controller_name is not None:
                     controller_name = current.controller_name
+        else:
+            controller_name = url.source
+
         await controller.connect(controller_name=controller_name)
         return controller
 

tests/unit/test_offerendpoint.py

@@ -6,6 +6,9 @@
 #
 
 import unittest
+import mock
+from juju.model import Model
+from juju.controller import Controller
 
 from juju.offerendpoints import (LocalEndpoint, OfferEndpoints, OfferURL,
                                  ParseError, maybe_parse_offer_url_source,
@@ -90,3 +93,59 @@ def test_parse_local_endpoint_failures(self):
                 self.assertEqual(e.message, case)
             except Exception:
                 raise
+
+
+class TestConsume(unittest.IsolatedAsyncioTestCase):
+    @mock.patch.object(Model, 'connection')
+    @mock.patch.object(Controller, 'disconnect')
+    @mock.patch('juju.model._create_consume_args')
+    @mock.patch('juju.client.client.ApplicationFacade.from_connection')
+    async def test_external_controller_consume(self, mock_from_connection,
+                                               mock_controller, mock_connection, mock_create_consume_args):
+        """ Test consuming an offer from an external controller. This would be
+        better suited as an integration test however pylibjuju does not allow
+        for bootstrapping of extra controllers.
+        """
+
+        mock_create_consume_args.return_value = None
+        mock_connection.return_value = None
+
+        mock_consume_details = mock.MagicMock()
+        mock_consume_details.offer.offer_url = "user/offerer.app"
+        mock_controller.get_consume_details = mock.AsyncMock(return_value=mock_consume_details)
+        mock_controller.disconnect = mock.AsyncMock()
+
+        mock_facade = mock.MagicMock()
+        mock_from_connection.return_value = mock_facade
+
+        mock_results = mock.MagicMock()
+        mock_results.results = [mock.MagicMock()]
+        mock_results.results[0].error = None
+        mock_facade.Consume = mock.AsyncMock(return_value=mock_results)
+
+        m = Model()
+        m._get_source_api = mock.AsyncMock(return_value=mock_controller)
+
+        # Test with an external controller.
+        offer_url = "externalcontroller:user/offerer.app"
+        await m.consume(offer_url)
+        m._get_source_api.assert_called_once_with(parse_offer_url(offer_url))
+        mock_controller.get_consume_details.assert_called_with("user/offerer.app")
+
+        # Test with an external controller and controller_name arg.
+        offer_url = "externalcontroller:user/offerer.app"
+        await m.consume(offer_url, controller_name="externalcontroller")
+        m._get_source_api.assert_called_with(parse_offer_url(offer_url))
+        mock_controller.get_consume_details.assert_called_with("user/offerer.app")
+
+        # Test with a local (mocked) controller.
+        offer_url = "user/offerer.app"
+        await m.consume(offer_url, controller=mock_controller)
+        mock_controller.get_consume_details.assert_called_with("user/offerer.app")
+
+        # Test with an external controller with just controller_name. This will
+        # soon be deprecated.
+        offer_url = "user/offerer.app"
+        await m.consume(offer_url, controller_name="externalcontroller")
+        m._get_source_api.assert_called_with(parse_offer_url("externalcontroller:user/offerer.app"))
+        mock_controller.get_consume_details.assert_called_with("user/offerer.app")