Merge pull request #729 from cderici/revise-application-upgrade-charm

#729

#### Description

The `application.refresh()` method was crashing due to the resources checks because of the old CharmStore code. This adds a new method, namely `list_resources` into the charmhub module and replaces the old code with the new CharmHub checks using the new method.

Flixes: #728 


#### QA Steps

Two tests were added, one for the `list_resources` method, and one for the `application.upgrade_charm`. Both of them need to pass, along with the other tests (modulo, we might still have some of those known intermittent CI failures).

```
tox -e integration -- tests/integration/test_charmhub.py::test_list_resources
```

```
tox -e integration -- tests/integration/test_application.py::test_upgrade_charm_switch_channel
```

The `test_upgrade_charm_switch_channel` also includes the reproduce/test section from #728, so no need for a manual check.

All CI tests need to pass.

Author: jujubot

juju/application.py

@@ -18,12 +18,14 @@
 import pathlib
 
 from . import model, tag, utils, jasyncio
+from .url import URL, Schema
 from .status import derive_status
 from .annotationhelper import _get_annotations, _set_annotations
 from .client import client
 from .errors import JujuError, JujuApplicationConfigError
 from .bundle import get_charm_series
 from .placement import parse as parse_placement
+from .origin import Channel
 
 log = logging.getLogger(__name__)
 
@@ -641,92 +643,135 @@ async def refresh(
         if switch is not None and revision is not None:
             raise ValueError("switch and revision are mutually exclusive")
 
-        charms_facade = client.CharmsFacade.from_connection(self.connection)
-        resources_facade = client.ResourcesFacade.from_connection(
-            self.connection)
         app_facade = self._facade()
+        resources_facade = client.ResourcesFacade.from_connection(self.connection)
+        charms_facade = client.CharmsFacade.from_connection(self.connection)
 
-        charmstore = self.model.charmstore
-        charmstore_entity = None
-
-        if switch is not None:
-            charm_url = switch
-            if not charm_url.startswith('cs:'):
-                charm_url = 'cs:' + charm_url
+        # 1 - Figure out the destination origin and destination charm_url
+        # 2 - Then take care of the resources
+        # 3 - Finally execute the upgrade
+
+        # Get the charm URL and charm origin of the given application is running at present.
+        charm_url_origin_result = await app_facade.GetCharmURLOrigin(application=self.name)
+        if charm_url_origin_result.error is not None:
+            err = charm_url_origin_result.error
+            raise JujuError(f'{err.code} : {err.message}')
+        charm_url = switch or charm_url_origin_result.url
+        origin = charm_url_origin_result.charm_origin
+
+        parsed_url = URL.parse(charm_url)
+        charm_name = parsed_url.name
+
+        if parsed_url.schema is None:
+            raise JujuError(f'A ch: or cs: schema is required for application refresh, given : {str(parsed_url)}')
+
+        if revision is not None:
+            origin.revision = revision
+
+        # Make the source-specific changes to the origin/channel/url
+        # (and also get the resources necessary to deploy the (destination) charm -- for later)
+        if Schema.CHARM_HUB.matches(parsed_url.schema):
+            origin.source = 'charm-hub'
+            if channel:
+                ch = Channel.parse(channel).normalize()
+                origin.risk = ch.risk
+                origin.track = ch.track
+
+            charmhub = self.model.charmhub
+            charm_resources = await charmhub.list_resources(charm_name)
         else:
-            charm_url = self.data['charm-url']
-            charm_url = charm_url.rpartition('-')[0]
-            if revision is not None:
-                charm_url = "%s-%d" % (charm_url, revision)
-            else:
-                charmstore_entity = await charmstore.entity(charm_url,
-                                                            channel=channel)
-                charm_url = charmstore_entity['Id']
-
-        if charm_url == self.data['charm-url']:
-            raise JujuError('already running charm "%s"' % charm_url)
-
-        # TODO (caner) : this needs to be revisited and updated with the charmhub stuff
-        origin = client.CharmOrigin(source="charm-store",
-                                    risk=channel,
-                                    )
-        # Update charm
-        await charms_facade.AddCharm(
-            url=charm_url,
-            force=force,
-            charm_origin=origin,
-        )
-
-        # Update resources
-        if not charmstore_entity:
-            charmstore_entity = await charmstore.entity(charm_url,
-                                                        channel=channel)
-        store_resources = charmstore_entity['Meta']['resources']
-
+            charmstore = self.model.charmstore
+            charmstore_entity = None
+            if switch is None:
+                charm_url = charm_url.rpartition('-')[0]
+                if revision is not None:
+                    charm_url = "%s-%d" % (charm_url, revision)
+                else:
+                    charmstore_entity = await charmstore.entity(charm_url, channel=channel)
+                    charm_url = charmstore_entity['Id']
+            origin.source = 'charm-store'
+            if channel:
+                origin.risk = channel
+            if charmstore_entity is None:
+                charmstore_entity = await charmstore.entity(charm_url, channel=channel)
+            charm_resources = charmstore_entity['Meta']['resources']
+
+        # Resolve the given charm URLs with an optionally specified preferred channel.
+        # Channel provided via CharmOrigin.
+        resolved_charm_with_channel_results = await charms_facade.ResolveCharms(
+            resolve=[client.ResolveCharmWithChannel(
+                charm_origin=origin,
+                switch_charm=True if switch else False,  # rpc expects boolean type
+                reference=charm_url,
+            )])
+        resolved_charm = resolved_charm_with_channel_results.results[0]
+
+        # Get the destination origin and destination charm_url from the resolved charm
+        if resolved_charm.error is not None:
+            err = resolved_charm.error
+            raise JujuError(f'{err.code} : {err.message}')
+        dest_origin = resolved_charm.charm_origin
+        charm_url = resolved_charm.url
+
+        # Add the charm with the destination url and origin
+        charm_origin_result = await charms_facade.AddCharm(url=charm_url,
+                                                           force=force,
+                                                           charm_origin=dest_origin)
+        if charm_origin_result.error is not None:
+            err = charm_origin_result.error
+            raise JujuError(f'{err.code} : {err.message}')
+
+        # Now take care of the resources:
+
+        # Already prepped the charm_resources
+        # Now get the existing resources from the ResourcesFacade
         request_data = [client.Entity(self.tag)]
         response = await resources_facade.ListResources(entities=request_data)
         existing_resources = {
             resource.name: resource
             for resource in response.results[0].resources
         }
 
+        # Compute the difference btw resources needed and the existing resources
         resources_to_update = [
-            resource for resource in store_resources
-            if resource['Name'] not in existing_resources or
-            existing_resources[resource['Name']].origin != 'upload'
+            resource for resource in charm_resources
+            if resource.get('Name', resource.get('name')) not in existing_resources or
+            existing_resources[resource.get('Name', resource.get('name'))].origin != 'upload'
         ]
 
+        # Update the resources
         if resources_to_update:
-            request_data = [
-                client.CharmResource(
-                    description=resource.get('Description'),
-                    fingerprint=resource['Fingerprint'],
-                    name=resource['Name'],
-                    path=resource['Path'],
-                    revision=resource['Revision'],
-                    size=resource['Size'],
-                    type_=resource['Type'],
+            request_data = []
+            for resource in resources_to_update:
+                request_data.append(client.CharmResource(
+                    description=resource.get('Description', resource.get('description')),
+                    fingerprint=resource.get('Fingerprint', resource.get('fingerprint')),
+                    name=resource.get('Name', resource.get('name')),
+                    path=resource.get('Path', resource.get('filename')),
+                    revision=resource.get('Revision', resource.get('revision', -1)),
+                    size=resource.get('Size', resource.get('size')),
+                    type_=resource.get('Type', resource.get('type')),
                     origin='store',
-                ) for resource in resources_to_update
-            ]
+                ))
             response = await resources_facade.AddPendingResources(
                 application_tag=self.tag,
                 charm_url=charm_url,
-                resources=request_data
+                resources=request_data,
+                charm_origin=dest_origin,
             )
             pending_ids = response.pending_ids
             resource_ids = {
-                resource['Name']: id
+                resource.get('Name', resource.get('name')): id
                 for resource, id in zip(resources_to_update, pending_ids)
             }
         else:
             resource_ids = None
 
-        # Update application
+        # Update the application
         await app_facade.SetCharm(
             application=self.entity_id,
-            channel=channel,
             charm_url=charm_url,
+            charm_origin=dest_origin,
             config_settings=None,
             config_settings_yaml=None,
             force=force,

juju/charmhub.py

@@ -41,6 +41,16 @@ async def is_subordinate(self, charm_name):
     # TODO (caner) : we should be able to recreate the channel-map through the
     #  api call without needing the CharmHub facade
 
+    async def list_resources(self, charm_name):
+        conn, headers, path_prefix = self.model.connection().https_connection()
+
+        model_conf = await self.model.get_config()
+        charmhub_url = model_conf['charmhub-url']
+        url = "{}/v2/charms/info/{}?fields=default-release.resources".format(charmhub_url.value, charm_name)
+        _response = self.request_charmhub_with_retry(url, 5)
+        response = json.loads(_response.text)
+        return response['default-release']['resources']
+
     async def info(self, name, channel=None):
         """info displays detailed information about a CharmHub charm. The charm
         can be specified by the exact name.

tests/integration/test_application.py

@@ -1,12 +1,16 @@
 from pathlib import Path
 
 import pytest
+import logging
 
 from .. import base
-from juju import jasyncio
+from juju import jasyncio, errors
+from juju.url import URL, Schema
 
 MB = 1
 
+logger = logging.getLogger(__name__)
+
 
 @base.bootstrapped
 @pytest.mark.asyncio
@@ -167,6 +171,50 @@ async def test_upgrade_charm_channel(event_loop):
         assert app.data['charm-url'] != 'cs:ubuntu-0'
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_upgrade_charm_switch_channel(event_loop):
+    # Note for future:
+    # This test requires a charm that has different
+    # revisions for different channels/risks.
+    # Currently, we use juju-qa-test, but eventually
+    # (when the 'edge' moves to 'stable') this test
+    # will be testing nothing (if not failing).
+    # So checks are in place for that.
+
+    async with base.CleanModel() as model:
+        app = await model.deploy('juju-qa-test', channel='2.0/stable')
+        await model.wait_for_idle(status='active')
+
+        charm_url = URL.parse(app.data['charm-url'])
+        assert Schema.CHARM_HUB.matches(charm_url.schema)
+        still22 = False
+        try:
+            assert charm_url.revision == 22
+            still22 = True
+        except AssertionError:
+            logger.warning("Charm used in test_upgrade_charm_switch_channel "
+                           "seems to have been updated, the test needs to be revised")
+
+        await app.upgrade_charm(channel='2.0/edge')
+        await model.wait_for_idle(status='active')
+
+        if still22:
+            try:
+                charm_url = URL.parse(app.data['charm-url'])
+                assert charm_url.revision == 23
+            except AssertionError:
+                raise errors.JujuError("Either the upgrade has failed, or the used charm moved "
+                                       "the candidate channel to stable, so no upgrade took place, "
+                                       "the test needs to be revised.")
+
+        # Try with another charm too, just in case, no need to check revisions etc
+        app = await model.deploy('ubuntu', channel='stable')
+        await model.wait_for_idle(status='active')
+        await app.upgrade_charm(channel='candidate')
+        await model.wait_for_idle(status='active')
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_upgrade_charm_revision(event_loop):
@@ -187,7 +235,9 @@ async def test_upgrade_charm_switch(event_loop):
         await model.block_until(lambda: (len(app.units) > 0 and
                                          app.units[0].machine))
         assert app.data['charm-url'] == 'cs:ubuntu-0'
-        await app.upgrade_charm(switch='ubuntu-8')
+        with pytest.raises(errors.JujuError):
+            await app.upgrade_charm(switch='ubuntu-8')
+        await app.upgrade_charm(switch='cs:ubuntu-8')
         assert app.data['charm-url'] == 'cs:ubuntu-8'
 
 
@@ -205,6 +255,18 @@ async def test_upgrade_local_charm(event_loop):
         assert app.data['charm-url'] == 'local:focal/ubuntu-0'
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_upgrade_switch_charmstore_to_charmhub(event_loop):
+    async with base.CleanModel() as model:
+        app = await model.deploy('cs:ubuntu', series='focal')
+        await model.wait_for_idle(status="active")
+        assert app.data['charm-url'].startswith('cs:ubuntu')
+        await app.upgrade_charm(channel='latest/stable', switch='ch:ubuntu-8')
+        await model.wait_for_idle(status="active")
+        assert app.data['charm-url'].startswith('ch:')
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_upgrade_charm_resource(event_loop):

tests/integration/test_charmhub.py

@@ -101,3 +101,11 @@ async def test_subordinate_charm_zero_units(event_loop):
         app2 = await model.deploy('rsyslog-forwarder-ha', num_units=1)
         await jasyncio.sleep(5)
         assert len(app2.units) == 0
+
+
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_list_resources(event_loop):
+    async with base.CleanModel() as model:
+        resources = await model.charmhub.list_resources('postgresql')
+        assert type(resources) == list and len(resources) > 0