Merge pull request #830 from cderici/deploy-by-revision

#830

#### Description

This adds the feature that allows deploying by revision, by passing through the revision info into the `ResolveCharms` API call. 

Fixes #690 


#### QA Steps

An example is added, so it should work:

```
 $ python examples/deploywithrevision.py
```

- [x] Along with that example, I need to write some integration tests for this before landing (see discussion below).

```
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision
```

```
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision_validate_flags
```

All CI tests need to pass.


#### Notes & Discussion

- [x] Before landing, we need to make sure that the input validation is being handled gracefully. For regular charms, `--revision` requires `--channel`, however for bundles, `--channel` and `--revision` are mutually exclusive. The `ResolveCharms` call should be getting us the right errors for those, we just need to make sure those errors are passed through correctly. (there should be integration tests for these validations)

Author: jujubot

examples/deploy_with_revision.py

@@ -0,0 +1,33 @@
+from juju import jasyncio
+from juju.model import Model
+
+
+async def main():
+    charm = 'juju-qa-test'
+
+    model = Model()
+    print('Connecting to model')
+    # connect to current model with current user, per Juju CLI
+    await model.connect()
+
+    try:
+        print(f'Deploying {charm} --channel 2.0/stable --revision 22')
+        application = await model.deploy(
+            'juju-qa-test',
+            application_name='test',
+            channel='2.0/stable',
+            revision=22,
+        )
+
+        print('Waiting for active')
+        await model.wait_for_idle(status='active')
+
+        print(f'Removing {charm}')
+        await application.remove()
+    finally:
+        print('Disconnecting from model')
+        await model.disconnect()
+
+
+if __name__ == '__main__':
+    jasyncio.run(main())

juju/bundle.py

@@ -17,6 +17,7 @@
 from . import utils, jasyncio
 from .origin import Channel, Source
 from .url import Schema, URL
+from .utils import get_base_from_origin_or_channel
 
 log = logging.getLogger(__name__)
 
@@ -350,11 +351,13 @@ async def _resolve_charms(self):
 
             charm_url = URL.parse(spec['charm'])
             channel = None
-            series = spec.get('series', None)
+            base = None
             track, risk = '', ''
             if 'channel' in spec:
                 channel = Channel.parse(spec['channel'])
                 track, risk = channel.track, channel.risk
+                series = spec.get('series', None)
+                base = get_base_from_origin_or_channel(channel, series)
 
             if self.charms_facade is not None:
                 if cons is not None and cons['arch'] != '':
@@ -365,16 +368,18 @@ async def _resolve_charms(self):
                 origin = client.CharmOrigin(source=Source.CHARM_HUB.value,
                                             architecture=architecture,
                                             risk=risk,
-                                            track=track)
-                if not self.model.connection().is_using_old_client and series:
-                    origin.base = client.Base(
-                        channel=utils.get_series_version(series), name='ubuntu')
-                charm_url, charm_origin, _ = await self.model._resolve_charm(charm_url, origin)
+                                            track=track,
+                                            base=base,
+                                            )
+
+                charm_url, charm_origin = await self.model._resolve_charm(charm_url, origin)
                 spec['charm'] = str(charm_url)
             else:
                 charm_origin = client.CharmOrigin(source=Source.CHARM_HUB.value,
                                                   risk=risk,
-                                                  track=track)
+                                                  track=track,
+                                                  base=base,
+                                                  )
 
             if str(channel) not in self.origins:
                 self.origins[str(charm_url)] = {}
@@ -715,15 +720,13 @@ async def run(self, context):
             arch = self.architecture
             if not arch:
                 arch = await context.model._resolve_architecture(url)
+            base = get_base_from_origin_or_channel(ch, self.series)
             origin = client.CharmOrigin(source=Source.CHARM_HUB.value,
                                         architecture=arch,
                                         risk=ch.risk,
-                                        track=ch.track)
-            if not context.model.connection().is_using_old_client and self.series:
-                origin.base = client.Base(
-                    channel=utils.get_series_version(self.series),
-                    name='ubuntu')
-            identifier, origin, _ = await context.model._resolve_charm(url, origin)
+                                        track=ch.track,
+                                        base=base)
+            identifier, origin = await context.model._resolve_charm(url, origin)
 
         if identifier is None:
             raise JujuError('unknown charm {}'.format(self.charm))

juju/model.py

@@ -441,10 +441,15 @@ class LocalDeployType:
     """LocalDeployType deals with local only deployments.
     """
 
-    async def resolve(self, url, architecture, app_name=None, channel=None, series=None, entity_url=None):
+    async def resolve(self, url, architecture,
+                      app_name=None, channel=None, series=None,
+                      revision=None, entity_url=None, force=False,
+                      model_conf=None):
         """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.
+
+        -- revision flag is ignored for local charms
         """
 
         entity_url = url.path()
@@ -492,11 +497,16 @@ class CharmhubDeployType:
     def __init__(self, charm_resolver):
         self.charm_resolver = charm_resolver
 
-    async def resolve(self, url, architecture, app_name=None, channel=None, series=None, entity_url=None):
+    async def resolve(self, url, architecture,
+                      app_name=None, channel=None, series=None,
+                      revision=None, entity_url=None, force=False,
+                      model_conf=None):
         """resolve attempts to resolve charmhub charms or bundles. A request to
         the charmhub API is required to correctly determine the charm url and
         underlying origin.
         """
+        if revision and not channel:
+            raise JujuError('specifying a revision requires a channel for future upgrades. Please use --channel')
 
         ch = Channel('latest', 'stable')
         if channel is not None:
@@ -511,26 +521,23 @@ async def resolve(self, url, architecture, app_name=None, channel=None, series=N
                                     risk=ch.risk,
                                     track=ch.track,
                                     base=base,
+                                    revision=revision,
                                     )
 
-        charm_url_str, origin, supported_series = await self.charm_resolver(url, origin)
-        charm_url = URL.parse(charm_url_str)
+        charm_url, origin = await self.charm_resolver(url, origin, force, series, model_conf)
+
+        is_bundle = origin.type_ == "bundle"
+        if is_bundle and revision and channel:
+            raise JujuError('revision and channel are mutually exclusive when deploying a bundle. Please choose one.')
 
         if app_name is None:
             app_name = url.name
 
-        if series:
-            if series in supported_series:
-                origin.series = series
-                charm_url.series = series
-            else:
-                raise JujuError("Series {} not supported for {}. Only {}".format(series, url, supported_series))
-
         return DeployTypeResult(
             identifier=str(charm_url),
             app_name=app_name,
             origin=origin,
-            is_bundle=origin.type_ == "bundle",
+            is_bundle=is_bundle,
         )
 
 
@@ -1611,7 +1618,7 @@ async def debug_log(
     async def deploy(
             self, entity_url, application_name=None, bind=None,
             channel=None, config=None, constraints=None, force=False,
-            num_units=1, overlays=[], base=None, resources=None, series=None,
+            num_units=1, overlays=[], base=None, resources=None, series=None, revision=None,
             storage=None, to=None, devices=None, trust=False, attach_storage=[]):
         """Deploy a new service or bundle.
 
@@ -1630,6 +1637,8 @@ async def deploy(
         :param str base: The base on which to deploy
         :param dict resources: <resource name>:<file path> pairs
         :param str series: Series on which to deploy DEPRECATED: use --base (with Juju 3.1)
+        :param int revision: specifying a revision requires a channel for future upgrades for charms.
+            For bundles, revision and channel are mutually exclusive.
         :param dict storage: Storage constraints TODO how do these look?
         :param to: Placement directive as a string. For example:
 
@@ -1675,7 +1684,12 @@ async def deploy(
         if str(url.schema) not in self.deploy_types:
             raise JujuError("unknown deploy type {}, expected charmhub or local".format(url.schema))
 
-        res = await self.deploy_types[str(url.schema)].resolve(url, architecture, application_name, channel, series, entity_url)
+        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)
 
         if res.identifier is None:
             raise JujuError('unknown charm or bundle {}'.format(entity_url))
@@ -1772,6 +1786,7 @@ async def deploy(
                 devices=devices,
                 charm_origin=charm_origin,
                 attach_storage=attach_storage,
+                force=force,
             )
 
     async def _add_charm(self, charm_url, origin):
@@ -1785,7 +1800,7 @@ async def _add_charm(self, charm_url, origin):
         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):
+    async def _resolve_charm(self, url, origin, force=False, series=None, model_config=None):
         """Calls Charms.ResolveCharms to resolve all the fields of the
         charm_origin and also the url and the supported_series
 
@@ -1812,7 +1827,8 @@ async def _resolve_charm(self, url, origin):
 
         resolve_origin = {'source': source, 'architecture': origin.architecture,
                           'track': origin.track, 'risk': origin.risk,
-                          'base': origin.base}
+                          'base': origin.base, 'revision': origin.revision,
+                          }
 
         resp = await charms_facade.ResolveCharms(resolve=[{
             'reference': str(url),
@@ -1825,7 +1841,16 @@ async def _resolve_charm(self, url, origin):
         if result.error:
             raise JujuError(result.error.message)
 
-        return (result.url, result.charm_origin, result.supported_series)
+        supported_series = result.supported_series
+        resolved_origin = result.charm_origin
+        charm_url = URL.parse(result.url)
+
+        # run the series selector to get a series for the base
+        selected_series = utils.series_selector(series, url, model_config, supported_series, force)
+        result.charm_origin.base = utils.get_base_from_origin_or_channel(resolved_origin, selected_series)
+        charm_url.series = selected_series
+
+        return result.url, result.charm_origin
 
     async def _resolve_architecture(self, url):
         if url.architecture:
@@ -1956,7 +1981,8 @@ async def add_local_resources(self, application, entity_url, metadata, resources
     async def _deploy(self, charm_url, application, series, config,
                       constraints, endpoint_bindings, resources, storage,
                       channel=None, num_units=None, placement=None,
-                      devices=None, charm_origin=None, attach_storage=[]):
+                      devices=None, charm_origin=None, attach_storage=[],
+                      force=False):
         """Logic shared between `Model.deploy` and `BundleHandler.deploy`.
         """
         log.info('Deploying %s', charm_url)
@@ -1984,6 +2010,7 @@ async def _deploy(self, charm_url, application, series, config,
             placement=placement,
             devices=devices,
             attach_storage=attach_storage,
+            force=force,
         )
         result = await app_facade.Deploy(applications=[app])
         errors = [r.error.message for r in result.results if r.error]

juju/utils.py

@@ -11,6 +11,7 @@
 
 from . import jasyncio, origin, errors
 from .client import client
+from .errors import JujuError
 
 
 async def execute_process(*cmd, log=None):
@@ -412,7 +413,104 @@ def parse_base_arg(base):
     """
     client.CharmBase()
     if type(base) != str or "@" not in base:
-        raise errors.JujuError("expected base string to contain os and channel separated by '@'")
+        raise errors.JujuError(f"expected base string to contain os and channel separated by '@', got : {base}")
 
     name, channel = base.split('@')
     return client.Base(name=name, channel=channel)
+
+
+DEFAULT_SUPPORTED_LTS = 'jammy'
+DEFAULT_SUPPORTED_LTS_BASE = client.Base(channel='22.04', name='ubuntu')
+
+
+def base_channel_from_series(track, risk, series=None):
+    return origin.Channel(track=track, risk=risk).normalize().compute_base_channel(series=series)
+
+
+def get_os_from_series(series=None):
+    if not series or series in UBUNTU_SERIES:
+        return 'ubuntu'
+    raise JujuError(f'os for the series {series} needs to be added')
+
+
+def get_base_from_origin_or_channel(origin_or_channel, series=None):
+    channel = base_channel_from_series(origin_or_channel.track, origin_or_channel.risk, series)
+    os_name = get_os_from_series(series)
+    return client.Base(channel=channel, name=os_name)
+
+
+def series_for_charm(requested_series, supported_series):
+    """series_for_charm takes a requested series and a list of series supported by a
+    charm and returns the series which is relevant.
+    If the requested series is empty, then the first supported series is used,
+    otherwise the requested series is validated against the supported series.
+    """
+    if len(supported_series) == 1 and supported_series[0] == '':
+        raise JujuError("invalid supported series reported by charm : ['']")
+    if len(supported_series) == 0:
+        if requested_series == '':
+            raise JujuError("missing series")
+        return requested_series
+
+    # use the charm default
+    if requested_series == '':
+        return supported_series[-1]
+
+    for s in supported_series:
+        if requested_series == s:
+            return requested_series
+    raise JujuError(f'requested series {requested_series} is not among the supported series {supported_series}')
+
+
+def user_requested(series_arg, supported_series, force):
+    series = series_for_charm(series_arg, supported_series)
+    if force:
+        series = series_arg
+    # Todo (cderici): validate the series with workload_series to see if juju is supporting that
+    return series
+
+
+def series_selector(series_arg='', charm_url=None, model_config=None, supported_series=[], force=False):
+    """
+    series_selector corresponds to the CharmSeries() in
+    https://github.com/juju/juju/blob/develop/core/charm/series_selector.go
+
+    determines what series to use with a charm.
+    Order of preference is:
+    - user requested with --series or defined by bundle when deploying
+    - user requested in charm's url (e.g. juju deploy jammy/ubuntu)
+    - model default, if set, acts like --series
+    - default from charm metadata supported series / series in url
+    - default LTS
+    """
+
+    # User has requested a series with --series.
+    if series_arg:
+        return user_requested(series_arg, supported_series, force)
+
+    # User specified a series in the charm URL, e.g.
+    # juju deploy precise/ubuntu.
+    if charm_url and charm_url.series:
+        return user_requested(charm_url.series, supported_series, force)
+
+    # No series explicitly requested by the user.
+    # Use model default series, if explicitly set and supported by the charm.
+    if model_config and model_config['default-base'].value:
+        default_base = model_config['default-base'].value
+        base = parse_base_arg(default_base)
+        series = base_channel_to_series(base.channel)
+        return user_requested(series, supported_series, force)
+
+    # Next fall back to the charm's list of series, filtered to what's supported
+    # by Juju. Preserve the order of the supported series from the charm
+    # metadata, as the order could be out of order compared to Ubuntu series
+    # order (precise, xenial, bionic, trusty, etc).
+    try:
+        # TODO (cderici): restrict the supported_series with JujuSupportedSeries
+        return user_requested('', supported_series, force)
+    except JujuError:
+        pass
+
+    # Charm hasn't specified a default (likely due to being a local charm
+    # deployed by path). Last chance, best we can do is default to LTS.
+    return DEFAULT_SUPPORTED_LTS

tests/integration/bundle/test-overlays/bundle-with-overlay-multi.yaml

@@ -5,7 +5,7 @@ applications:
     num_units: 1
   mysql:
     charm: "mysql"
-    channel: candidate
+    channel: 8.0/stable
     num_units: 1
 relations:
   - ["ghost", "mysql"]