Merge branch 'master' into wait-for-at-least-units-param-rename

Author: cderici

juju/application.py

@@ -749,11 +749,10 @@ async def refresh(
         }
 
         # Compute the difference btw resources needed and the existing resources
-        resources_to_update = [
-            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'
-        ]
+        resources_to_update = []
+        for resource in charm_resources:
+            if utils.should_upgrade_resource(resource, existing_resources):
+                resources_to_update.append(resource)
 
         # Update the resources
         if resources_to_update:

juju/utils.py

@@ -523,3 +523,30 @@ def series_selector(series_arg='', charm_url=None, model_config=None, supported_
     # Charm hasn't specified a default (likely due to being a local charm
     # deployed by path). Last chance, best we can do is default to LTS.
     return DEFAULT_SUPPORTED_LTS
+
+
+def should_upgrade_resource(available_resource, existing_resources):
+    """Called in the context of upgrade_charm. Given a resource R, takes a look at the resources we
+    already have and decides if we need to refresh R.
+
+    :param dict[str] available_resource: The dict representing the client.Resource coming from the
+    charmhub api. We're considering if we need to refresh this during upgrade_charm.
+    :param dict[str] existing_resources: The dict coming from resources_facade.ListResources
+    representing the resources of the currently deployed charm.
+
+    :result bool: The decision to refresh the given resource
+    """
+    # should upgrade resource?
+    res_name = available_resource.get('Name', available_resource.get('name'))
+    # no, if it's upload
+    if existing_resources[res_name].origin == 'upload':
+        return False
+
+    # no, if we already have it (and upstream doesn't have a newer res available)
+    if res_name in existing_resources:
+        available_rev = available_resource.get('Revision', available_resource.get('revision', -1))
+        u_fields = existing_resources[res_name].unknown_fields
+        existing_rev = u_fields.get('Revision', u_fields.get('revision', -1))
+        if existing_rev >= available_rev:
+            return False
+    return True

tests/integration/test_application.py

@@ -245,6 +245,17 @@ async def test_upgrade_local_charm_resource(event_loop):
         assert ress['file-res']
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_upgrade_charm_resource_same_rev_no_update(event_loop):
+    async with base.CleanModel() as model:
+        app = await model.deploy('keystone', channel='victoria/stable')
+        ress = await app.get_resources()
+        await app.refresh(channel='ussuri/stable')
+        ress2 = await app.get_resources()
+        assert ress['policyd-override'].fingerprint == ress2['policyd-override'].fingerprint
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_trusted(event_loop):

tests/unit/test_utils.py

@@ -2,22 +2,22 @@
 import pytest
 
 from juju.utils import series_selector, get_base_from_origin_or_channel, \
-    parse_base_arg, juju_config_dir, juju_ssh_key_paths, \
-    DEFAULT_SUPPORTED_LTS, get_series_version, get_version_series, \
-    base_channel_to_series, base_channel_from_series, \
-    get_os_from_series
-from juju.client import client
+    parse_base_arg, DEFAULT_SUPPORTED_LTS, get_series_version, \
+    get_version_series, base_channel_to_series, \
+    base_channel_from_series, get_os_from_series
 from juju.errors import JujuError
 from juju.url import URL
+from juju import utils
+from juju.client import client
 
 
 class TestDirResolve(unittest.TestCase):
     def test_config_dir(self):
-        config_dir = juju_config_dir()
+        config_dir = utils.juju_config_dir()
         assert 'local/share/juju' in config_dir
 
     def test_juju_ssh_key_paths(self):
-        public, private = juju_ssh_key_paths()
+        public, private = utils.juju_ssh_key_paths()
         assert public.endswith('ssh/juju_id_rsa.pub')
         assert private.endswith('ssh/juju_id_rsa')
 
@@ -73,3 +73,73 @@ def test_get_os_from_series(self):
     def test_get_base_from_series(self):
         orgn = client.CharmOrigin(track='latest', risk='edge')
         assert get_base_from_origin_or_channel(orgn, series='jammy') == client.Base('22.04/edge', 'ubuntu')
+
+
+class TestShouldUpgradeResource(unittest.TestCase):
+    def test_should_upgrade_resource_no_same_rev(self):
+        # fields are trimmed for readability
+        res = {'created-at': '2019-10-24T20:45:19.201000',
+               'description': 'The policy.d overrides file',
+               'download': {'hash-sha-256': 'e3b0c4', 'hash-sha-384': '38b060a751ac914898b95b',
+                            'hash-sha-512': 'cf83e1357eef1a538327af927da3e',
+                            'hash-sha3-384': '0c63a75b1bbed1e058d5f004',
+                            'size': 0,
+                            'url': 'https://api.charmhub.io/api/v1/resMGU0L516cGTTwNam.policyd-override_0'},
+               'filename': 'policyd-override.zip', 'name': 'policyd-override', 'revision': 0, 'type': 'file'}
+
+        existing = {
+            'policyd-override':
+                client.Resource(charmresource=None,
+                                application='keystone', id_='keystone/policyd-override', pending_id='',
+                                timestamp='0001-01-01T00:00:00Z', username='', name='policyd-override',
+                                origin='store', type='file', path='policyd-override.zip',
+                                description='The policy.doverrides file',
+                                revision=0, fingerprint='OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb',
+                                size=0)}
+        assert not utils.should_upgrade_resource(res, existing)
+
+    def test_should_upgrade_resource_no_local_upload(self):
+        # fields are trimmed for readability
+        res = {'created-at': '2019-10-24T20:45:19.201000',
+               'description': 'The policy.d overrides file',
+               'download': {'hash-sha-256': 'e3b0c4', 'hash-sha-384': '38b060a751ac914898b95b',
+                            'hash-sha-512': 'cf83e1357eef1a538327af927da3e',
+                            'hash-sha3-384': '0c63a75b1bbed1e058d5f004',
+                            'size': 0,
+                            'url': 'https://api.charmhub.io/api/v1/resMGU0L516cGTTwNam.policyd-override_0'},
+               'filename': 'policyd-override.zip', 'name': 'local_res', 'revision': 0,
+               'type': 'file'}
+
+        existing = {
+            'local_res':
+                client.Resource(charmresource=None,
+                                application='keystone', id_='keystone/policyd-override', pending_id='',
+                                timestamp='0001-01-01T00:00:00Z', username='', name='policyd-override',
+                                origin='upload', type='file', path='policyd-override.zip',
+                                description='The policy.doverrides file',
+                                revision=0, fingerprint='OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb',
+                                size=0)}
+        assert not utils.should_upgrade_resource(res, existing)
+
+    def test_should_upgrade_resource_yes_new_revision(self):
+        # fields are trimmed for readability
+        res = {'created-at': '2019-10-24T20:45:19.201000',
+               'description': 'The policy.d overrides file',
+               'download': {'hash-sha-256': 'e3b0c4', 'hash-sha-384': '38b060a751ac914898b95b',
+                            'hash-sha-512': 'cf83e1357eef1a538327af927da3e',
+                            'hash-sha3-384': '0c63a75b1bbed1e058d5f004',
+                            'size': 0,
+                            'url': 'https://api.charmhub.io/api/v1/resMGU0L516cGTTwNam.policyd-override_0'},
+               'filename': 'policyd-override.zip', 'name': 'policyd-override', 'revision': 1,
+               'type': 'file'}
+
+        existing = {
+            'policyd-override':
+                client.Resource(charmresource=None,
+                                application='keystone', id_='keystone/policyd-override', pending_id='',
+                                timestamp='0001-01-01T00:00:00Z', username='', name='policyd-override',
+                                origin='store', type='file', path='policyd-override.zip',
+                                description='The policy.doverrides file',
+                                revision=0, fingerprint='OLBgp1GsljhM2TJ+sbHjaiH9txEUvgdDTAzHv2P24donTt6/529l+9Ua0vFImLlb',
+                                size=0)}
+        assert utils.should_upgrade_resource(res, existing)