Merge lp:~makyo/juju-deployer/new-bundle-format into lp:juju-deployer

[Madison Scott-Clary]

Support the new v4 bundle format, first iteration. This supports bundles without the top-level yaml tag - next iteration will support machine placement directives.

To QA:

- tests should pass
- run `PYTHONPATH=. python deployer/cli.py -c deployer/tests/test_data/blog_v4.yaml` to deploy a sample v4 bundle to a bootstrapped environment.

[j.c.sackett (jcsackett)]

Code looks good. QAing now.

[j.c.sackett (jcsackett)]

QA OK. Thanks for this, Madison.

review: Approve

[lp:~makyo/juju-deployer/new-bundle-format]

138. By Madison Scott-Clary on 2015-02-20

Simplify gets

[Kapil Thangavelu (hazmat)]

more inline comments

[lp:~makyo/juju-deployer/new-bundle-format]

139. By Madison Scott-Clary on 2015-02-23

Simplify version checking; test that _resolve_inherited is not called

[Richard Harding (rharding)]

+1 with the comments from kapil on the missing test and more pythonic detection.

Kapil, I'd prefer to get your ok on the fingerprinting vs a version defined in the file itself as we're only talking the first 'versioning' going on here and it's a pretty light diff by just not allowing multiple bundles to be defined in the single file (thus the raising of services to the root vs under a name).

Is there another big reason for the explicit versioning in the yaml?

review: Approve

[Kapil Thangavelu (hazmat)]

On Mon, Feb 23, 2015 at 3:29 PM, Richard Harding <<email address hidden>
> wrote:

> Review: Approve
>
> +1 with the comments from kapil on the missing test and more pythonic
> detection.
>
> Kapil, I'd prefer to get your ok on the fingerprinting vs a version
> defined in the file itself as we're only talking the first 'versioning'
> going on here and it's a pretty light diff by just not allowing multiple
> bundles to be defined in the single file (thus the raising of services to
> the root vs under a name).
>
> Is there another big reason for the explicit versioning in the yaml?
>

because there are other format changes inbound, say we add 'machines' or
other variants, .. i'd rather not have a morass of implicit checks vs an
explicit version.

>
> Diff comments:
>
> > === modified file '.bzrignore'
> > --- .bzrignore 2013-12-18 09:52:33 +0000
> > +++ .bzrignore 2015-02-20 19:52:56 +0000
> > @@ -10,3 +10,5 @@
> > juju-deployer
> > juju-deployer.sublime-workspace
> > tmp
> > +report/
> > +.coverage
> >
> > === modified file 'deployer/config.py'
> > --- deployer/config.py 2014-08-28 20:14:01 +0000
> > +++ deployer/config.py 2015-02-20 19:52:56 +0000
> > @@ -19,6 +19,7 @@
> > def __init__(self, config_files, cli_series=None):
> > self.config_files = config_files
> > self.cli_series = cli_series
> > + self.version = 3
> > self.data = {}
> > self.yaml = {}
> > self.include_dirs = []
> > @@ -42,13 +43,21 @@
> >
> > with open(config_file) as fh:
> > try:
> > - self.yaml[config_file] = yaml_load(fh.read())
> > + yaml_result = yaml_load(fh.read())
> > except Exception, e:
> > self.log.warning(
> > "Couldn't load config file @ %r, error: %s:%s",
> > config_file, type(e), e)
> > raise
> >
> > + # Check if this is a v4 bundle.
>
> Since bundles are often hand written relying on a field in them to
> determine the version seems like it'll lead to a less than ideal process,
> we'd want to guess in the case the version isn't present anyway.
>
> > + if yaml_result.get('services') is not None \
>
> +1 just check
>
> if 'services' in yaml_result:
>
> > + and yaml_result['services'].get('services') is None:
> > + self.version = 4
> > + yaml_result = {config_file: yaml_result}
> > +
> > + self.yaml[config_file] = yaml_result
> > +
> > return self.yaml[config_file]
> >
> > def keys(self):
> > @@ -60,7 +69,8 @@
> > key, ", ".join(self.keys()))
> > raise ErrorExit()
> > deploy_data = self.data[key]
> > - deploy_data = self._resolve_inherited(deploy_data)
> > + if self.version != 4:
> > + deploy_data = self._resolve_inherited(deploy_data)
> > if self.cli_series:
> > deploy_data['series'] = self.cli_series
> > return Deployment(
> >
> > === modified file 'deployer/tests/test_config.py'
> > --- deployer/tests/test_config.py 2014-01-23 15:02:57 +0000
> > ...

[Richard Harding (rharding)]

> > Is there another big reason for the explicit versioning in the yaml?
> >
>
> because there are other format changes inbound, say we add 'machines' or
> other variants, .. i'd rather not have a morass of implicit checks vs an
> explicit version.

But adding a key like machines isn't a backwards incompatible change. I'd not think to increment the revision of the file based on a change like that.

[lp:~makyo/juju-deployer/new-bundle-format]

140. By Madison Scott-Clary on 2015-02-24

Remove extra services check.

[David Britton (dpb)]

Merged with a couple extra test checks (that version was being set for old and new bundles)

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2013-12-18 09:52:33 +0000
+++ .bzrignore	2015-02-24 15:49:41 +0000
@@ -10,3 +10,5 @@
 juju-deployer
 juju-deployer.sublime-workspace
 tmp
+report/
+.coverage

=== modified file 'deployer/config.py'
--- deployer/config.py	2014-08-28 20:14:01 +0000
+++ deployer/config.py	2015-02-24 15:49:41 +0000
@@ -19,6 +19,7 @@
     def __init__(self, config_files, cli_series=None):
         self.config_files = config_files
         self.cli_series = cli_series
+        self.version = 3
         self.data = {}
         self.yaml = {}
         self.include_dirs = []
@@ -42,13 +43,20 @@
 
         with open(config_file) as fh:
             try:
-                self.yaml[config_file] = yaml_load(fh.read())
+                yaml_result = yaml_load(fh.read())
             except Exception, e:
                 self.log.warning(
                     "Couldn't load config file @ %r, error: %s:%s",
                     config_file, type(e), e)
                 raise
 
+        # Check if this is a v4 bundle.
+        if 'services' in yaml_result:
+            self.version = 4
+            yaml_result = {config_file: yaml_result}
+
+        self.yaml[config_file] = yaml_result
+
         return self.yaml[config_file]
 
     def keys(self):
@@ -60,7 +68,8 @@
                              key, ", ".join(self.keys()))
             raise ErrorExit()
         deploy_data = self.data[key]
-        deploy_data = self._resolve_inherited(deploy_data)
+        if self.version != 4:
+            deploy_data = self._resolve_inherited(deploy_data)
         if self.cli_series:
             deploy_data['series'] = self.cli_series
         return Deployment(

=== modified file 'deployer/tests/test_config.py'
--- deployer/tests/test_config.py	2014-01-23 15:02:57 +0000
+++ deployer/tests/test_config.py	2015-02-24 15:49:41 +0000
@@ -1,4 +1,5 @@
 import logging
+import mock
 import os
 import tempfile
 import yaml
@@ -39,6 +40,19 @@
         deployment = config.get("wordpress")
         self.assertTrue(deployment)
 
+    def test_config_v4(self):
+        config = ConfigStack([
+            os.path.join(self.test_data_dir, 'blog_v4.yaml')])
+        config.load()
+        self.assertEqual(
+            config.keys(),
+            [os.path.join(self.test_data_dir, 'blog_v4.yaml')])
+        with mock.patch('deployer.config.ConfigStack._resolve_inherited') \
+                as mock_resolve:
+            deployment = config.get(config.keys()[0])
+            self.assertTrue(deployment)
+            self.assertFalse(mock_resolve.called)
+
     def test_config_include_file(self):
         config = ConfigStack([
             os.path.join(self.test_data_dir, "stack-includes.cfg")])

=== added file 'deployer/tests/test_data/blog_v4.yaml'
--- deployer/tests/test_data/blog_v4.yaml	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_data/blog_v4.yaml	2015-02-24 15:49:41 +0000
@@ -0,0 +1,36 @@
+services:
+  mediawiki:
+    charm: cs:precise/mediawiki-10
+    num_units: 1
+    options:
+      debug: false
+      name: Please set name of wiki
+      skin: vector
+    annotations:
+      gui-x: "609"
+      gui-y: "-15"
+  mysql:
+    charm: cs:precise/mysql-28
+    num_units: 1
+    options:
+      binlog-format: MIXED
+      block-size: 5
+      dataset-size: 80%
+      flavor: distro
+      ha-bindiface: eth0
+      ha-mcastport: 5411
+      max-connections: -1
+      preferred-storage-engine: InnoDB
+      query-cache-size: -1
+      query-cache-type: "OFF"
+      rbd-name: mysql1
+      tuning-level: safest
+      vip_cidr: 24
+      vip_iface: eth0
+    annotations:
+      gui-x: "610"
+      gui-y: "255"
+series: precise
+relations:
+- - mediawiki:db
+  - mysql:db