Repository: juju/python-libjuju
Pull request #974 (2023-10-25 15:48:24)

Parse charm URLs consistantly for local charms
Port forward https://github.com/juju/python-libjuju/pull/969

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 unit -- 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())
```

jack-w-shaw: /merge
cderici: Might need a rebase

Patch:
diff --git a/juju/bundle.py b/juju/bundle.py
index a52252a..e8eee57 100644
--- a/juju/bundle.py
+++ b/juju/bundle.py
@@ -229,7 +229,7 @@ class BundleHandler:
 
         return self.bundle, self.overlays
 
-    async def fetch_plan(self, bundle_url, origin, overlays=[]):
+    async def fetch_plan(self, bundle, origin, overlays=[]):
         """fetch_plan is called by the model.deploy(). It gathers the information about the
         bundle to be deployed (whether local or CharmHub), straightens it up, applies overlays
         if any overlays are given. Validates the bundle against known issues. Resolves and adds
@@ -245,19 +245,21 @@ class BundleHandler:
 
         :returns: None
         """
-        entity_id = bundle_url.path()
-        is_local = Schema.LOCAL.matches(bundle_url.schema)
         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_HUB.matches(bundle_url.schema):
-            bundle_yaml = await self._download_bundle(bundle_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()
+            bundle_yaml = await self._download_bundle(bundle, origin)
 
         if not bundle_yaml:
             raise JujuError('empty bundle, nothing to deploy')
@@ -284,7 +286,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)
@@ -295,7 +297,7 @@ class BundleHandler:
         yaml_data = "---\n".join(_yaml_data)
 
         self.plan = await self.bundle_facade.GetChangesMapArgs(
-            bundleurl=entity_id,
+            bundleurl=path,
             yaml=yaml_data)
 
         if self.plan.errors and any(self.plan.errors):
@@ -389,7 +391,6 @@ class BundleHandler:
                                             track=track,
                                             base=base,
                                             )
-
                 charm_url, charm_origin = await self.model._resolve_charm(charm_url, origin)
                 spec['charm'] = str(charm_url)
             else:
@@ -443,6 +444,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).
@@ -676,12 +692,12 @@ class AddCharmChange(ChangeInfo):
 
         # We don't add local charms because they've already been added
         # by self._handle_local_charms
+        if is_local_charm(str(self.charm)):
+            return self.charm
+
         url = URL.parse(str(self.charm))
         ch = None
         identifier = None
-        if Schema.LOCAL.matches(url.schema):
-            return self.charm
-
         if Schema.CHARM_HUB.matches(url.schema):
             ch = Channel('latest', 'stable')
             if self.channel:
@@ -700,7 +716,7 @@ class AddCharmChange(ChangeInfo):
         if identifier is None:
             raise JujuError('unknown charm {}'.format(self.charm))
 
-        await context.model._add_charm(identifier, origin)
+        await context.model._add_charm(str(identifier), origin)
 
         if str(ch) not in context.origins:
             context.origins[str(identifier)] = {}
diff --git a/juju/model.py b/juju/model.py
index df58f99..794c235 100644
--- a/juju/model.py
+++ b/juju/model.py
@@ -450,7 +450,7 @@ class LocalDeployType:
     """LocalDeployType deals with local only deployments.
     """
 
-    async def resolve(self, url, architecture,
+    async def resolve(self, charm_path, architecture,
                       app_name=None, channel=None, series=None,
                       revision=None, entity_url=None, force=False,
                       model_conf=None):
@@ -461,36 +461,27 @@ class LocalDeployType:
         -- revision flag is ignored for local charms
         """
 
-        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,
@@ -541,7 +532,7 @@ class CharmhubDeployType:
             raise JujuError('revision and channel are mutually exclusive when deploying a bundle. Please choose one.')
 
         if app_name is None:
-            app_name = url.name
+            app_name = charm_url.name
 
         return DeployTypeResult(
             identifier=str(charm_url),
@@ -591,8 +582,8 @@ class Model:
         self._charmhub = CharmHub(self)
 
         self.deploy_types = {
-            "local": LocalDeployType(),
-            "ch": CharmhubDeployType(self._resolve_charm),
+            Schema.LOCAL: LocalDeployType(),
+            Schema.CHARM_HUB: CharmhubDeployType(self._resolve_charm),
         }
 
     def is_connected(self):
@@ -1670,7 +1661,7 @@ class Model:
             storage=None, to=None, devices=None, trust=False, attach_storage=[]):
         """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
@@ -1716,26 +1707,33 @@ class Model:
             raise JujuError("Expected attach_storage to be a list of strings, given {}".format(attach_storage))
 
         # 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 or local".format(url.schema))
+        if schema not in self.deploy_types:
+            raise JujuError("unknown deploy type {}, expected charmhub or local".format(schema))
 
         model_conf = await self.get_config()
-        res = await self.deploy_types[str(url.schema)].resolve(url, architecture,
-                                                               application_name, channel,
-                                                               series, revision,
-                                                               entity_url, force,
-                                                               model_conf)
+        res = await self.deploy_types[schema].resolve(entity, architecture,
+                                                      application_name, channel,
+                                                      series, revision,
+                                                      entity_url, force,
+                                                      model_conf)
 
         if res.identifier is None:
             raise JujuError('unknown charm or bundle {}'.format(entity_url))
@@ -1750,7 +1748,7 @@ class Model:
 
         if res.is_bundle:
             handler = BundleHandler(self, trusted=trust, forced=force)
-            await handler.fetch_plan(url, charm_origin, overlays=overlays)
+            await handler.fetch_plan(entity, charm_origin, overlays=overlays)
             await handler.execute_plan()
             extant_apps = {app for app in self.applications}
             pending_apps = handler.applications - extant_apps
@@ -1778,8 +1776,7 @@ class Model:
                                                      charm_origin)
                 else:
                     charm_origin = add_charm_res.charm_origin
-                if Schema.CHARM_HUB.matches(url.schema):
-
+                if Schema.CHARM_HUB.matches(schema):
                     if client.ApplicationFacade.best_facade_version(self.connection()) >= 19:
                         server_side_deploy = True
                     else:
@@ -1793,7 +1790,7 @@ class Model:
                                                                        identifier,
                                                                        add_charm_res.charm_origin)
 
-                    is_sub = await self.charmhub.is_subordinate(url.name)
+                    is_sub = await self.charmhub.is_subordinate(name)
                     if is_sub:
                         if num_units > 1:
                             raise JujuError("cannot use num_units with subordinate application")
@@ -1879,7 +1876,7 @@ class Model:
         Returns the confirmed origin returned by the Juju API to be used in
         calls like ApplicationFacade.Deploy
 
-        :returns str, client.CharmOrigin, [str]
+        :returns url.URL, client.CharmOrigin, [str]
         """
         charms_cls = client.CharmsFacade
         if charms_cls.best_facade_version(self.connection()) < 3:
@@ -1919,16 +1916,18 @@ class Model:
         result.charm_origin.base = utils.get_base_from_origin_or_channel(resolved_origin, selected_series)
         charm_url.series = selected_series
 
-        return str(charm_url), resolved_origin
+        return charm_url, resolved_origin
 
-    async def _resolve_architecture(self, url):
+    async def _resolve_architecture(self, url=None):
         """_resolve_architecture returns the architecture for a given charm url.
+        If the charm url is absent, or doesn't specific an arch, we return the
+        default architecture from the model.
+
         :param str url: the client.URL to determine the arch for
 
         :returns str architecture for the given url
         """
-
-        if url.architecture:
+        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 d1f3c6e..cd872f6 100644
--- a/tests/unit/test_url.py
+++ b/tests/unit/test_url.py
@@ -7,28 +7,55 @@ from juju.url import Schema, URL
 
 
 class TestURLV1(unittest.TestCase):
-    def test_parse_local(self):
-        u = URL.parse("local:mysql")
-        self.assertEqual(u, URL(Schema.LOCAL, name="mysql"))
+    def test_parse_charmstore(self):
+        u = URL.parse("cs:mysql")
+        self.assertEqual(u, URL(Schema.CHARM_STORE, 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"))
+
+    def test_parse_v1_revision(self):
+        u = URL.parse("cs:~fred/mysql-1")
+        self.assertEqual(u, URL(Schema.CHARM_STORE, name="mysql", user="fred", revision=1))
+
+    def test_parse_v1_large_revision(self):
+        u = URL.parse("cs:~fred/mysql-12345")
+        self.assertEqual(u, URL(Schema.CHARM_STORE, name="mysql", user="fred", revision=12345))
+
+    def test_parse_v1_series(self):
+        u = URL.parse("cs:~fred/bionic/mysql-1")
+        self.assertEqual(u, URL(Schema.CHARM_STORE, name="mysql", user="fred", revision=1, series="bionic"))
 
 
 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