Merge pull request #974 from jack-w-shaw/JUJU-4779_parse_local_charms_properly-master

#974

Port forward #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 #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())
```

Author: jujubot

juju/bundle.py

@@ -229,7 +229,7 @@ def _resolve_include_file_config(self, bundle_dir):
 
         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 @@ async def fetch_plan(self, bundle_url, origin, overlays=[]):
 
         :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 @@ async def fetch_plan(self, bundle_url, origin, overlays=[]):
 
         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 @@ async def fetch_plan(self, bundle_url, origin, overlays=[]):
         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 @@ async def _resolve_charms(self):
                                             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 @@ async def run(self, context):
 
         # 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 @@ async def run(self, context):
         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)] = {}

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 @@ async def resolve(self, url, architecture,
         -- 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 @@ async def resolve(self, url, architecture,
             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 @@ def __init__(
         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 @@ async def deploy(
             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 @@ async def deploy(
             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 @@ async def deploy(
 
         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 @@ async def deploy(
                                                      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 @@ async def deploy(
                                                                        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 @@ async def _resolve_charm(self, url, origin, force=False, series=None, model_conf
         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 @@ async def _resolve_charm(self, url, origin, force=False, series=None, model_conf
         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()

juju/url.py

@@ -43,13 +43,11 @@ def parse(s, default_store=Schema.CHARM_HUB):
         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)