Merge lp:~makyo/juju-quickstart/no-gui-in-bundle into lp:juju-quickstart

[Madison Scott-Clary]

Error on juju-gui in the bundle services

Quickstart should fail if there is an instance of juju-gui in the services, because that will lead to a name conflict when deploying the bundle.

To test: `make check`

To QA: `.venv/bin/python juju-quickstart https://gist.github.com/makyo/7435270/raw/6f0d5ff910dd1a82b97c7170915a196381b2b146/bundle.yaml` should fail with a message explaining that juju-gui should be removed from the bundle; other bundles will still succeed.

https://codereview.appspot.com/25340043/

[Madison Scott-Clary (makyo)]

Reviewers: mp+194907_code.launchpad.net,

Message:
Please take a look.

Description:
Error on juju-gui in the bundle services

Quickstart should fail if there is an instance of juju-gui in the
services, because that will lead to a name conflict when deploying the
bundle.

To test: `make check`

To QA: `.venv/bin/python juju-quickstart
https://gist.github.com/makyo/7435270/raw/6f0d5ff910dd1a82b97c7170915a196381b2b146/bundle.yaml`
should fail with a message explaining that juju-gui should be removed
from the bundle; other bundles will still succeed.

https://code.launchpad.net/~makyo/juju-quickstart/no-gui-in-bundle/+merge/194907

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/25340043/

Affected files (+18, -2 lines):
   A [revision details]
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: quickstart/utils.py
=== modified file 'quickstart/utils.py'
--- quickstart/utils.py 2013-11-06 10:16:42 +0000
+++ quickstart/utils.py 2013-11-12 17:45:33 +0000
@@ -151,8 +151,11 @@
      if not bundle_services:
          msg = 'bundle {} does not include any services'.format(bundle_name)
          raise ValueError(msg)
- # XXX 2013-10-21 frankban:
- # Handle the case when the GUI is included in the bundle.
+ if 'juju-gui' in bundle_services:
+ raise ValueError('bundle {} contains an instance of juju-gui. '
+ 'quickstart will install the latest version of
the '
+ 'Juju GUI automatically, please remove juju-gui
from '
+ 'the bundle.'.format(bundle_name))
      return bundle_name, bundle_services

Index: quickstart/tests/test_utils.py
=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py 2013-11-06 14:55:30 +0000
+++ quickstart/tests/test_utils.py 2013-11-12 17:45:33 +0000
@@ -233,6 +233,17 @@
          with self.assert_value_error(expected):
              utils.parse_bundle(contents)

+ def test_yaml_gui_in_services(self):
+ # A ValueError is raised if the bundle contains juju-gui.
+ contents = yaml.safe_dump({
+ 'mybundle': {'services': {'juju-gui': {}}},
+ })
+ expected = 'bundle mybundle contains an instance of juju-gui. ' \
+ 'quickstart will install the latest version of the Juju GUI ' \
+ 'automatically, please remove juju-gui from the bundle.'
+ with self.assert_value_error(expected):
+ utils.parse_bundle(contents)
+
      def test_success_no_name(self):
          # The function succeeds when an implicit bundle name is used.
          contents = yaml.safe_dump({

[Brad Crittenden (bac)]

LGTM with small change and QA-OK

https://codereview.appspot.com/25340043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/25340043/diff/1/quickstart/utils.py#newcode155
quickstart/utils.py:155: raise ValueError('bundle {} contains an
instance of juju-gui. '
Would be more readable with quotes around the bundle name in the error
output.

https://codereview.appspot.com/25340043/

[Madison Scott-Clary (makyo)]

Thanks for the review!

https://codereview.appspot.com/25340043/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/25340043/diff/1/quickstart/utils.py#newcode155
quickstart/utils.py:155: raise ValueError('bundle {} contains an
instance of juju-gui. '
On 2013/11/12 18:11:47, bac wrote:
> Would be more readable with quotes around the bundle name in the error
output.

I agree. Would like to defer until we can get all, however. I started
in on this but there are ramifications all throughout the project.

https://codereview.appspot.com/25340043/

[Madison Scott-Clary (makyo)]

*** Submitted:

Error on juju-gui in the bundle services

Quickstart should fail if there is an instance of juju-gui in the
services, because that will lead to a name conflict when deploying the
bundle.

To test: `make check`

To QA: `.venv/bin/python juju-quickstart
https://gist.github.com/makyo/7435270/raw/6f0d5ff910dd1a82b97c7170915a196381b2b146/bundle.yaml`
should fail with a message explaining that juju-gui should be removed
from the bundle; other bundles will still succeed.

R=bac
CC=
https://codereview.appspot.com/25340043

https://codereview.appspot.com/25340043/

=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2013-11-12 15:34:15 +0000
+++ quickstart/tests/test_utils.py	2013-11-12 17:49:15 +0000
@@ -255,6 +255,17 @@
         with self.assert_value_error(expected):
             utils.parse_bundle(contents)
 
+    def test_yaml_gui_in_services(self):
+        # A ValueError is raised if the bundle contains juju-gui.
+        contents = yaml.safe_dump({
+            'mybundle': {'services': {'juju-gui': {}}},
+        })
+        expected = 'bundle mybundle contains an instance of juju-gui. ' \
+            'quickstart will install the latest version of the Juju GUI ' \
+            'automatically, please remove juju-gui from the bundle.'
+        with self.assert_value_error(expected):
+            utils.parse_bundle(contents)
+
     def test_success_no_name(self):
         # The function succeeds when an implicit bundle name is used.
         contents = yaml.safe_dump({

=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2013-11-12 15:34:15 +0000
+++ quickstart/utils.py	2013-11-12 17:49:15 +0000
@@ -162,8 +162,11 @@
     if not bundle_services:
         msg = 'bundle {} does not include any services'.format(bundle_name)
         raise ValueError(msg)
-    # XXX 2013-10-21 frankban:
-        # Handle the case when the GUI is included in the bundle.
+    if 'juju-gui' in bundle_services:
+        raise ValueError('bundle {} contains an instance of juju-gui. '
+                         'quickstart will install the latest version of the '
+                         'Juju GUI automatically, please remove juju-gui from '
+                         'the bundle.'.format(bundle_name))
     return bundle_name, bundle_services