Merge pull request #697 from cderici/fix-no-model-name-case

#697

#### Description

This PR fixes the `model.connect_current()` call which currently fails at the after_connect where we pull up the model info. The reason of the failure is that we neither have a `model_name`, nor a `model_uuid` for the controller to pull up the info. To solve it, this change gets the `model_uuid` from the connector during connect (if we don't use the endpoints, in which case we don't have a problem to begin with).

#### QA Steps

This change also introduces an integration test, namely, `test_connect_current`, so the following should pass.

```
tox -e integration -- tests/integration/test_model.py::test_connect_current
```

#### Notes & Discussion

Author: jujubot

examples/config.py

@@ -37,12 +37,12 @@ async def main():
     # update and check app config
     await ubuntu_app.set_config({'tuning-level': 'fast'})
     config = await ubuntu_app.get_config()
-    assert(config['tuning-level']['value'] == 'fast')
+    assert (config['tuning-level']['value'] == 'fast')
 
     # update and check app constraints
     await ubuntu_app.set_constraints({'mem': 512 * MB})
     constraints = await ubuntu_app.get_constraints()
-    assert(constraints['mem'] == 512 * MB)
+    assert (constraints['mem'] == 512 * MB)
 
     await model.disconnect()
 

examples/relate.py

@@ -17,14 +17,14 @@
 class MyRemoveObserver(ModelObserver):
     async def on_change(self, delta, old, new, model):
         if delta.type == 'remove':
-            assert(new.latest() == new)
-            assert(new.next() is None)
-            assert(new.dead)
-            assert(new.current)
-            assert(new.connected)
-            assert(new.previous().dead)
-            assert(not new.previous().current)
-            assert(not new.previous().connected)
+            assert (new.latest() == new)
+            assert (new.next() is None)
+            assert (new.dead)
+            assert (new.current)
+            assert (new.connected)
+            assert (new.previous().dead)
+            assert (not new.previous().current)
+            assert (not new.previous().connected)
 
 
 class MyModelObserver(ModelObserver):

juju/client/connection.py

@@ -595,7 +595,7 @@ async def rpc(self, msg, encoder=None):
         '''
         self.__request_id__ += 1
         msg['request-id'] = self.__request_id__
-        if'params' not in msg:
+        if 'params' not in msg:
             msg['params'] = {}
         if "version" not in msg:
             msg['version'] = self.facades[msg['type']]

juju/client/connector.py

@@ -113,7 +113,7 @@ async def connect_controller(self, controller_name=None, specified_facades=None)
         self.controller_name = controller_name
         self.controller_uuid = controller["uuid"]
 
-    async def connect_model(self, model_name=None, **kwargs):
+    async def connect_model(self, _model_name=None, **kwargs):
         """Connect to a model by name. If either controller or model
         parts of the name are empty, the current controller and/or model
         will be used.
@@ -122,7 +122,7 @@ async def connect_model(self, model_name=None, **kwargs):
         """
 
         try:
-            controller_name, model_name = self.jujudata.parse_model(model_name)
+            controller_name, _model_name = self.jujudata.parse_model(_model_name)
             controller = self.jujudata.controllers().get(controller_name)
         except JujuError as e:
             raise JujuConnectionError(e.message) from e
@@ -134,8 +134,8 @@ async def connect_model(self, model_name=None, **kwargs):
         models = self.jujudata.models().get(controller_name, {}).get('models',
                                                                      {})
         model_uuid = None
-        if model_name in models:
-            model_uuid = models[model_name]['uuid']
+        if _model_name in models:
+            model_uuid = models[_model_name]['uuid']
         else:
             # let's try to find it through the actual controller
             await self.connect_controller(controller_name=controller_name)
@@ -146,11 +146,11 @@ async def connect_model(self, model_name=None, **kwargs):
             response = await controller_facade.AllModels()
             # search the one that contains admin/model_name
             for user_model in response.user_models:
-                if 'admin/' + user_model.model.name == model_name:
+                if 'admin/' + user_model.model.name == _model_name:
                     model_uuid = user_model.model.uuid
 
         if model_uuid is None:
-            raise JujuConnectionError('Model not found: {}'.format(model_name))
+            raise JujuConnectionError('Model not found: {}'.format(_model_name))
 
         proxy = proxy_from_config(controller.get('proxy-config', None))
 
@@ -167,7 +167,8 @@ async def connect_model(self, model_name=None, **kwargs):
         # TODO this might be a good spot to trigger refreshing the
         # local cache (the connection to the model might help)
         self.controller_name = controller_name
-        self.model_name = controller_name + ':' + model_name
+        self.model_name = controller_name + ':' + _model_name
+        return self.controller_name, model_uuid
 
     def bakery_client_for_controller(self, controller_name):
         '''Make a copy of the bakery client with a the appropriate controller's

juju/controller.py

@@ -499,7 +499,10 @@ async def get_model_info(self, model_name=None, model_uuid=None):
         facade = client.ModelManagerFacade.from_connection(self.connection())
         if model_uuid is None:
             uuids = await self.model_uuids()
-            model_uuid = uuids[model_name]
+            try:
+                model_uuid = uuids[model_name]
+            except KeyError:
+                raise errors.JujuError("{} is not among the models in the controller : {}".format(model_name, uuids))
         entity = client.Entity(tag.model(model_uuid))
         _model_info_results = await facade.ModelInfo(entities=[entity])
         return _model_info_results.results[0].result

juju/model.py

@@ -690,14 +690,16 @@ async def connect(self, *args, **kwargs):
             await self.disconnect()
         model_name = model_uuid = None
         if 'endpoint' not in kwargs and len(args) < 2:
+            # Then we're using the model_name to pick the model
             if args and 'model_name' in kwargs:
                 raise TypeError('connect() got multiple values for model_name')
             elif args:
                 model_name = args[0]
             else:
                 model_name = kwargs.pop('model_name', None)
-            await self._connector.connect_model(model_name, **kwargs)
+            _, model_uuid = await self._connector.connect_model(model_name, **kwargs)
         else:
+            # Then we're using the endpoint to pick the model
             if 'model_name' in kwargs:
                 raise TypeError('connect() got values for both '
                                 'model_name and endpoint')

tests/integration/test_model.py

@@ -445,7 +445,7 @@ async def add_manual_machine_ssh(event_loop, is_root=False):
         def wait_for_network(container, timeout=30):
             """Wait for eth0 to have an ipv4 address."""
             starttime = time.time()
-            while(time.time() < starttime + timeout):
+            while time.time() < starttime + timeout:
                 time.sleep(1)
                 if 'eth0' in container.state().network:
                     addresses = container.state().network['eth0']['addresses']
@@ -1012,6 +1012,14 @@ async def test_connect_to_connection(event_loop):
         await m.disconnect()
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_connect_current(event_loop):
+    m = Model()
+    await m.connect_current()
+    assert m.is_connected()
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_model_cache_update(event_loop):

tests/unit/test_model.py

@@ -170,20 +170,23 @@ class TestModelConnect(asynctest.TestCase):
     @pytest.mark.asyncio
     async def test_no_args(self, mock_connect_model, _):
         m = Model()
+        mock_connect_model.side_effect = [("_", "uuid")]
         await m.connect()
         mock_connect_model.assert_called_once_with(None)
 
     @asynctest.patch('juju.client.connector.Connector.connect_model')
     @pytest.mark.asyncio
     async def test_with_model_name(self, mock_connect_model, _):
         m = Model()
+        mock_connect_model.side_effect = [("_", "uuid")]
         await m.connect(model_name='foo')
         mock_connect_model.assert_called_once_with('foo')
 
     @asynctest.patch('juju.client.connector.Connector.connect_model')
     @pytest.mark.asyncio
     async def test_with_endpoint_but_no_uuid(self, mock_connect_model, _):
         m = Model()
+        mock_connect_model.side_effect = [("_", "uuid")]
         with self.assertRaises(TypeError):
             await m.connect(endpoint='0.1.2.3:4566')
         self.assertEqual(mock_connect_model.call_count, 0)
@@ -251,6 +254,7 @@ async def test_with_endpoint_and_uuid_with_macaroon(self, mock_connect, _):
     @asynctest.patch('juju.client.connector.Connector.connect')
     async def test_with_posargs(self, mock_connect, mock_connect_model, _):
         m = Model()
+        mock_connect_model.side_effect = [("_", "uuid")]
         await m.connect('foo')
         mock_connect_model.assert_called_once_with('foo')
         with self.assertRaises(TypeError):