Merge pull request #884 from cderici/no-upgr-resource-if-same-revision

#884

#### Description

This adds a condition to the logic of deciding if we need to upgrade a resource during an `app.refresh()`, analogous to the [`shouldUpgradeResource()` utility](https://github.com/juju/juju/blob/cdda0d771219c9e59bfef4579da8b6389e1b0d72/cmd/juju/application/utils/utils.go#L159) in Juju.

Fixes #883 

#### QA Steps

This adds the use case in #883 as an integration test, so the following should pass:

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

Also per #884 (comment), this also splits out the resource refresh decider logic into a separate function and adds unit tests for it, so the following should also pass:

```sh
 tox -e py3 -- tests/unit/test_utils.py::TestShouldUpgradeResource
```

Author: jujubot

juju/application.py

@@ -740,11 +740,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

@@ -389,3 +389,30 @@ def base_channel_to_series(channel):
     :return: str series (e.g. focal)
     """
     return get_version_series(origin.Channel.parse(channel).track)
+
+
+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

@@ -226,6 +226,17 @@ async def test_upgrade_charm_resource(event_loop):
         assert app.units[0].workload_status_message == expected_message
 
 
+@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

@@ -1,14 +1,85 @@
 import unittest
 
-from juju.utils import juju_config_dir, juju_ssh_key_paths
+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')
+
+
+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)