Merge lp:~frankban/juju-quickstart/open-urls into lp:juju-quickstart

[Francesco Banconi]

Deploy a bundle from a HTTP(S) url.

The bundle argument can be a file path or
a http/https URL pointing to a bundle YAML.

Also get the URL of the last Juju GUI charm
revision from charmworld (API 2 for now).

Tests: `make check`.

QA:
I copied a bundle YAML over here:
http://dpaste.com/1435065/plain/
You should be able to deploy that bundle
by running the following (after juju switching to ec2 or similar):
`.venv/bin/python juju-quickstart http://dpaste.com/1435065/plain/`
Remember to destroy your ec2 environment...

https://codereview.appspot.com/19870043/

[Francesco Banconi (frankban)]

Reviewers: mp+193297_code.launchpad.net,

Message:
Please take a look.

Description:
Deploy a bundle from a HTTP(S) url.

The bundle argument can be a file path or
a http/https URL pointing to a bundle YAML.

Also get the URL of the last Juju GUI charm
revision from charmworld (API 2 for now).

Tests: `make check`.

QA:
I copied a bundle YAML over here:
http://dpaste.com/1435065/plain/
You should be able to deploy that bundle
by running the following (after juju switching to ec2 or similar):
`.venv/bin/python juju-quickstart http://dpaste.com/1435065/plain/`
Remember to destroy your ec2 environment...

https://code.launchpad.net/~frankban/juju-quickstart/open-urls/+merge/193297

(do not edit description out of merge proposal)

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

Affected files (+224, -35 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/tests/helpers.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

[Gary Poster (gary)]

Code LGTM with trivial update. Trying QA now.

https://codereview.appspot.com/19870043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/19870043/diff/1/quickstart/app.py#newcode126
quickstart/app.py:126: logging.warn(msg.format(err))
nice.

https://codereview.appspot.com/19870043/diff/1/quickstart/tests/test_app.py
File quickstart/tests/test_app.py (right):

https://codereview.appspot.com/19870043/diff/1/quickstart/tests/test_app.py#newcode263
quickstart/tests/test_app.py:263: with
self.patch_get_charm_url(side_effect=IOError('boo!')):
You scared me.

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

https://codereview.appspot.com/19870043/diff/1/quickstart/utils.py#newcode35
quickstart/utils.py:35: # Switch to charmworld API 3 when the 500 error
is fixed.
We had a fix and a deployment, and
http://manage.jujucharms.com/api/3/charm/precise/juju-gui works now
(yay!)

https://codereview.appspot.com/19870043/

[Gary Poster (gary)]

Go you and your bad 100% test coverage self.

qa good (though EC2 left a bunch of services in pending state and the
haproxy and daisy charms were DOA :-/ )

Thank you!

https://codereview.appspot.com/19870043/

[lp:~frankban/juju-quickstart/open-urls]

11. By Francesco Banconi on 2013-10-31

Changes as per review.

[Francesco Banconi (frankban)]

*** Submitted:

Deploy a bundle from a HTTP(S) url.

The bundle argument can be a file path or
a http/https URL pointing to a bundle YAML.

Also get the URL of the last Juju GUI charm
revision from charmworld (API 2 for now).

Tests: `make check`.

QA:
I copied a bundle YAML over here:
http://dpaste.com/1435065/plain/
You should be able to deploy that bundle
by running the following (after juju switching to ec2 or similar):
`.venv/bin/python juju-quickstart http://dpaste.com/1435065/plain/`
Remember to destroy your ec2 environment...

R=gary.poster
CC=
https://codereview.appspot.com/19870043

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

https://codereview.appspot.com/19870043/diff/1/quickstart/utils.py#newcode35
quickstart/utils.py:35: # Switch to charmworld API 3 when the 500 error
is fixed.
On 2013/10/30 20:58:55, gary.poster wrote:
> We had a fix and a deployment, and
> http://manage.jujucharms.com/api/3/charm/precise/juju-gui works now
(yay!)

Cool! Done.

https://codereview.appspot.com/19870043/

[Francesco Banconi (frankban)]

Thanks for the review Gary!

https://codereview.appspot.com/19870043/

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2013-10-30 10:16:36 +0000
+++ quickstart/app.py	2013-10-31 08:49:45 +0000
@@ -19,6 +19,7 @@
 from __future__ import print_function
 import functools
 import json
+import logging
 
 import jujuclient
 
@@ -28,6 +29,11 @@
 )
 
 
+# The default Juju GUI charm URL to use when it is not possible to retrieve it
+# from the charmworld API, e.g. due to temporary connection/charmworld errors.
+DEFAULT_CHARM_URL = 'cs:precise/juju-gui-78'
+
+
 class ProgramExit(Exception):
     """An error occurred while setting up the Juju environment.
 
@@ -113,10 +119,12 @@
 
     Raise a ProgramExit if the API server returns an error response.
     """
-    # XXX 2013-10-17 frankban:
-        # Retrieve the URL of the last charm revision from
-        # manage.jujucharms.com.
-    charm_url = 'cs:precise/juju-gui-78'
+    try:
+        charm_url = utils.get_charm_url()
+    except (IOError, ValueError) as err:
+        msg = 'unable to retrieve the Juju GUI charm URL from the API: {}'
+        logging.warn(msg.format(err))
+        charm_url = DEFAULT_CHARM_URL
     try:
         env.deploy(service_name, charm_url, to=0)
         env.expose(service_name)
 
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2013-10-30 10:16:36 +0000
+++ quickstart/manage.py	2013-10-31 08:49:45 +0000
@@ -46,21 +46,26 @@
     """Validate and process the bundle options.
 
     Populate the options namespace with the following names:
-        - bundle_file: the full path to the bundle file;
         - bundle_name: the name of the bundle;
         - bundle_services: a list of service names included in the bundle;
         - bundle_yaml: the YAML encoded contents of the bundle.
 
     Exit with an error if the bundle options are not valid.
     """
-    # XXX 2013-10-18 frankban:
-        # This function is supposed to also support bundle URLs.
-    bundle_file = os.path.abspath(os.path.expanduser(options.bundle))
-    # Load the bundle file.
-    try:
-        bundle_yaml = open(bundle_file).read()
-    except IOError as err:
-        return parser.error('unable to open bundle file: {}'.format(err))
+    bundle = options.bundle
+    if bundle.startswith('http://') or bundle.startswith('https://'):
+        # Load the bundle URL.
+        try:
+            bundle_yaml = utils.urlread(bundle)
+        except IOError as err:
+            return parser.error('unable to open bundle URL: {}'.format(err))
+    else:
+        # Load the bundle file.
+        bundle_file = os.path.abspath(os.path.expanduser(bundle))
+        try:
+            bundle_yaml = open(bundle_file).read()
+        except IOError as err:
+            return parser.error('unable to open bundle file: {}'.format(err))
     # Validate the bundle.
     try:
         bundle_name, bundle_services = utils.parse_bundle(
@@ -68,7 +73,6 @@
     except ValueError as err:
         return parser.error(str(err))
     # Update the options namespace with the new values.
-    options.bundle_file = bundle_file
     options.bundle_name = bundle_name
     options.bundle_services = bundle_services
     options.bundle_yaml = bundle_yaml
@@ -140,11 +144,9 @@
         env_help = '{} (%(default)s)'.format(env_help)
     # Create and set up the arguments parser.
     parser = argparse.ArgumentParser(description=quickstart.__doc__)
-    # XXX 2013-10-18 frankban:
-        # Make it possible to pass a URL as bundle argument.
     parser.add_argument(
         'bundle', default=None, nargs='?',
-        help='The path to the bundle file to deploy')
+        help='The bundle URL or the path to the bundle file to deploy')
     parser.add_argument(
         '-e', '--environment', default=default_env_name, dest='env_name',
         help=env_help)
 
=== modified file 'quickstart/tests/helpers.py'
--- quickstart/tests/helpers.py	2013-10-30 10:16:36 +0000
+++ quickstart/tests/helpers.py	2013-10-31 08:49:45 +0000
@@ -100,6 +100,23 @@
         return env_file.name
 
 
+class UrlReadTestsMixin(object):
+    """Expose a method to mock the quickstart.utils.urlread helper function."""
+
+    def patch_urlread(self, contents=None, error=False):
+        """Patch the quickstart.utils.urlread helper function.
+
+        If contents is not None, urlread() will return the provided contents.
+        If error is set to True, an IOError will be simulated.
+        """
+        mock_urlread = mock.Mock()
+        if contents is not None:
+            mock_urlread.return_value = contents
+        if error:
+            mock_urlread.side_effect = IOError('bad wolf')
+        return mock.patch('quickstart.utils.urlread', mock_urlread)
+
+
 class ValueErrorTestsMixin(object):
     """Set up some base methods for testing functions raising ValueErrors."""
 
 
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2013-10-30 10:16:36 +0000
+++ quickstart/tests/test_app.py	2013-10-31 08:49:45 +0000
@@ -235,12 +235,37 @@
 
 class TestDeployGui(ProgramExitTestsMixin, unittest.TestCase):
 
+    charm_url = 'cs:precise/juju-gui-42'
+
+    def patch_get_charm_url(self, side_effect=None):
+        """Patch the get_charm_url helper function."""
+        if side_effect is None:
+            side_effect = [self.charm_url]
+        mock_get_charm_url = mock.Mock(side_effect=side_effect)
+        return mock.patch('quickstart.utils.get_charm_url', mock_get_charm_url)
+
     def test_deployment(self):
-        # The function correctly deploys and exposes the service.
-        env = mock.Mock()
-        app.deploy_gui(env, 'my-gui')
-        env.assert_has_calls([
-            mock.call.deploy('my-gui', 'cs:precise/juju-gui-78', to=0),
+        # The function correctly deploys and exposes the service, retrieving
+        # the charm URL from the charmworld API.
+        env = mock.Mock()
+        with self.patch_get_charm_url():
+            app.deploy_gui(env, 'my-gui')
+        env.assert_has_calls([
+            mock.call.deploy('my-gui', 'cs:precise/juju-gui-42', to=0),
+            mock.call.expose('my-gui')
+        ])
+
+    def test_deployment_default_charm_url(self):
+        # The function correctly deploys and exposes the service, even if it is
+        # not able to retrieve the charm URL from the charmworld API.
+        env = mock.Mock()
+        log = 'unable to retrieve the Juju GUI charm URL from the API: boo!'
+        with self.patch_get_charm_url(side_effect=IOError('boo!')):
+            # A warning is logged which notifies we are using the default URL.
+            with helpers.assert_logs([log], level='warn'):
+                app.deploy_gui(env, 'my-gui')
+        env.assert_has_calls([
+            mock.call.deploy('my-gui', app.DEFAULT_CHARM_URL, to=0),
             mock.call.expose('my-gui')
         ])
 
@@ -249,20 +274,22 @@
         env = mock.Mock()
         env.deploy.side_effect = self.make_env_error('service already exists')
         expected = 'bad API server response: service already exists'
-        with self.assert_program_exit(expected):
-            app.deploy_gui(env, 'another-gui')
+        with self.patch_get_charm_url():
+            with self.assert_program_exit(expected):
+                app.deploy_gui(env, 'another-gui')
         env.deploy.assert_called_once_with(
-            'another-gui', 'cs:precise/juju-gui-78', to=0)
+            'another-gui', 'cs:precise/juju-gui-42', to=0)
 
     def test_other_errors(self):
         # Any other errors occurred during the process are not trapped.
         error = ValueError('explode!')
         env = mock.Mock()
         env.expose.side_effect = error
-        with self.assertRaises(ValueError) as context_manager:
-            app.deploy_gui(env, 'juju-gui')
+        with self.patch_get_charm_url():
+            with self.assertRaises(ValueError) as context_manager:
+                app.deploy_gui(env, 'juju-gui')
         env.deploy.assert_called_once_with(
-            'juju-gui', 'cs:precise/juju-gui-78', to=0)
+            'juju-gui', 'cs:precise/juju-gui-42', to=0)
         env.expose.assert_called_once_with('juju-gui')
         self.assertIs(error, context_manager.exception)
 
 
=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2013-10-30 10:16:36 +0000
+++ quickstart/tests/test_manage.py	2013-10-31 08:49:45 +0000
@@ -46,7 +46,9 @@
         mock_exit.assert_called_once_with(0)
 
 
-class TestValidateBundle(helpers.BundleFileTestsMixin, unittest.TestCase):
+class TestValidateBundle(
+        helpers.BundleFileTestsMixin, helpers.UrlReadTestsMixin,
+        unittest.TestCase):
 
     def setUp(self):
         self.parser = mock.Mock()
@@ -55,12 +57,24 @@
         """Return a mock options object which includes the passed arguments."""
         return mock.Mock(bundle=bundle, bundle_name=bundle_name)
 
-    def test_resulting_options(self):
-        # The options object is correctly set up.
+    def test_resulting_options_from_file(self):
+        # The options object is correctly set up when a bundle file is passed.
         bundle_file = self.make_bundle_file()
         options = self.make_options(bundle_file, bundle_name='bundle1')
         manage._validate_bundle(options, self.parser)
-        self.assertEqual(bundle_file, options.bundle_file)
+        self.assertEqual('bundle1', options.bundle_name)
+        self.assertEqual(
+            ['mysql', 'wordpress'], sorted(options.bundle_services))
+        self.assertEqual(open(bundle_file).read(), options.bundle_yaml)
+
+    def test_resulting_options_from_url(self):
+        # The options object is correctly set up when a bundle URL is passed.
+        bundle_file = self.make_bundle_file()
+        url = 'http://example.com/bundle.yaml'
+        options = self.make_options(url, bundle_name='bundle1')
+        with self.patch_urlread(contents=self.valid_bundle) as mock_urlread:
+            manage._validate_bundle(options, self.parser)
+        mock_urlread.assert_called_once_with(url)
         self.assertEqual('bundle1', options.bundle_name)
         self.assertEqual(
             ['mysql', 'wordpress'], sorted(options.bundle_services))
@@ -78,7 +92,7 @@
         options = self.make_options(bundle=path, bundle_name='bundle2')
         with mock.patch('os.environ', {'HOME': base_path}):
             manage._validate_bundle(options, self.parser)
-        self.assertEqual(bundle_file, options.bundle_file)
+        self.assertEqual(self.valid_bundle, options.bundle_yaml)
 
     def test_bundle_file_not_found(self):
         # A parser error is invoked if the bundle file is not found.
@@ -90,8 +104,19 @@
         )
         self.parser.error.assert_called_once_with(expected)
 
-    def test_error_parsing_bundle_file(self):
-        # A parser error is invoked if an error occurs parsing the bundle file.
+    def test_url_error(self):
+        # A parser error is invoked if the bundle cannot be fetched from the
+        # provided URL.
+        url = 'http://example.com/bundle.yaml'
+        options = self.make_options(url)
+        with self.patch_urlread(error=True) as mock_urlread:
+            manage._validate_bundle(options, self.parser)
+        mock_urlread.assert_called_once_with(url)
+        self.parser.error.assert_called_once_with(
+            'unable to open bundle URL: bad wolf')
+
+    def test_error_parsing_bundle_contents(self):
+        # A parser error is invoked if an error occurs parsing the bundle YAML.
         bundle_file = self.make_bundle_file()
         options = self.make_options(bundle_file, bundle_name='no-such')
         manage._validate_bundle(options, self.parser)
 
=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2013-10-30 10:16:36 +0000
+++ quickstart/tests/test_utils.py	2013-10-31 08:49:45 +0000
@@ -16,7 +16,11 @@
 
 """Tests for the Juju Quickstart utility functions and classes."""
 
+import httplib
+import json
+import socket
 import unittest
+import urllib2
 
 import mock
 import yaml
@@ -71,6 +75,35 @@
             utils.call('echo', 'we are the borg!')
 
 
+class TestGetCharmUrl(helpers.UrlReadTestsMixin, unittest.TestCase):
+
+    def test_charm_url(self):
+        # The Juju GUI charm URL is correctly returned.
+        contents = json.dumps({'charm': {'url': 'cs:precise/juju-gui-42'}})
+        with self.patch_urlread(contents=contents) as mock_urlread:
+            charm_url = utils.get_charm_url()
+        self.assertEqual('cs:precise/juju-gui-42', charm_url)
+        mock_urlread.assert_called_once_with(utils.CHARMWORLD_API)
+
+    def test_io_error(self):
+        # IOErrors are properly propagated.
+        with self.patch_urlread(error=True) as mock_urlread:
+            with self.assertRaises(IOError) as context_manager:
+                utils.get_charm_url()
+        mock_urlread.assert_called_once_with(utils.CHARMWORLD_API)
+        self.assertEqual('bad wolf', str(context_manager.exception))
+
+    def test_value_error(self):
+        # A ValueError is raised if the API response is not valid.
+        contents = json.dumps({'charm': {}})
+        with self.patch_urlread(contents=contents) as mock_urlread:
+            with self.assertRaises(ValueError) as context_manager:
+                utils.get_charm_url()
+        mock_urlread.assert_called_once_with(utils.CHARMWORLD_API)
+        self.assertEqual(
+            'unable to find the charm URL', str(context_manager.exception))
+
+
 class TestGetDefaultEnvName(helpers.CallTestsMixin, unittest.TestCase):
 
     def test_environment_variable(self):
@@ -354,6 +387,46 @@
             utils.unit_changes(self.unit, changeset))
 
 
+class TestUrlread(unittest.TestCase):
+
+    def patch_urlopen(self, contents=None, error=None):
+        """Patch the urllib2.urlopen function.
+
+        If contents is not None, the read() method of the returned mock object
+        returns the given contents.
+        If an error is provided, the call raises the error.
+        """
+        mock_urlopen = mock.Mock()
+        if contents is not None:
+            mock_urlopen().read.return_value = contents
+        if error is not None:
+            mock_urlopen.side_effect = error
+        mock_urlopen.reset_mock()
+        return mock.patch('urllib2.urlopen', mock_urlopen)
+
+    def test_contents(self):
+        # The URL contents are correctly returned.
+        with self.patch_urlopen(contents='URL contents') as mock_urlopen:
+            contents = utils.urlread('http://example.com/path/')
+        self.assertEqual('URL contents', contents)
+        mock_urlopen.assert_called_once_with('http://example.com/path/')
+
+    def test_errors(self):
+        # An IOError is raised if an error occurs connecting to the API.
+        errors = {
+            'httplib HTTPException': httplib.HTTPException,
+            'socket error': socket.error,
+            'urllib2 URLError': urllib2.URLError,
+        }
+        for message, exception_class in errors.items():
+            exception = exception_class(message)
+            with self.patch_urlopen(error=exception) as mock_urlopen:
+                with self.assertRaises(IOError) as context_manager:
+                    utils.urlread('http://example.com/path/')
+            mock_urlopen.assert_called_once_with('http://example.com/path/')
+            self.assertEqual(message, str(context_manager.exception))
+
+
 class TestUtf8(unittest.TestCase):
 
     def test_unicode(self):
 
=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2013-10-30 10:16:36 +0000
+++ quickstart/utils.py	2013-10-31 08:49:45 +0000
@@ -17,16 +17,22 @@
 """Juju Quickstart utility functions and classes."""
 
 import collections
+import httplib
+import json
 import logging
 import os
 import pipes
 import re
+import socket
 import subprocess
+import urllib2
 
 import yaml
 
 # Compile the regular expression used to parse the "juju switch" output.
 _juju_switch_expression = re.compile(r'Current environment: "([\w-]+)"\n')
+# Define the URL containing information about the last Juju GUI charm version.
+CHARMWORLD_API = 'http://manage.jujucharms.com/api/3/charm/precise/juju-gui'
 
 
 def call(*args):
@@ -52,6 +58,19 @@
     return retcode, output, error
 
 
+def get_charm_url():
+    """Return the charm URL of the latest Juju GUI charm revision.
+
+    Raise an IOError if any problems occur connecting to the API endpoint.
+    Raise a ValueError if the API returns invalid data.
+    """
+    charm_info = json.loads(urlread(CHARMWORLD_API))
+    charm_url = charm_info.get('charm', {}).get('url')
+    if charm_url is None:
+        raise ValueError('unable to find the charm URL')
+    return charm_url
+
+
 def get_default_env_name():
     """Return the current Juju environment name.
 
@@ -212,6 +231,20 @@
             return change
 
 
+def urlread(url):
+    """Open the given URL and return the page contents.
+
+    Raise an IOError if any problems occur.
+    """
+    try:
+        response = urllib2.urlopen(url)
+    except urllib2.URLError as err:
+        raise IOError(err.reason)
+    except (httplib.HTTPException, socket.error, urllib2.HTTPError) as err:
+        raise IOError(str(err))
+    return response.read()
+
+
 def utf8(value):
     """Return the utf8 encoded version of the given value.