Repository: juju/python-libjuju
Pull request #969 (2023-10-23 12:48:18)

Parse charm URLs consistantly for local charms
Parse charm URLs consistantly for local charms

Local charm URLs are confused in pylibjuju because often a charm
url-like string is passed into deploy, to explicitly specify a local
charm. These 'urls' were of the form 'local:/path/to/charm'

Local URLs were parsed accordingly.

However, the above is in no sense a url really so should be treated as
such. This also means that pylibjuju is unable to parse real local charm
urls returned to the client from the server when, say, uploading a local
charm.

Drop support for local charms like this. Instead parse local charm urls the
same way we parse charmhub charm url.

This has the side-effect, however, that we can no longer treat all
objects to deploy as URLs. Now, they are either URLs (ch or cs charms)
or paths or paths prepended with 'local:' (local charms)

As such, in deploy code and refresh code, we often need to branch on
is_local_charm. This is to ensure we don't attempt to parse paths as
URLs. Everywhere we URL.parse user input, we ensure the entity is not a
path

This resolves https://github.com/juju/python-libjuju/issues/961

#### QA Steps

```
tox -e py3 -- tests/unit/test_url.py
```

```
tox -e integration -- tests/integration/test_model.py::test_deploy_local_bundle_dir
tox -e integration -- tests/integration/test_model.py::test_deploy_local_bundle_file
tox -e integration -- tests/integration/test_model.py::test_deploy_bundle_local_charms
tox -e integration -- tests/integration/test_model.py::test_deploy_local_charm
```

All CI tests need to pass.

(In a python interpreter)
```
>>> from juju.url import URL
>>> u = URL.parse("local:focal/ubuntu-12")
>>> print(u.name)
ubuntu
>>> print(u.revision)
12
>>> URL.parse("local:/path/to/charm")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jack/juju/python-libjuju/juju/url.py", line 50, in parse
    c = parse_v2_url(u, s, default_store)
  File "/home/jack/juju/python-libjuju/juju/url.py", line 138, in parse_v2_url
    raise JujuError("charm or bundle URL {} malformed".format(s))
juju.errors.JujuError: charm or bundle URL local:/path/to/charm malformed
```

Ensure the example scripts still function
```
$ python examples/deploy_bundle.py
$ python examples/deploy_local_bundle_with_resources.py
```

Verify the following example script runs successfully:
```
from juju import jasyncio
from juju.model import Model


async def main():
    model = Model()
    print('Connecting to model')
    await model.connect()
    try:
        print('path="/home/jack/charms/ubuntu"')
        await depl(model, path="/home/jack/charms/ubuntu")
        print('switch="/home/jack/charms/ubuntu"')
        await depl(model, switch="/home/jack/charms/ubuntu")
        print('switch="local:/home/jack/charms/ubuntu"')
        await depl(model, switch="local:/home/jack/charms/ubuntu")
    finally:
        print('Disconnecting from model')
        await model.disconnect()

async def depl(model, **kwargs):
    try:
        app = await model.deploy("ubuntu")
        await model.block_until(lambda: all(u[0].workload_status == 'active' for u in app.units))
        await app.refresh(**kwargs)
    finally:
        await app.remove()
        await model.block_until(lambda: not len(model.applications))

if __name__ == '__main__':
    jasyncio.run(main())
```




Patch:
diff --git a/juju/bundle.py b/juju/bundle.py
index 6637af7..58e8db1 100644
--- a/juju/bundle.py
+++ b/juju/bundle.py
@@ -223,24 +223,28 @@ class BundleHandler:
 
         return self.bundle, self.overlays
 
-    async def fetch_plan(self, charm_url, origin, overlays=[]):
-        entity_id = charm_url.path()
-        is_local = Schema.LOCAL.matches(charm_url.schema)
+    async def fetch_plan(self, bundle, origin, overlays=[]):
         bundle_dir = None
 
-        if is_local and os.path.isfile(entity_id):
-            bundle_yaml = Path(entity_id).read_text()
-            bundle_dir = Path(entity_id).parent
-        elif is_local and os.path.isdir(entity_id):
-            bundle_yaml = (Path(entity_id) / "bundle.yaml").read_text()
-            bundle_dir = Path(entity_id)
+        if is_local_bundle(str(bundle)):
+            path = str(bundle)
+            if path.startswith("local:"):
+                path = path[6:]
+            bundle_yaml, bundle_dir = read_local_bundle(path)
 
-        if Schema.CHARM_STORE.matches(charm_url.schema):
-            bundle_yaml = await self.charmstore.files(entity_id,
-                                                      filename='bundle.yaml',
-                                                      read_file=True)
-        elif Schema.CHARM_HUB.matches(charm_url.schema):
-            bundle_yaml = await self._download_bundle(charm_url, origin)
+        else:
+            if client.CharmsFacade.best_facade_version(self.model.connection()) < 3:
+                url = URL.parse(bundle, default_store=Schema.CHARM_STORE)
+            else:
+                url = URL.parse(bundle)
+            path = url.path()
+
+            if Schema.CHARM_STORE.matches(url.schema):
+                bundle_yaml = await self.charmstore.files(bundle,
+                                                          filename='bundle.yaml',
+                                                          read_file=True)
+            elif Schema.CHARM_HUB.matches(url.schema):
+                bundle_yaml = await self._download_bundle(bundle, origin)
 
         if not bundle_yaml:
             raise JujuError('empty bundle, nothing to deploy')
@@ -267,7 +271,7 @@ class BundleHandler:
 
         self.bundle = await self._validate_bundle(self.bundle)
 
-        if is_local:
+        if is_local_bundle(path):
             self.bundle = await self._handle_local_charms(self.bundle, bundle_dir)
 
         self.bundle, self.overlays = self._resolve_include_file_config(bundle_dir)
@@ -278,7 +282,7 @@ class BundleHandler:
         yaml_data = "---\n".join(_yaml_data)
 
         self.plan = await self.bundle_facade.GetChanges(
-            bundleurl=entity_id,
+            bundleurl=path,
             yaml=yaml_data)
 
         if self.plan.errors:
@@ -362,7 +366,7 @@ class BundleHandler:
                                             architecture=architecture,
                                             risk=risk,
                                             track=track)
-                charm_url, charm_origin = await self.model._resolve_charm(charm_url, origin)
+                charm_url, charm_origin = await self.model._charmhub_resolve_charm(charm_url, origin)
 
                 spec['charm'] = str(charm_url)
             else:
@@ -416,6 +420,21 @@ def is_local_charm(charm_url):
     return charm_url.startswith('.') or charm_url.startswith('local:') or os.path.isabs(charm_url)
 
 
+is_local_bundle = is_local_charm
+
+
+def read_local_bundle(path):
+    path = Path(path)
+    if os.path.isfile(path):
+        bundle_yaml = path.read_text()
+        bundle_dir = path.parent
+    elif os.path.isdir(path):
+        bundle_yaml = (path / "bundle.yaml").read_text()
+        bundle_dir = path
+
+    return (bundle_yaml, bundle_dir)
+
+
 async def get_charm_series(metadata, model):
     """Inspects the given metadata and returns a default series from its
     metadata.yaml (the first item in the 'series' list).
@@ -709,14 +728,14 @@ class AddCharmChange(ChangeInfo):
 
         # We don't add local charms because they've already been added
         # by self._handle_local_charms
-        url = URL.parse(str(self.charm))
-        ch = None
-        identifier = None
-        if Schema.LOCAL.matches(url.schema):
+        if is_local_charm(str(self.charm)):
             origin = client.CharmOrigin(source="local", risk="stable")
             context.origins[self.charm] = {str(None): origin}
             return self.charm
 
+        url = URL.parse(str(self.charm))
+        ch = None
+        identifier = None
         if Schema.CHARM_STORE.matches(url.schema):
             entity_id = await context.charmstore.entityId(self.charm, channel=self.channel)
             log.debug('Adding %s', entity_id)
@@ -735,7 +754,7 @@ class AddCharmChange(ChangeInfo):
                                         architecture=arch,
                                         risk=ch.risk,
                                         track=ch.track)
-            identifier, origin = await context.model._resolve_charm(url, origin)
+            identifier, origin = await context.model._charmhub_resolve_charm(url, origin)
 
         if identifier is None:
             raise JujuError('unknown charm {}'.format(self.charm))
diff --git a/juju/model.py b/juju/model.py
index 145827f..099606e 100644
--- a/juju/model.py
+++ b/juju/model.py
@@ -452,43 +452,34 @@ class LocalDeployType:
     """LocalDeployType deals with local only deployments.
     """
 
-    async def resolve(self, url, architecture, app_name=None, channel=None, series=None, revision=None,
+    async def resolve(self, charm_path, architecture, app_name=None, channel=None, series=None, revision=None,
                       entity_url=None, force=False):
         """resolve attempts to resolve a local charm or bundle using the url
         and architecture. If information is missing, it will attempt to backfill
         that information, before sending the result back.
         """
 
-        entity_url = url.path()
-        entity_path = Path(entity_url)
-        bundle_path = entity_path / 'bundle.yaml'
+        entity_path = Path(charm_path)
+        if entity_path.suffix == '.yaml':
+            bundle_path = entity_path
+        else:
+            bundle_path = entity_path / 'bundle.yaml'
 
-        identifier = entity_url
         origin = client.CharmOrigin(source="local", architecture=architecture)
         if not (entity_path.is_dir() or entity_path.is_file()):
             raise JujuError('{} path not found'.format(entity_url))
 
-        is_bundle = (
-            (entity_path.suffix == ".yaml" and entity_path.exists()) or
-            bundle_path.exists()
-        )
+        is_bundle = bundle_path.exists()
 
         if app_name is None:
-            app_name = url.name
-
-            if not is_bundle:
-                entity_url = url.path()
-                entity_path = Path(entity_url)
-                if entity_path.suffix == '.charm':
-                    with zipfile.ZipFile(str(entity_path), 'r') as charm_file:
-                        metadata = yaml.load(charm_file.read('metadata.yaml'), Loader=yaml.FullLoader)
-                else:
-                    metadata_path = entity_path / 'metadata.yaml'
-                    metadata = yaml.load(metadata_path.read_text(), Loader=yaml.FullLoader)
-                app_name = metadata['name']
+            if is_bundle:
+                bundle_with_overlays = [b for b in yaml.safe_load_all(bundle_path.read_text())]
+                app_name = bundle_with_overlays[0].get('name', '')
+            else:
+                app_name = utils.get_local_charm_metadata(entity_path)["name"]
 
         return DeployTypeResult(
-            identifier=identifier,
+            identifier=charm_path,
             origin=origin,
             app_name=app_name,
             is_local=True,
@@ -521,7 +512,7 @@ class CharmStoreDeployType:
                                               include_stats=False)
 
         identifier = result['Id']
-        is_bundle = url.series == "bundle" or url.parse(identifier).series == "bundle"
+        is_bundle = URL.parse(url).series == "bundle" or URL.parse(identifier).series == "bundle"
         if not series:
             series = "bundle" if is_bundle else self.get_series(entity_url, result)
 
@@ -578,7 +569,7 @@ class CharmhubDeployType:
         charm_url = URL.parse(charm_url_str)
 
         if app_name is None:
-            app_name = url.name
+            app_name = charm_url.name
 
         return DeployTypeResult(
             identifier=str(charm_url),
@@ -629,9 +620,9 @@ class Model:
         self._charmstore = CharmStore()
 
         self.deploy_types = {
-            "local": LocalDeployType(),
-            "cs": CharmStoreDeployType(self._charmstore, self._get_series),
-            "ch": CharmhubDeployType(self._resolve_charm),
+            Schema.LOCAL: LocalDeployType(),
+            Schema.CHARM_STORE: CharmStoreDeployType(self._charmstore, self._get_series),
+            Schema.CHARM_HUB: CharmhubDeployType(self._charmhub_resolve_charm),
         }
 
     def is_connected(self):
@@ -1603,7 +1594,7 @@ class Model:
             storage=None, to=None, devices=None, trust=False):
         """Deploy a new service or bundle.
 
-        :param str entity_url: Charm or bundle url
+        :param str entity_url: Charm or bundle to deploy. Charm url or file path
         :param str application_name: Name to give the service
         :param dict bind: <charm endpoint>:<network space> pairs
         :param str channel: Charm store channel from which to retrieve
@@ -1646,23 +1637,30 @@ class Model:
             raise NotImplementedError("trusted is not supported on model version {}".format(self.info.agent_version))
 
         # Ensure what we pass in, is a string.
-        entity_url = str(entity_url)
-        if is_local_charm(entity_url) and not entity_url.startswith("local:"):
-            entity_url = "local:{}".format(entity_url)
+        entity = str(entity_url)
+        if is_local_charm(entity):
+            if entity.startswith("local:"):
+                entity = entity[6:]
+            architecture = await self._resolve_architecture()
+            schema = Schema.LOCAL
 
-        if client.CharmsFacade.best_facade_version(self.connection()) < 3:
-            url = URL.parse(str(entity_url), default_store=Schema.CHARM_STORE)
         else:
-            url = URL.parse(str(entity_url))
+            if client.CharmsFacade.best_facade_version(self.connection()) < 3:
+                url = URL.parse(entity, default_store=Schema.CHARM_STORE)
+            else:
+                url = URL.parse(entity)
+            entity = str(url)
 
-        architecture = await self._resolve_architecture(url)
+            architecture = await self._resolve_architecture(url)
+            schema = url.schema
+            name = url.name
 
-        if str(url.schema) not in self.deploy_types:
-            raise JujuError("unknown deploy type {}, expected charmhub, charmstore or local".format(url.schema))
+        if schema not in self.deploy_types:
+            raise JujuError("unknown deploy type {}, expected charmhub, charmstore or local".format(schema))
 
-        res = await self.deploy_types[str(url.schema)].resolve(url, architecture,
-                                                               application_name, channel, series,
-                                                               revision, entity_url, force)
+        res = await self.deploy_types[schema].resolve(entity, architecture,
+                                                      application_name, channel, series,
+                                                      revision, entity_url, force)
 
         if res.identifier is None:
             raise JujuError('unknown charm or bundle {}'.format(entity_url))
@@ -1672,7 +1670,7 @@ class Model:
         charm_series = series
         if res.is_bundle:
             handler = BundleHandler(self, trusted=trust, forced=force)
-            await handler.fetch_plan(url, res.origin, overlays=overlays)
+            await handler.fetch_plan(entity, res.origin, overlays=overlays)
             await handler.execute_plan()
             extant_apps = {app for app in self.applications}
             pending_apps = handler.applications - extant_apps
@@ -1700,11 +1698,11 @@ class Model:
                 else:
                     charm_origin = add_charm_res.charm_origin
 
-                if Schema.CHARM_HUB.matches(url.schema):
+                if Schema.CHARM_HUB.matches(schema):
                     resources = await self._add_charmhub_resources(res.app_name,
                                                                    identifier,
                                                                    add_charm_res.charm_origin)
-                    charm_info = await self.charmhub.info(url.name)
+                    charm_info = await self.charmhub.info(name)
                     is_subordinate = False
                     try:
                         is_subordinate = charm_info.result.charm.subordinate
@@ -1715,7 +1713,7 @@ class Model:
                             raise JujuError("cannot use num_units with subordinate application")
                         num_units = 0
 
-                if Schema.CHARM_STORE.matches(url.schema):
+                if Schema.CHARM_STORE.matches(schema):
                     resources = await self._add_store_resources(res.app_name,
                                                                 identifier)
             else:
@@ -1777,23 +1775,18 @@ class Model:
         client_facade = client.ClientFacade.from_connection(self.connection())
         return await client_facade.AddCharm(channel=str(origin.risk), url=charm_url, force=False)
 
-    async def _resolve_charm(self, url, origin, force=False):
+    async def _charmhub_resolve_charm(self, url, origin, force=False):
         charms_cls = client.CharmsFacade
         if charms_cls.best_facade_version(self.connection()) < 3:
             raise JujuError("resolve charm")
 
         charms_facade = charms_cls.from_connection(self.connection())
 
-        if Schema.CHARM_STORE.matches(url.schema):
-            source = "charm-store"
-        else:
-            source = "charm-hub"
-
         resp = await charms_facade.ResolveCharms(resolve=[{
             'reference': str(url),
             'charm-origin': {
                 'series': origin.series,
-                'source': source,
+                'source': "charm-hub",
                 'architecture': origin.architecture,
                 'track': origin.track,
                 'risk': origin.risk,
@@ -1820,8 +1813,8 @@ class Model:
 
         return result.url, result.charm_origin
 
-    async def _resolve_architecture(self, url):
-        if url.architecture:
+    async def _resolve_architecture(self, url=None):
+        if url is not None and url.architecture:
             return url.architecture
 
         constraints = await self.get_constraints()
diff --git a/juju/url.py b/juju/url.py
index b95b18d..a00798d 100644
--- a/juju/url.py
+++ b/juju/url.py
@@ -43,13 +43,11 @@ class URL:
         if u.query != "" or u.fragment != "" or u.username or u.password:
             raise JujuError("charm or bundle URL {} has unrecognized parts".format(u))
 
-        if Schema.LOCAL.matches(u.scheme):
-            c = URL(Schema.LOCAL, name=u.path)
-        elif Schema.CHARM_STORE.matches(u.scheme) or \
+        if Schema.CHARM_STORE.matches(u.scheme) or \
                 (u.scheme == "" and Schema.CHARM_STORE.matches(default_store)):
             c = parse_v1_url(Schema.CHARM_STORE, u, s)
         else:
-            c = parse_v2_url(u, s)
+            c = parse_v2_url(u, s, default_store)
 
         if not c or not c.schema:
             raise JujuError("expected schema for charm or bundle URL {}".format(u))
@@ -121,8 +119,15 @@ def parse_v1_url(schema, u, s):
     return c
 
 
-def parse_v2_url(u, s):
-    c = URL(Schema.CHARM_HUB)
+def parse_v2_url(u, s, default_store):
+    if not u.scheme:
+        c = URL(default_store)
+    elif Schema.CHARM_HUB.matches(u.scheme):
+        c = URL(Schema.CHARM_HUB)
+    elif Schema.LOCAL.matches(u.scheme):
+        c = URL(Schema.LOCAL)
+    else:
+        raise JujuError("invalid charm url schema {}".format(u.scheme))
 
     parts = u.path.split("/")
     num = len(parts)


Test patch:
diff --git a/tests/unit/test_url.py b/tests/unit/test_url.py
index 05bfebb..cd872f6 100644
--- a/tests/unit/test_url.py
+++ b/tests/unit/test_url.py
@@ -11,10 +11,6 @@ class TestURLV1(unittest.TestCase):
         u = URL.parse("cs:mysql")
         self.assertEqual(u, URL(Schema.CHARM_STORE, name="mysql"))
 
-    def test_parse_local(self):
-        u = URL.parse("local:mysql")
-        self.assertEqual(u, URL(Schema.LOCAL, name="mysql"))
-
     def test_parse_v1_user(self):
         u = URL.parse("cs:~fred/mysql")
         self.assertEqual(u, URL(Schema.CHARM_STORE, name="mysql", user="fred"))
@@ -33,22 +29,33 @@ class TestURLV1(unittest.TestCase):
 
 
 class TestURLV2(unittest.TestCase):
+
+    schema = Schema.CHARM_HUB
+
     def test_parse_charmhub(self):
-        u = URL.parse("ch:arm64/bionic/mysql-1")
-        self.assertEqual(u, URL(Schema.CHARM_HUB, name="mysql", architecture="arm64", series="bionic", revision=1))
+        u = URL.parse(f"{self.schema}:arm64/bionic/mysql-1")
+        self.assertEqual(u, URL(self.schema, name="mysql", architecture="arm64", series="bionic", revision=1))
 
     def test_parse_charmhub_with_no_series(self):
-        u = URL.parse("ch:arm64/mysql")
-        self.assertEqual(u, URL(Schema.CHARM_HUB, name="mysql", architecture="arm64"))
+        u = URL.parse(f"{self.schema}:arm64/mysql")
+        self.assertEqual(u, URL(self.schema, name="mysql", architecture="arm64"))
 
     def test_parse_charmhub_with_no_series_arch(self):
-        u = URL.parse("ch:mysql")
-        self.assertEqual(u, URL(Schema.CHARM_HUB, name="mysql"))
+        u = URL.parse(f"{self.schema}:mysql")
+        self.assertEqual(u, URL(self.schema, name="mysql"))
 
     def test_parse_v2_revision(self):
-        u = URL.parse("ch:mysql-1")
-        self.assertEqual(u, URL(Schema.CHARM_HUB, name="mysql", revision=1))
+        u = URL.parse(f"{self.schema}:mysql-1")
+        self.assertEqual(u, URL(self.schema, name="mysql", revision=1))
 
     def test_parse_v2_large_revision(self):
-        u = URL.parse("ch:mysql-12345")
-        self.assertEqual(u, URL(Schema.CHARM_HUB, name="mysql", revision=12345))
+        u = URL.parse(f"{self.schema}:mysql-12345")
+        self.assertEqual(u, URL(self.schema, name="mysql", revision=12345))
+
+    def test_parse_v2_without_store(self):
+        u = URL.parse("mysql-1", default_store=self.schema)
+        self.assertEqual(u, URL(self.schema, name="mysql", revision=1))
+
+
+class TestURLLocal(TestURLV2):
+    schema = Schema.LOCAL