Merge pull request #488 from juju/johnsca/idempotent-bundle-deploy

#488

When a bundle is deployed which contains an application which already exists in the model, the CLI will skip adding the application but libjuju will raise an error. This fixes the latter behavior.

Author: jujubot

juju/bundle.py

@@ -329,6 +329,13 @@ async def run(self, context):
         :param context: is used for any methods or properties required to
             perform a change.
         """
+        # NB: this should really be handled by the controller when generating the
+        # bundle change plan, and this short-term workaround may be missing some
+        # aspects of the logic which the CLI client contains to handle edge cases.
+        if self.application in context.model.applications:
+            log.debug('Skipping %s; already in model', self.application)
+            return self.application
+
         # resolve indirect references
         charm = context.resolve(self.charm)
         options = {}
@@ -579,6 +586,14 @@ async def run(self, context):
         ep1 = context.resolveRelation(self.endpoint1)
         ep2 = context.resolveRelation(self.endpoint2)
 
+        # NB: this should really be handled by the controller when generating the
+        # bundle change plan, and this short-term workaround may be missing some
+        # aspects of the logic which the CLI client contains to handle edge cases.
+        existing = [rel for rel in context.model.relations if rel.matches(ep1, ep2)]
+        if existing:
+            log.info('Skipping %s <-> %s; already related', ep1, ep2)
+            return existing[0]
+
         log.info('Relating %s <-> %s', ep1, ep2)
         return await context.model.add_relation(ep1, ep2)
 

tests/unit/test_bundle.py

@@ -166,6 +166,7 @@ async def test_run(self, event_loop):
         model = mock.Mock()
         model._deploy = base.AsyncMock(return_value=None)
         model._add_store_resources = base.AsyncMock(return_value=["resource1"])
+        model.applications = {}
 
         context = mock.Mock()
         context.resolve.return_value = "charm1"
@@ -192,6 +193,13 @@ async def test_run(self, event_loop):
                                          devices="devices",
                                          num_units="num_units")
 
+        # confirm that it's idempotent
+        model.applications = {"application": None}
+        result = await change.run(context)
+        assert result == "application"
+        model._add_store_resources.assert_called_once()
+        model._deploy.assert_called_once()
+
     @pytest.mark.asyncio
     async def test_run_local(self, event_loop):
         change = AddApplicationChange(1, [], params={"charm": "local:charm",
@@ -206,6 +214,7 @@ async def test_run_local(self, event_loop):
 
         model = mock.Mock()
         model._deploy = base.AsyncMock(return_value=None)
+        model.applications = {}
 
         context = mock.Mock()
         context.resolve.return_value = "local:charm1"
@@ -407,19 +416,32 @@ async def test_run(self, event_loop):
         change = AddRelationChange(1, [], params={"endpoint1": "endpoint1",
                                                   "endpoint2": "endpoint2"})
 
+        rel1 = mock.Mock(name="rel1", **{"matches.return_value": False})
+        rel2 = mock.Mock(name="rel2", **{"matches.return_value": True})
+
         model = mock.Mock()
-        model.add_relation = base.AsyncMock(return_value="relation1")
+        model.add_relation = base.AsyncMock(return_value=rel2)
 
         context = mock.Mock()
         context.resolveRelation = mock.Mock(side_effect=['endpoint_1', 'endpoint_2'])
         context.model = model
+        model.relations = [rel1]
 
         result = await change.run(context)
-        assert result == "relation1"
+        assert result is rel2
 
         model.add_relation.assert_called_once()
         model.add_relation.assert_called_with("endpoint_1", "endpoint_2")
 
+        # confirm that it's idempotent
+        context.resolveRelation.side_effect = ['endpoint_1', 'endpoint_2']
+        model.add_relation.reset_mock()
+        model.add_relation.return_value = None
+        model.relations = [rel1, rel2]
+        result = await change.run(context)
+        assert result is rel2
+        assert not model.add_relation.called
+
 
 class TestAddUnitChange(unittest.TestCase):