Merge pull request #907 from jack-w-shaw/JUJU-4048_use_GetChangesMapArgs

#907

Use GetChangesMapArgs for bundle changes instead of GetChanges

Also, drop support for GetChanges when parsing ChangeInfos

GetChanges returns a list of params in a magical order. This makes it brittle and adds lots of special knowledge boiler plate. GetChangesMapArgs was added a few years ago to replace it.

 Wer already support map arg params, we just never do so

#### QA Steps

```
tox -e py3
```

```
tox -e integration -- -m bundle
```

Author: jujubot

juju/bundle.py

@@ -12,7 +12,7 @@
 from toposort import toposort_flatten
 
 from .client import client
-from .constraints import parse as parse_constraints, parse_storage_constraint, parse_device_constraint
+from .constraints import parse as parse_constraints
 from .errors import JujuError
 from . import utils, jasyncio
 from .origin import Channel, Source
@@ -288,7 +288,7 @@ async def fetch_plan(self, bundle_url, origin, overlays=[]):
             _yaml_data.append(yaml.dump(overlay).replace('null', ''))
         yaml_data = "---\n".join(_yaml_data)
 
-        self.plan = await self.bundle_facade.GetChanges(
+        self.plan = await self.bundle_facade.GetChangesMapArgs(
             bundleurl=entity_id,
             yaml=yaml_data)
 
@@ -488,10 +488,12 @@ def sorted(self):
 class ChangeInfo:
     _toPy = {}
 
-    def __init__(self, change_id, requires):
+    def __init__(self, change_id, requires, params=None):
         self.change_id = change_id
         self.requires = requires
 
+        type(self).from_dict(self, params)
+
     @classmethod
     def from_dict(cls, self, data):
         """from_dict converts a data bag into fields on a class instance.
@@ -538,39 +540,6 @@ class AddApplicationChange(ChangeInfo):
     :local_resources: identifies the path to the local resource of the
         application's charm.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(AddApplicationChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.charm = params[0]
-            self.series = params[1]
-            self.application = params[2]
-            self.options = params[3]
-            self.constraints = params[4]
-            self.storage = {k: parse_storage_constraint(v) for k, v in params[5].items()}
-            self.channel = None
-            if len(params) == 8:
-                # Juju 2.4 and below only sends the endpoint bindings and resources
-                self.endpoint_bindings = params[6]
-                self.resources = params[7]
-                self.devices = None
-                self.num_units = None
-            else:
-                # Juju 2.5+ sends devices before endpoint bindings, as well as num_units
-                # There might be placement but we need to ignore that.
-                self.devices = {k: parse_device_constraint(v) for k, v in params[6].items()}
-                self.endpoint_bindings = params[7]
-                self.resources = params[8]
-                self.num_units = params[9]
-                if len(params) > 10:
-                    self.channel = params[10]
-
-        elif isinstance(params, dict):
-            AddApplicationChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -685,26 +654,6 @@ class AddCharmChange(ChangeInfo):
            not sufficient.
         :channel: preferred channel for obtaining the charm.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(AddCharmChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.charm = params[0]
-            self.series = params[1]
-            if len(params) > 2 and params[2] != "":
-                self.channel = params[2]
-            else:
-                self.channel = None
-            if len(params) > 3 and params[3] != "":
-                self.architecture = params[3]
-            else:
-                self.architecture = None
-        elif isinstance(params, dict):
-            AddCharmChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -787,21 +736,6 @@ class AddMachineChange(ChangeInfo):
             "lxc" or kvm"). It is not specified for top level machines.
         :parent_id: id of the parent machine.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(AddMachineChange, self).__init__(change_id, requires)
-        # this one is weird, as it returns a set of parameters inside a list.
-        if isinstance(params, list):
-            options = params[0] or {}
-            self.series = options.get("series")
-            self.constraints = options.get("constraints")
-            self.container_type = options.get("containerType")
-            self.parent_id = options.get("parentId")
-        elif isinstance(params, dict):
-            AddMachineChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -879,18 +813,6 @@ class AddRelationChange(ChangeInfo):
         application, and the interface is optional. Examples are
         "$deploy-42:web", "$deploy-42", "mysql:db".
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(AddRelationChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.endpoint1 = params[0]
-            self.endpoint2 = params[1]
-        elif isinstance(params, dict):
-            AddRelationChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -941,18 +863,6 @@ class AddUnitChange(ChangeInfo):
         :to: optional location where to add the unit, as a placeholder
             pointing to another unit change or to a machine change.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(AddUnitChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.application = params[0]
-            self.to = params[1]
-        elif isinstance(params, dict):
-            AddUnitChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -1010,19 +920,6 @@ class CreateOfferChange(ChangeInfo):
             offer.
         :offer_name: describes the offer name.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(CreateOfferChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.application = params[0]
-            self.endpoints = params[1]
-            self.offer_name = params[2]
-        elif isinstance(params, dict):
-            CreateOfferChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -1065,18 +962,6 @@ class ConsumeOfferChange(ChangeInfo):
         :url: contains the location of the offer
         :application_name: describes the application name on offer.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(ConsumeOfferChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.url = params[0]
-            self.application_name = params[1]
-        elif isinstance(params, dict):
-            ConsumeOfferChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -1121,18 +1006,6 @@ class ExposeChange(ChangeInfo):
             that should be able to access the port ranges that the application
             has opened for each endpoint.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(ExposeChange, self).__init__(change_id, requires)
-
-        self.exposed_endpoints = None
-        if isinstance(params, list):
-            self.application = params[0]
-        elif isinstance(params, dict):
-            ExposeChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -1171,18 +1044,6 @@ class ScaleChange(ChangeInfo):
         :application: placeholder name of the application to be scaled.
         :scale: is the new scale value to use.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(ScaleChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.application = params[0]
-            self.scale = params[1]
-        elif isinstance(params, dict):
-            ScaleChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.
@@ -1224,19 +1085,6 @@ class SetAnnotationsChange(ChangeInfo):
         :entity_type: type of the entity, "application" or "machine".
         :ennotations: annotations as key/value pairs.
     """
-
-    def __init__(self, change_id, requires, params=None):
-        super(SetAnnotationsChange, self).__init__(change_id, requires)
-
-        if isinstance(params, list):
-            self.id = params[0]
-            self.entity_type = params[1]
-            self.annotations = params[2]
-        elif isinstance(params, dict):
-            SetAnnotationsChange.from_dict(self, params)
-        else:
-            raise Exception("unexpected params type")
-
     @staticmethod
     def method():
         """method returns an associated ID for the Juju API call.

tests/integration/test_crossmodel.py

@@ -142,6 +142,7 @@ async def test_relate_with_offer(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_add_bundle(event_loop):
     pytest.skip("skip until we have a faster example to test")
     tests_dir = Path(__file__).absolute().parent

tests/integration/test_model.py

@@ -36,6 +36,7 @@ async def test_model_name(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_local_bundle_dir(event_loop):
     bundle_path = TESTS_DIR / 'bundle'
 
@@ -56,6 +57,7 @@ async def test_deploy_local_bundle_dir(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_local_bundle_file(event_loop):
     bundle_path = TESTS_DIR / 'bundle'
     mini_bundle_file_path = bundle_path / 'mini-bundle.yaml'
@@ -73,6 +75,7 @@ async def test_deploy_local_bundle_file(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle_local_resource_relative_path(event_loop):
     bundle_file_path = INTEGRATION_TEST_DIR / 'bundle-file-resource.yaml'
 
@@ -126,6 +129,7 @@ async def test_deploy_by_revision_validate_flags(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_local_bundle_include_file(event_loop):
     bundle_dir = INTEGRATION_TEST_DIR / 'bundle'
     bundle_yaml_path = bundle_dir / 'bundle-include-file.yaml'
@@ -143,6 +147,7 @@ async def test_deploy_local_bundle_include_file(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_local_bundle_include_base64(event_loop):
     bundle_dir = INTEGRATION_TEST_DIR / 'bundle'
     bundle_yaml_path = bundle_dir / 'bundle-include-base64.yaml'
@@ -159,6 +164,7 @@ async def test_deploy_local_bundle_include_base64(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle_local_charms(event_loop):
     bundle_path = INTEGRATION_TEST_DIR / 'bundle' / 'local.yaml'
 
@@ -174,6 +180,7 @@ async def test_deploy_bundle_local_charms(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_invalid_bundle(event_loop):
     pytest.skip('test_deploy_invalid_bundle intermittent test failure')
     bundle_path = TESTS_DIR / 'bundle' / 'invalid.yaml'
@@ -252,6 +259,7 @@ async def test_wait_local_charm_waiting_timeout(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle(event_loop):
     async with base.CleanModel() as model:
         await model.deploy('anbox-cloud-core', channel='stable',
@@ -263,6 +271,7 @@ async def test_deploy_bundle(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_local_bundle_with_overlay_multi(event_loop):
     async with base.CleanModel() as model:
         bundle_with_overlay_path = OVERLAYS_DIR / 'bundle-with-overlay-multi.yaml'
@@ -276,6 +285,7 @@ async def test_deploy_local_bundle_with_overlay_multi(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle_with_overlay_as_argument(event_loop):
     async with base.CleanModel() as model:
         overlay_path = OVERLAYS_DIR / 'test-overlay.yaml'
@@ -297,6 +307,7 @@ async def test_deploy_bundle_with_overlay_as_argument(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle_with_multi_overlay_as_argument(event_loop):
     async with base.CleanModel() as model:
         overlay_path = OVERLAYS_DIR / 'test-multi-overlay.yaml'
@@ -309,6 +320,7 @@ async def test_deploy_bundle_with_multi_overlay_as_argument(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_bundle_with_multiple_overlays_with_include_files(event_loop):
     async with base.CleanModel() as model:
         bundle_yaml_path = TESTS_DIR / 'integration' / 'bundle' / 'bundle.yaml'
@@ -357,6 +369,7 @@ async def test_deploy_from_ch_channel_revision_success(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_deploy_trusted_bundle(event_loop):
     pytest.skip("skip until we have a deployable bundle available. Right now the landscape-dense fails because postgresql is broken")
     async with base.CleanModel() as model:
@@ -767,6 +780,7 @@ async def test_attach_resource(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_store_resources_bundle(event_loop):
     pytest.skip('test_store_resources_bundle intermittent test failure')
     async with base.CleanModel() as model:
@@ -789,6 +803,7 @@ async def test_store_resources_bundle(event_loop):
 
 @base.bootstrapped
 @pytest.mark.asyncio
+@pytest.mark.bundle
 async def test_store_resources_bundle_revs(event_loop):
     pytest.skip('test_store_resources_bundle_revs intermittent test failure')
     async with base.CleanModel() as model: