Merge pull request #894 from cderici/pass-through-local-refresh-if-resources

#894

#### Description

This patches a bug that 5d843ec introduced, where if `--path` is given (i.e., refreshing with a local charm), it errors with NYI if `--resources` is given (instead of falling through the `local_refresh`).

Should fix #881

#### QA Steps

Adds an integration test, so the following should pass:

```sh
tox -e integration -- tests/integration/test_application.py::test_upgrade_local_charm_with_resource
```

Author: jujubot

juju/application.py

@@ -623,7 +623,7 @@ async def set_constraints(self, constraints):
 
     async def refresh(
             self, channel=None, force=False, force_series=False, force_units=False,
-            path=None, resources=None, revision=None, switch=None):
+            local_charm_path=None, resources=None, revision=None, switch=None):
         """Refresh the charm for this application.
 
         :param str channel: Channel to use when getting the charm from the
@@ -632,17 +632,17 @@ async def refresh(
             application is not supported by the new charm
         :param bool force_units: Refresh all units immediately, even if in
             error state
-        :param str path: Refresh to a charm located at path
+        :param str local_charm_path: Refresh to a charm located at path
         :param dict resources: Dictionary of resource name/filepath pairs
         :param int revision: Explicit refresh revision
         :param str switch: Crossgrade charm url
 
         """
-        if resources is not None:
-            raise NotImplementedError("resources option is not implemented")
+        if local_charm_path is None and resources is not None:
+            raise NotImplementedError("refreshing a non-local charm with resources option is not yet implemented")
 
         if switch is not None and revision is not None:
-            raise ValueError("switch and revision are mutually exclusive")
+            raise ValueError("switch and revision parameters are mutually exclusive in application refresh")
 
         app_facade = self._facade()
         resources_facade = client.ResourcesFacade.from_connection(self.connection)
@@ -660,9 +660,9 @@ async def refresh(
         charm_url = switch or charm_url_origin_result.url
         origin = charm_url_origin_result.charm_origin
 
-        if path is not None:
+        if local_charm_path is not None:
             await self.local_refresh(origin, force, force_series,
-                                     force_units, path, resources)
+                                     force_units, local_charm_path, resources)
             return
 
         parsed_url = URL.parse(charm_url)

tests/integration/test_application.py

@@ -4,6 +4,7 @@
 import logging
 
 from .. import base
+from ..utils import INTEGRATION_TEST_DIR
 from juju import errors
 from juju.url import URL, Schema
 
@@ -237,6 +238,28 @@ async def test_upgrade_charm_resource_same_rev_no_update(event_loop):
         assert ress['policyd-override'].fingerprint == ress2['policyd-override'].fingerprint
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_upgrade_local_charm_with_resource(event_loop):
+    charm_path = INTEGRATION_TEST_DIR / 'file-resource-charm'
+    async with base.CleanModel() as model:
+        app = await model.deploy(str(charm_path))
+        assert 'file-resource-charm' in model.applications
+
+        await model.wait_for_idle()
+        assert app.units[0].agent_status == 'idle'
+
+        resources = {"file-res": "test.file"}
+        await app.refresh(local_charm_path=str(charm_path), resources=resources)
+
+        await model.wait_for_idle()
+        assert app.units[0].agent_status == 'idle'
+
+        ress = await app.get_resources()
+        assert 'file-res' in ress
+        assert ress['file-res']
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_trusted(event_loop):

tests/integration/test_model.py

@@ -5,7 +5,6 @@
 import time
 import uuid
 from concurrent.futures import ThreadPoolExecutor
-from pathlib import Path
 
 import mock
 import paramiko
@@ -19,13 +18,7 @@
 from juju.utils import block_until, run_with_interrupt, wait_for_bundle
 
 from .. import base
-
-MB = 1
-GB = 1024
-SSH_KEY = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCsYMJGNGG74HAJha3n2CFmWYsOOaORnJK6VqNy86pj0MIpvRXBzFzVy09uPQ66GOQhTEoJHEqE77VMui7+62AcMXT+GG7cFHcnU8XVQsGM6UirCcNyWNysfiEMoAdZScJf/GvoY87tMEszhZIUV37z8PUBx6twIqMdr31W1J0IaPa+sV6FEDadeLaNTvancDcHK1zuKsL39jzAg7+LYjKJfEfrsQP+lj/EQcjtKqlhVS5kzsJVfx8ZEd0xhW5G7N6bCdKNalS8mKCMaBXJpijNQ82AiyqCIDCRrre2To0/i7pTjRiL0U9f9mV3S4NJaQaokR050w/ZLySFf6F7joJT mathijs@Qrama-Mathijs'  # noqa
-HERE_DIR = Path(__file__).absolute().parent  # tests/integration
-TESTS_DIR = HERE_DIR.parent  # tests/
-OVERLAYS_DIR = HERE_DIR / 'bundle' / 'test-overlays'
+from ..utils import MB, GB, TESTS_DIR, OVERLAYS_DIR, SSH_KEY, INTEGRATION_TEST_DIR
 
 
 @base.bootstrapped
@@ -68,7 +61,7 @@ async def test_deploy_local_bundle_file(event_loop):
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_deploy_bundle_local_resource_relative_path(event_loop):
-    bundle_file_path = HERE_DIR / 'bundle-file-resource.yaml'
+    bundle_file_path = INTEGRATION_TEST_DIR / 'bundle-file-resource.yaml'
 
     async with base.CleanModel() as model:
         await model.deploy(str(bundle_file_path))
@@ -82,7 +75,7 @@ async def test_deploy_bundle_local_resource_relative_path(event_loop):
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_deploy_local_bundle_include_file(event_loop):
-    bundle_dir = TESTS_DIR / 'integration' / 'bundle'
+    bundle_dir = INTEGRATION_TEST_DIR / 'bundle'
     bundle_yaml_path = bundle_dir / 'bundle-include-file.yaml'
 
     async with base.CleanModel() as model:
@@ -99,7 +92,7 @@ async def test_deploy_local_bundle_include_file(event_loop):
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_deploy_local_bundle_include_base64(event_loop):
-    bundle_dir = TESTS_DIR / 'integration' / 'bundle'
+    bundle_dir = INTEGRATION_TEST_DIR / 'bundle'
     bundle_yaml_path = bundle_dir / 'bundle-include-base64.yaml'
 
     async with base.CleanModel() as model:
@@ -115,7 +108,7 @@ async def test_deploy_local_bundle_include_base64(event_loop):
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_deploy_bundle_local_charms(event_loop):
-    bundle_path = TESTS_DIR / 'integration' / 'bundle' / 'local.yaml'
+    bundle_path = INTEGRATION_TEST_DIR / 'bundle' / 'local.yaml'
 
     async with base.CleanModel() as model:
         await model.deploy(bundle_path)
@@ -678,7 +671,7 @@ async def test_local_oci_image_resource_charm(event_loop):
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_local_file_resource_charm(event_loop):
-    charm_path = TESTS_DIR / 'integration' / 'file-resource-charm'
+    charm_path = INTEGRATION_TEST_DIR / 'file-resource-charm'
     async with base.CleanModel() as model:
         resources = {"file-res": "test.file"}
         app = await model.deploy(str(charm_path), resources=resources)
@@ -716,7 +709,7 @@ async def test_attach_resource(event_loop):
 async def test_store_resources_bundle(event_loop):
     pytest.skip('test_store_resources_bundle intermittent test failure')
     async with base.CleanModel() as model:
-        bundle = str(Path(__file__).parent / 'bundle')
+        bundle = INTEGRATION_TEST_DIR / 'bundle'
         await model.deploy(bundle)
         assert 'ghost' in model.applications
         ghost = model.applications['ghost']
@@ -738,7 +731,7 @@ async def test_store_resources_bundle(event_loop):
 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:
-        bundle = str(Path(__file__).parent / 'bundle/bundle-resource-rev.yaml')
+        bundle = INTEGRATION_TEST_DIR / 'bundle/bundle-resource-rev.yaml'
         await model.deploy(bundle)
         assert 'ghost' in model.applications
         ghost = model.applications['ghost']

tests/utils.py

@@ -0,0 +1,12 @@
+from pathlib import Path
+
+# Utilities for tests
+
+MB = 1
+GB = 1024
+SSH_KEY = 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCsYMJGNGG74HAJha3n2CFmWYsOOaORnJK6VqNy86pj0MIpvRXBzFzVy09uPQ66GOQhTEoJHEqE77VMui7+62AcMXT+GG7cFHcnU8XVQsGM6UirCcNyWNysfiEMoAdZScJf/GvoY87tMEszhZIUV37z8PUBx6twIqMdr31W1J0IaPa+sV6FEDadeLaNTvancDcHK1zuKsL39jzAg7+LYjKJfEfrsQP+lj/EQcjtKqlhVS5kzsJVfx8ZEd0xhW5G7N6bCdKNalS8mKCMaBXJpijNQ82AiyqCIDCRrre2To0/i7pTjRiL0U9f9mV3S4NJaQaokR050w/ZLySFf6F7joJT mathijs@Qrama-Mathijs'  # noqa
+HERE_DIR = Path(__file__).absolute()  # tests/integration
+TESTS_DIR = HERE_DIR.parent  # tests/
+INTEGRATION_TEST_DIR = TESTS_DIR / 'integration'
+UNIT_TEST_DIR = TESTS_DIR / 'unit'
+OVERLAYS_DIR = INTEGRATION_TEST_DIR / 'bundle' / 'test-overlays'