Merge lp:~frankban/juju-quickstart/code-reorg into lp:juju-quickstart

[Francesco Banconi]

Code reorganization + docs.

Reorganize the jenv related code in preparation of the
bootstrap strategy change.

Separate jenv handling from the environments.yaml file
management in models.

As a consequence of only supporting juju >= 1.18 we
can now safely always retrieve the admin-secret from
the jenv file.

Also updated the HACKING docs to include a brief
description of the project structure.

Tests: `make check`.

QA: use quickstart as usual.

https://codereview.appspot.com/169380043/

[Francesco Banconi (frankban)]

Reviewers: mp+241416_code.launchpad.net,

Message:
Please take a look.

Description:
Code reorganization + docs.

Reorganize the jenv related code in preparation of the
bootstrap strategy change.

Separate jenv handling from the environments.yaml file
management in models.

As a consequence of only supporting juju >= 1.18 we
can now safely always retrieve the admin-secret from
the jenv file.

Also updated the HACKING docs to include a brief
description of the project structure.

Tests: `make check`.

QA: use quickstart as usual.

https://code.launchpad.net/~frankban/juju-quickstart/code-reorg/+merge/241416

(do not edit description out of merge proposal)

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

Affected files (+372, -222 lines):
   M HACKING.rst
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/models/envs.py
   A quickstart/models/jenv.py
   M quickstart/serializers.py
   M quickstart/tests/helpers.py
   M quickstart/tests/models/test_envs.py
   A quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_serializers.py

[Brad Crittenden (bac)]

LGTM. Will QA now.

https://codereview.appspot.com/169380043/diff/1/HACKING.rst
File HACKING.rst (right):

https://codereview.appspot.com/169380043/diff/1/HACKING.rst#newcode78
HACKING.rst:78:
A very nice introduction to the code layout. Thanks for including it.

https://codereview.appspot.com/169380043/diff/1/quickstart/models/jenv.py
File quickstart/models/jenv.py (right):

https://codereview.appspot.com/169380043/diff/1/quickstart/models/jenv.py#newcode39
quickstart/models/jenv.py:39: def get_value(env_name, *args):
Why did you chose to use *args here instead of a tuple of key names?

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

https://codereview.appspot.com/169380043/diff/1/quickstart/tests/helpers.py#newcode156
quickstart/tests/helpers.py:156: In the contentx manager block, the
JUJU_HOME is set to the ancestor
typo: contextx

https://codereview.appspot.com/169380043/

[Richard Harding (rharding)]

Thanks for the update. I just have one forward looking question that
doesn't directly effect this branch but I'm curious about while looking
at this branch.

LGTM, no qa

https://codereview.appspot.com/169380043/diff/1/HACKING.rst
File HACKING.rst (right):

https://codereview.appspot.com/169380043/diff/1/HACKING.rst#newcode128
HACKING.rst:128: the code: go have a look!
nice ty

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

https://codereview.appspot.com/169380043/diff/1/quickstart/app.py#newcode263
quickstart/app.py:263: Parse the jenv file to retrieve the admin secret.
do you know if we can use the password field in the jenv for this? Next
week we hope to start updating the gui for the new username/password
work in juju 1.21 and I know we'll need to update quickstart to pull the
username/password from the jenv file.

https://codereview.appspot.com/169380043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (left):

https://codereview.appspot.com/169380043/diff/1/quickstart/models/envs.py#oldcode202
quickstart/models/envs.py:202: def load_generated(env_file,
section='bootstrap-config'):
was this unused?

https://codereview.appspot.com/169380043/

[Brad Crittenden (bac)]

Had some QA oddities but it looks OK.

https://codereview.appspot.com/169380043/

[Francesco Banconi (frankban)]

Thanks for the reviews!

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

https://codereview.appspot.com/169380043/diff/1/quickstart/app.py#newcode263
quickstart/app.py:263: Parse the jenv file to retrieve the admin secret.
On 2014/11/11 15:52:36, rharding wrote:
> do you know if we can use the password field in the jenv for this?
Next week we
> hope to start updating the gui for the new username/password work in
juju 1.21
> and I know we'll need to update quickstart to pull the
username/password from
> the jenv file.

With this change it should be easy to grab what we want from the jenv
with jenv.get_value(). We will want to use some fallback logic to
support backward compatibility.
If the user is != user-admin, we will need to update the one-show
automatic authentication with token as well in the guiserver and maybe
in the GUI itself.

https://codereview.appspot.com/169380043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (left):

https://codereview.appspot.com/169380043/diff/1/quickstart/models/envs.py#oldcode202
quickstart/models/envs.py:202: def load_generated(env_file,
section='bootstrap-config'):
On 2014/11/11 15:52:36, rharding wrote:
> was this unused?

It was used, it is no longer required now.

https://codereview.appspot.com/169380043/diff/1/quickstart/models/jenv.py
File quickstart/models/jenv.py (right):

https://codereview.appspot.com/169380043/diff/1/quickstart/models/jenv.py#newcode39
quickstart/models/jenv.py:39: def get_value(env_name, *args):
On 2014/11/11 15:37:55, bac wrote:
> Why did you chose to use *args here instead of a tuple of key names?

No real reason actually, I started with just one single key as the
argument and then, after deciding to allow nested keys, it just felt
natural.

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

https://codereview.appspot.com/169380043/diff/1/quickstart/tests/helpers.py#newcode156
quickstart/tests/helpers.py:156: In the contentx manager block, the
JUJU_HOME is set to the ancestor
On 2014/11/11 15:37:55, bac wrote:
> typo: contextx

Done.

https://codereview.appspot.com/169380043/

[Francesco Banconi (frankban)]

*** Submitted:

Code reorganization + docs.

Reorganize the jenv related code in preparation of the
bootstrap strategy change.

Separate jenv handling from the environments.yaml file
management in models.

As a consequence of only supporting juju >= 1.18 we
can now safely always retrieve the admin-secret from
the jenv file.

Also updated the HACKING docs to include a brief
description of the project structure.

Tests: `make check`.

QA: use quickstart as usual.

R=bac, rharding
CC=
https://codereview.appspot.com/169380043

https://codereview.appspot.com/169380043/

=== modified file 'HACKING.rst'
--- HACKING.rst	2014-09-01 14:49:47 +0000
+++ HACKING.rst	2014-11-11 14:26:05 +0000
@@ -73,6 +73,60 @@
 
     $ .venv/bin/python juju-quickstart --help
 
+Project structure
+~~~~~~~~~~~~~~~~~
+
+Juju Quickstart is a Python project. Source files can be found in the
+``quickstart`` Python package included in this distribution.
+
+A brief  description of the project structure follows, including the goals of
+each module in the ``quickstart`` package.
+
+* ``manage.py`` includes the main entry points for the ``juju-quickstart``
+  command. Specifically:
+  * ``manage.setup`` is used to set up the command line parser, the logging
+    infrastructure and the interactive session (if required);
+  * ``manage.run`` executes the application with the given options.
+
+* ``app.py`` defines the main application functions, like bootstrapping an
+  environment, deploying the Juju GUI or watching the deployment progress.
+  The ``app.ProgramExit`` error can only be raised by functions in this module,
+  and it causes the immediate exit (with an error) from the application.
+
+The ``manage.py`` and ``app.py`` modules must be considered ``juju-quickstart``
+execution specific: for this reason those modules are unlikely to be used as
+libraries. All the other packages/modules in the application (except for views,
+see below), should only include library-like code that can be reused in
+different places by either Juju Quickstart or external programs.
+
+* ``setting.py`` includes the application settings. Those must be considered as
+  constant values to be reused throughout the application. The settings module
+  should not import other ``quickstart`` modules.
+
+* ``utils.py`` defines general purpose helper functions: when writing such
+  utility objects it is likely that this is the right place where they should
+  live. Separate modules are created when a set of utilities are related each
+  other: this is the case, for instance, of ``serializers.py`` (YAML/JSON
+  serialization utilities), ``ssh.py`` (SSH protocol related functions),
+  ``platform_support.py`` etc.
+
+* ``juju.py`` defines the WebSocket API client used by Juju Quickstart to
+  connect to the Juju API server.
+
+* the ``models`` package includes a module for each application model. Models
+  are abstractions representing the data and information managed by Juju
+  Quickstart, e.g. environment files, jenv files or charms. In the
+  implementation of models, an effort has been made to use simple data
+  structures in order to represent entities/objects, and composable functions
+  to manipulate this information.
+
+* the ``cli`` package contains the command line interactive session handling.
+  Juju Quickstart uses Urwid to implement an ncurses-like interactive session:
+  Urwid code must only live in the ``cli`` package.
+
+That said, module docstrings often describe the goals and usage of each part of
+the code: go have a look!
+
 Pre-release QA
 ~~~~~~~~~~~~~~
 
@@ -98,7 +152,7 @@
 
 * Verify an environment that has already been bootstrapped is recogized and
   the GUI is deployed.  This test also shows that a remote bundle is properly
-  deployed.::
+  deployed::
 
     juju bootstrap -e local
     juju quickstart -e local bundle:mediawiki/single

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-11-10 11:39:40 +0000
+++ quickstart/app.py	2014-11-11 14:26:05 +0000
@@ -23,7 +23,6 @@
 
 import json
 import logging
-import os
 import sys
 import time
 
@@ -37,7 +36,7 @@
     utils,
     watchers,
 )
-from quickstart.models import envs
+from quickstart.models import jenv
 
 
 class ProgramExit(Exception):
@@ -258,27 +257,17 @@
     raise ProgramExit('the state server is not ready:\n{}'.format(details))
 
 
-def get_value_from_jenv(env_name, juju_home, key):
-    """Read the key from the generated environment file.
-
-    At bootstrap, juju (v1.16 and later) writes a generated file
-    located in JUJU_HOME.  Return the value for the key.
-
-    Raise a ValueError if:
-        - the environment file is not found;
-        - the environment file contents are not parsable by YAML;
-        - the YAML contents are not properly structured;
-        - the key is not found.
+def get_admin_secret(env_name):
+    """Return the Juju admin secret for the given environment name.
+
+    Parse the jenv file to retrieve the admin secret.
+    Raise a ProgramExit if the admin secret cannot be retrieved.
     """
-    filename = '{}.jenv'.format(env_name)
-    juju_env_file = os.path.expanduser(
-        os.path.join(juju_home, 'environments', filename))
-    jenv_db = envs.load_generated(juju_env_file)
     try:
-        return jenv_db[key]
-    except KeyError:
-        msg = '{} not found in {}'.format(key, juju_env_file)
-        raise ValueError(msg.encode('utf-8'))
+        return jenv.get_value(env_name, 'bootstrap-config', 'admin-secret')
+    except ValueError as err:
+        msg = b'cannot retrieve environment admin-secret: {}'.format(err)
+        raise ProgramExit(msg)
 
 
 def get_api_url(env_name, juju_command):

=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2014-11-10 11:31:02 +0000
+++ quickstart/manage.py	2014-11-11 14:26:05 +0000
@@ -293,7 +293,6 @@
             'this host platform does not support local environments'
         )
     # Update the options namespace with the new values.
-    options.admin_secret = env_data.get('admin-secret')
     options.env_file = env_file
     options.env_name = env_data['name']
     options.default_series = env_data.get('default-series')
@@ -343,10 +342,9 @@
     """Set up the application options and logger.
 
     Return the options as a namespace containing the following attributes:
-        - admin_secret: the password to use to access the Juju API or None if
-          no admin-secret is present in the $JUJU_HOME/environment.yaml file;
         - bundle: the optional bundle (path or URL) to be deployed;
         - charm_url: the Juju GUI charm URL or None if not specified;
+        - constraints: the environment constrains or None if not set;
         - debug: whether debug mode is activated;
         - distro_only: install Juju only using the distribution packages;
         - env_file: the absolute path of the Juju environments.yaml file;
@@ -357,8 +355,7 @@
         - platform: The host platform;
         - upload_tools: whether to upload local version of tools;
         - upload_series: the comma-separated series list for which tools will
-          be uploaded, or None if not set;
-        - constraints: the environment constrains or None if not set.
+          be uploaded, or None if not set.
 
     The following attributes will also be included in the namespace if a bundle
     deployment is requested:
@@ -522,16 +519,7 @@
         constraints=options.constraints)
 
     # Retrieve the admin-secret for the current environment.
-    try:
-        admin_secret = app.get_value_from_jenv(
-            options.env_name, settings.JUJU_HOME, 'admin-secret')
-    except ValueError as err:
-        admin_secret = options.admin_secret
-        if admin_secret is None:
-            # The admin-secret cannot be found in the jenv file and is not
-            # explicitly specified in the environments.yaml file.
-            msg = b'{} or {}'.format(err, options.env_file.encode('utf-8'))
-            raise app.ProgramExit(msg)
+    admin_secret = app.get_admin_secret(options.env_name)
 
     print('retrieving the Juju API address')
     api_url = app.get_api_url(options.env_name, juju_command)

=== modified file 'quickstart/models/envs.py'
--- quickstart/models/envs.py	2014-10-09 13:14:30 +0000
+++ quickstart/models/envs.py	2014-11-11 14:26:05 +0000
@@ -134,23 +134,6 @@
     return {'environments': {}}
 
 
-def _load_file(env_file):
-    """Load the given file and return the YAML-parsed contents."""
-    # Load the Juju environments file.
-    try:
-        environments_file = open(env_file.encode('utf-8'))
-    except IOError as err:
-        msg = b'unable to open environments file: {}'.format(err)
-        raise ValueError(msg)
-    # Parse the Juju environments file.
-    try:
-        contents = serializers.yaml_load(environments_file)
-    except Exception as err:
-        msg = b'unable to parse environments file {}: {}'
-        raise ValueError(msg.format(env_file.encode('utf-8'), err))
-    return contents
-
-
 def load(env_file):
     """Load and parse the provided Juju environments.yaml file.
 
@@ -174,7 +157,7 @@
         - the environment file contents are not parsable by YAML;
         - the YAML contents are not properly structured.
     """
-    contents = _load_file(env_file)
+    contents = serializers.yaml_load_from_path(env_file)
     if contents is None:
         return create_empty_env_db()
     # Retrieve the environment list.
@@ -199,18 +182,6 @@
     return env_db
 
 
-def load_generated(env_file, section='bootstrap-config'):
-    """Given the path to a YAML file, load the file and return the section."""
-    contents = _load_file(env_file)
-    try:
-        section_contents = contents[section]
-    except (KeyError, TypeError):
-        msg = 'invalid YAML contents in {}: {}'.format(env_file, contents)
-        raise ValueError(msg.encode('utf-8'))
-
-    return section_contents
-
-
 def save(env_file, env_db, backup_function=None):
     """Save the given env_db to the provided environments.yaml file.
 

=== added file 'quickstart/models/jenv.py'
--- quickstart/models/jenv.py	1970-01-01 00:00:00 +0000
+++ quickstart/models/jenv.py	2014-11-11 14:26:05 +0000
@@ -0,0 +1,71 @@
+# This file is part of the Juju Quickstart Plugin, which lets users set up a
+# Juju environment in very few steps (https://launchpad.net/juju-quickstart).
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License version 3, as published by
+# the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Juju Quickstart jenv generated files handling.
+
+At bootstrap, Juju writes a generated file (jenv) located in JUJU_HOME.
+This module includes functions to load and parse those jenv file contents.
+"""
+
+from __future__ import unicode_literals
+
+import os
+
+from quickstart import (
+    serializers,
+    settings,
+)
+
+
+def exists(env_name):
+    """Report whether the jenv generated file exists for the given env_name."""
+    jenv_path = _get_jenv_path(env_name)
+    return os.path.isfile(jenv_path)
+
+
+def get_value(env_name, *args):
+    """Read a value from the Juju generated environment file (jenv).
+
+    Return the value corresponding to the section specified in args.
+    For instance, calling get_value('ec2', 'bootstrap-config', 'admin-secret')
+    returns the value associated with the "admin-secret" key included on the
+    "bootstrap-config" section of the jenv file.
+
+    Raise a ValueError if:
+        - the environment file is not found;
+        - the environment file contents are not parsable by YAML;
+        - the YAML contents are not properly structured;
+        - one the keys in args is not found.
+    """
+    jenv_path = _get_jenv_path(env_name)
+    data = serializers.yaml_load_from_path(jenv_path)
+    section = 'root'
+    for key in args:
+        try:
+            data = data[key]
+        except (KeyError, TypeError):
+            msg = ('invalid YAML contents in {}: ''{} key not found in the {} '
+                   'section'.format(jenv_path, key, section))
+            raise ValueError(msg.encode('utf-8'))
+        section = key
+    return data
+
+
+def _get_jenv_path(env_name):
+    """Return the path to the generated jenv file for the given env_name."""
+    filename = '{}.jenv'.format(env_name)
+    path = os.path.join(settings.JUJU_HOME, 'environments', filename)
+    return os.path.expanduser(path)

=== modified file 'quickstart/serializers.py'
--- quickstart/serializers.py	2013-12-06 17:17:19 +0000
+++ quickstart/serializers.py	2014-11-11 14:26:05 +0000
@@ -45,3 +45,23 @@
     Always serialize collections in the block style.
     """
     return yaml.safe_dump(data, stream, default_flow_style=False)
+
+
+def yaml_load_from_path(path):
+    """Load the given file path and return the YAML-parsed contents.
+
+    Raise a ValueError if the file cannot be opened or parsed.
+    """
+    filename = path.encode('utf-8')
+    try:
+        yaml_file = open(filename)
+    except IOError as err:
+        msg = b'unable to open file {}: {}'.format(filename, err)
+        raise ValueError(msg)
+    # Parse the Juju environments file.
+    try:
+        contents = yaml_load(yaml_file)
+    except Exception as err:
+        msg = b'unable to parse file {}: {}'.format(filename, err)
+        raise ValueError(msg)
+    return contents

=== modified file 'quickstart/tests/helpers.py'
--- quickstart/tests/helpers.py	2014-11-10 09:08:44 +0000
+++ quickstart/tests/helpers.py	2014-11-11 14:26:05 +0000
@@ -136,6 +136,42 @@
         return env_file.name
 
 
+class JenvFileTestsMixin(object):
+    """Shared methods for testing Juju generated environment files (jenv)."""
+
+    jenv_data = {
+        'user': 'admin',
+        'state-servers': ['localhost:17070', '10.0.3.1:17070'],
+        'bootstrap-config': {
+            'admin-secret': 'Secret!',
+            'api-port': 17070,
+        },
+        'life': {'universe': {'everything': 42}},
+    }
+
+    @contextmanager
+    def make_jenv(self, env_name, contents):
+        """Create a temporary jenv file with the given env_name and contents.
+
+        In the contentx manager block, the JUJU_HOME is set to the ancestor
+        of the generated temporary file.
+
+        Return the jenv file path.
+        """
+        # Create a temporary JUJU_HOME.
+        playground = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, playground)
+        environments_dir = os.path.join(playground, 'environments')
+        os.mkdir(environments_dir)
+        # Create the jenv file inside the temporary JUJU_HOME.
+        jenv_path = os.path.join(environments_dir, '{}.jenv'.format(env_name))
+        with open(jenv_path, 'w') as jenv_file:
+            jenv_file.write(contents)
+        # Patch the JUJU_HOME and return the jenv file path.
+        with mock.patch('quickstart.settings.JUJU_HOME', playground):
+            yield jenv_path
+
+
 def make_env_db(default=None, exclude_invalid=False):
     """Create and return an env_db.
 

=== modified file 'quickstart/tests/models/test_envs.py'
--- quickstart/tests/models/test_envs.py	2014-10-09 15:29:46 +0000
+++ quickstart/tests/models/test_envs.py	2014-11-11 14:26:05 +0000
@@ -99,28 +99,6 @@
         self.assertEqual({'environments': {}}, env_db)
 
 
-class TestLoadFile(
-        helpers.EnvFileTestsMixin, helpers.ValueErrorTestsMixin,
-        unittest.TestCase):
-
-    def test_no_file(self):
-        # A ValueError is raised if the environments file is not found.
-        expected = (
-            "unable to open environments file: "
-            "[Errno 2] No such file or directory: '/no/such/file.yaml'"
-        )
-        with self.assert_value_error(expected):
-            envs._load_file('/no/such/file.yaml')
-
-    def test_invalid_yaml(self):
-        # A ValueError is raised if the environments file is not a valid YAML.
-        env_file = self.make_env_file(':')
-        with self.assertRaises(ValueError) as context_manager:
-            envs._load_file(env_file)
-        expected = 'unable to parse environments file {}'.format(env_file)
-        self.assertIn(expected, bytes(context_manager.exception))
-
-
 class TestLoad(
         helpers.EnvFileTestsMixin, helpers.ValueErrorTestsMixin,
         unittest.TestCase):
@@ -205,60 +183,6 @@
         self.assertEqual(expected, env_db)
 
 
-class TestLoadGenerated(
-        helpers.EnvFileTestsMixin, helpers.ValueErrorTestsMixin,
-        unittest.TestCase):
-
-    def test_empty_file(self):
-        # A ValueError is raised if the environments file is empty.
-        env_file = self.make_env_file('')
-        expected = 'invalid YAML contents in {}: None'.format(env_file)
-        with self.assert_value_error(expected):
-            envs.load_generated(env_file)
-
-    def test_invalid_yaml_contents(self):
-        # A ValueError is raised if the environments file is not well formed.
-        env_file = self.make_env_file('a-string')
-        expected = 'invalid YAML contents in {}: a-string'.format(env_file)
-        with self.assert_value_error(expected):
-            envs.load_generated(env_file)
-
-    def test_section_not_found(self):
-        expected = {
-            'shoehorn-config': {
-                'admin-secret': 'Secret!',
-                'type': 'ec2'},
-        }
-        yaml_contents = expected.copy()
-        env_file = self.make_env_file(yaml.safe_dump(yaml_contents))
-        expected = 'invalid YAML contents in {}: {}'.format(
-            env_file, yaml_contents)
-        with self.assert_value_error(expected):
-            envs.load_generated(env_file)
-
-    def test_successful_default_section(self):
-        expected = {
-            'bootstrap-config': {
-                'admin-secret': 'Secret!',
-                'type': 'ec2'},
-        }
-        yaml_contents = expected.copy()
-        env_file = self.make_env_file(yaml.safe_dump(yaml_contents))
-        env_db = envs.load_generated(env_file)
-        self.assertEqual(expected['bootstrap-config'], env_db)
-
-    def test_successful_specified_section(self):
-        expected = {
-            'my-config': {
-                'admin-secret': 'Secret!',
-                'type': 'ec2'},
-        }
-        yaml_contents = expected.copy()
-        env_file = self.make_env_file(yaml.safe_dump(yaml_contents))
-        env_db = envs.load_generated(env_file, section='my-config')
-        self.assertEqual(expected['my-config'], env_db)
-
-
 class TestSave(helpers.EnvFileTestsMixin, unittest.TestCase):
 
     def setUp(self):

=== added file 'quickstart/tests/models/test_jenv.py'
--- quickstart/tests/models/test_jenv.py	1970-01-01 00:00:00 +0000
+++ quickstart/tests/models/test_jenv.py	2014-11-11 14:26:05 +0000
@@ -0,0 +1,110 @@
+# This file is part of the Juju Quickstart Plugin, which lets users set up a
+# Juju environment in very few steps (https://launchpad.net/juju-quickstart).
+# Copyright (C) 2014 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License version 3, as published by
+# the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Tests for the Juju Quickstart jenv generated files handling."""
+
+from __future__ import unicode_literals
+
+import unittest
+
+import yaml
+
+from quickstart.models import jenv
+from quickstart.tests import helpers
+
+
+class TestExists(helpers.JenvFileTestsMixin, unittest.TestCase):
+
+    def test_found(self):
+        # True is returned if the jenv file exists.
+        with self.make_jenv('ec2', ''):
+            exists = jenv.exists('ec2')
+        self.assertTrue(exists)
+
+    def test_not_found(self):
+        # False is returned if the jenv file does not exist.
+        with self.make_jenv('ec2', ''):
+            exists = jenv.exists('local')
+        self.assertFalse(exists)
+
+
+class TestGetValue(
+        helpers.JenvFileTestsMixin, helpers.ValueErrorTestsMixin,
+        unittest.TestCase):
+
+    def test_whole_content(self):
+        # The function correctly returns the whole jenv content.
+        with self.make_jenv('local', yaml.safe_dump(self.jenv_data)):
+            data = jenv.get_value('local')
+        self.assertEqual(self.jenv_data, data)
+
+    def test_section(self):
+        # The function correctly returns a whole section.
+        with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
+            data = jenv.get_value('ec2', 'bootstrap-config')
+        self.assertEqual(self.jenv_data['bootstrap-config'], data)
+
+    def test_value(self):
+        # The function correctly returns a value in the root node.
+        with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
+            value = jenv.get_value('ec2', 'user')
+        self.assertEqual('admin', value)
+
+    def test_section_value(self):
+        # The function correctly returns a section value.
+        with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
+            value = jenv.get_value('ec2', 'bootstrap-config', 'admin-secret')
+        self.assertEqual('Secret!', value)
+
+    def test_nested_section(self):
+        # The function correctly returns nested section's values.
+        with self.make_jenv('hp', yaml.safe_dump(self.jenv_data)):
+            value = jenv.get_value('hp', 'life', 'universe', 'everything')
+        self.assertEqual(42, value)
+
+    def test_file_not_found(self):
+        # A ValueError is raised if the jenv file cannot be found.
+        expected_error = 'unable to open file'
+        with self.make_jenv('hp', yaml.safe_dump(self.jenv_data)):
+            with self.assertRaises(ValueError) as context_manager:
+                jenv.get_value('local')
+        self.assertIn(expected_error, bytes(context_manager.exception))
+
+    def test_section_not_found(self):
+        # A ValueError is raised if the specified section cannot be found.
+        with self.make_jenv('local', yaml.safe_dump(self.jenv_data)) as path:
+            expected_error = (
+                'invalid YAML contents in {}: no-such key not found in the '
+                'root section'.format(path))
+            with self.assert_value_error(expected_error):
+                jenv.get_value('local', 'no-such')
+
+    def test_subsection_not_found(self):
+        # A ValueError is raised if the specified subsection cannot be found.
+        with self.make_jenv('local', yaml.safe_dump(self.jenv_data)) as path:
+            expected_error = (
+                'invalid YAML contents in {}: series key not found in the '
+                'bootstrap-config section'.format(path))
+            with self.assert_value_error(expected_error):
+                jenv.get_value('local', 'bootstrap-config', 'series')
+
+    def test_invalid_yaml_contents(self):
+        # A ValueError is raised if the jenv file is not well formed.
+        expected_error = 'unable to parse file'
+        with self.make_jenv('ec2', ':'):
+            with self.assertRaises(ValueError) as context_manager:
+                jenv.get_value('ec2')
+        self.assertIn(expected_error, bytes(context_manager.exception))

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-11-10 11:39:40 +0000
+++ quickstart/tests/test_app.py	2014-11-11 14:26:05 +0000
@@ -673,26 +673,24 @@
         ] + self.make_status_calls(2))
 
 
-class TestGetValueFromJenv(unittest.TestCase):
-
-    def test_no_key(self):
-        with mock.patch('quickstart.manage.envs.load_generated',
-                        lambda x: {}):
-            with self.assertRaises(ValueError) as exc:
-                app.get_value_from_jenv(
-                    'local', '/home/bac/.juju', 'my-key')
-        expected = (
-            u'my-key not found in '
-            '/home/bac/.juju/environments/local.jenv')
-        self.assertIn(expected, bytes(exc.exception))
+class TestGetAdminSecret(
+        helpers.JenvFileTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
 
     def test_success(self):
-        expected = 'superchunk'
-        with mock.patch('quickstart.manage.envs.load_generated',
-                        lambda x: {'my-key': expected}):
-            secret = app.get_value_from_jenv(
-                'local', '~bac/.juju', 'my-key')
-        self.assertEqual(expected, secret)
+        # The admin secret is successfully retrieved.
+        with self.make_jenv('ec2', yaml.safe_dump(self.jenv_data)):
+            admin_secret = app.get_admin_secret('ec2')
+        self.assertEqual('Secret!', admin_secret)
+
+    def test_error(self):
+        # A ProgramExit is raised if the admin secret cannot be retrieved.
+        with self.make_jenv('ec2', '') as path:
+            expected_error = (
+                'cannot retrieve environment admin-secret: invalid YAML '
+                'contents in {}: bootstrap-config key not found in the root '
+                'section'.format(path))
+            with self.assert_program_exit(expected_error):
+                app.get_admin_secret('ec2')
 
 
 class TestGetApiUrl(

=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2014-11-10 11:27:04 +0000
+++ quickstart/tests/test_manage.py	2014-11-11 14:26:05 +0000
@@ -509,7 +509,6 @@
         options = self.make_options(
             env_file, env_name='aws', interactive=False)
         manage._setup_env(options, self.parser)
-        self.assertEqual('Secret!', options.admin_secret)
         self.assertEqual(env_file, options.env_file)
         self.assertEqual('aws', options.env_name)
         self.assertEqual('ec2', options.env_type)
@@ -550,7 +549,6 @@
         options = self.make_options(
             env_file, env_name='lxc', interactive=False)
         manage._setup_env(options, self.parser)
-        self.assertEqual('Secret!', options.admin_secret)
         self.assertEqual(env_file, options.env_file)
         self.assertEqual('lxc', options.env_name)
         self.assertEqual('local', options.env_type)
@@ -593,7 +591,6 @@
         mock_interactive.assert_called_once_with(
             self.parser, mock_get_env_type_db(), env_db, env_file)
         # The options is updated with data from the selected environment.
-        self.assertEqual('Secret!', options.admin_secret)
         self.assertEqual(env_file, options.env_file)
         self.assertEqual('aws', options.env_name)
         self.assertEqual('ec2', options.env_type)
@@ -754,7 +751,6 @@
     def make_options(self, **kwargs):
         """Set up the options to be passed to the run function."""
         options = {
-            'admin_secret': 'Secretz!',
             'bundle': None,
             'bundle_id': None,
             'charm_url': None,
@@ -767,17 +763,6 @@
         options.update(kwargs)
         return mock.Mock(**options)
 
-    @staticmethod
-    def mock_get_value_from_jenv_success(name, home, key):
-        return 'from jenv'
-
-    @staticmethod
-    def mock_get_value_from_jenv_error(name, home, key):
-        fn = '{}.jenv'.format(name)
-        path = os.path.join(home, 'environments', fn)
-        msg = '{} not found in {}'.format(key, path)
-        raise ValueError(msg.encode('utf-8'))
-
     def test_no_bundle(self, mock_app, mock_open):
         # The application runs correctly if no bundle is provided.
         mock_app.ensure_dependencies.return_value = (1, 18, 0)
@@ -790,7 +775,8 @@
         unit_data = {'Name': 'juju-gui/0'}
         mock_app.check_environment.return_value = (
             'cs:trusty/juju-gui-42', '0', service_data, unit_data)
-        mock_app.get_value_from_jenv = self.mock_get_value_from_jenv_error
+        # Make mock_app.get_admin_secret return the admin secret
+        mock_app.get_admin_secret.return_value = 'Secret!'
         # Make mock_app.watch return the Juju GUI unit address.
         mock_app.watch.return_value = '1.2.3.4'
         # Make mock_app.create_auth_token return a fake authentication token.
@@ -810,10 +796,9 @@
         mock_app.get_api_url.assert_called_once_with(
             options.env_name, self.juju_command)
         mock_app.connect.assert_has_calls([
-            mock.call(
-                mock_app.get_api_url(), options.admin_secret),
+            mock.call(mock_app.get_api_url(), 'Secret!'),
             mock.call().close(),
-            mock.call('wss://1.2.3.4:443/ws', options.admin_secret),
+            mock.call('wss://1.2.3.4:443/ws', 'Secret!'),
             mock.call().close(),
         ])
         mock_app.check_environment.assert_called_once_with(
@@ -906,44 +891,9 @@
         manage.run(options)
         self.assertFalse(mock_open.called)
 
-    def test_admin_secret_fetched(self, mock_app, mock_open):
-        # If an admin secret is fetched from jenv it is used, even if one is
-        # found in environments.yaml, as set in options.admin_secret.
-        mock_app.get_value_from_jenv = self.mock_get_value_from_jenv_success
-        # Make mock_app.bootstrap return the already_bootstrapped flag and the
-        # bootstrap node series.
-        mock_app.bootstrap.return_value = (True, 'precise')
-        # Make mock_app.check_environment return the charm URL, the machine
-        # where to deploy the charm, the service and unit data.
-        mock_app.check_environment.return_value = (
-            'cs:precise/juju-gui-42', '0', None, None)
-        options = self.make_options(admin_secret='secret in environments.yaml')
-        manage.run(options)
-        mock_app.connect.assert_has_calls([
-            mock.call(mock_app.get_api_url(), 'from jenv'),
-        ])
-
-    def test_admin_secret_from_environments_yaml(self, mock_app, mock_open):
-        # If an admin secret is not fetched from jenv, then the one from
-        # environments.yaml is used, as found in options.admin_secret.
-        mock_app.get_value_from_jenv = self.mock_get_value_from_jenv_error
-        # Make mock_app.bootstrap return the already_bootstrapped flag and the
-        # bootstrap node series.
-        mock_app.bootstrap.return_value = (True, 'precise')
-        # Make mock_app.check_environment return the charm URL, the machine
-        # where to deploy the charm, the service and unit data.
-        mock_app.check_environment.return_value = (
-            'cs:precise/juju-gui-42', '0', None, None)
-        options = self.make_options(admin_secret='secret in environments.yaml')
-        manage.run(options)
-        mock_app.connect.assert_has_calls([
-            mock.call(mock_app.get_api_url(), 'secret in environments.yaml'),
-        ])
-
     def test_no_admin_secret_found(self, mock_app, mock_open):
-        # If admin-secret cannot be found anywhere a ProgramExit is called.
-        mock_app.ProgramExit = app.ProgramExit
-        mock_app.get_value_from_jenv = self.mock_get_value_from_jenv_error
+        # If admin-secret cannot be found a ProgramExit is called.
+        mock_app.get_admin_secret.side_effect = app.ProgramExit('bad wolf')
         # Make mock_app.bootstrap return the already_bootstrapped flag and the
         # bootstrap node series.
         mock_app.bootstrap.return_value = (True, 'precise')
@@ -952,15 +902,11 @@
         mock_app.check_environment.return_value = (
             'cs:precise/juju-gui-42', '0', None, None)
         options = self.make_options(
-            admin_secret=None,
             env_name='local',
             env_file='environments.yaml')
         with self.assertRaises(app.ProgramExit) as context:
             manage.run(options)
-        expected = (
-            u'admin-secret not found in {}/environments/local.jenv '
-            'or environments.yaml'.format(settings.JUJU_HOME))
-        self.assertEqual(expected, context.exception.message)
+        self.assertEqual('bad wolf', context.exception.message)
 
     def test_juju_environ_var_set(self, mock_app, mock_open):
         mock_app.bootstrap.return_value = (True, 'precise')

=== modified file 'quickstart/tests/test_serializers.py'
--- quickstart/tests/test_serializers.py	2013-12-06 18:16:02 +0000
+++ quickstart/tests/test_serializers.py	2014-11-11 14:26:05 +0000
@@ -18,12 +18,15 @@
 
 from __future__ import unicode_literals
 
+import os
+import tempfile
 import unittest
 
 import mock
 import yaml
 
 from quickstart import serializers
+from quickstart.tests import helpers
 
 
 class TestYamlLoad(unittest.TestCase):
@@ -56,3 +59,41 @@
         # Collections are serialized in the block style.
         contents = serializers.yaml_dump(self.data)
         self.assertEqual('myint: 42\nmystring: foo\n', contents)
+
+
+class TestYamlLoadFromPath(helpers.ValueErrorTestsMixin, unittest.TestCase):
+
+    def make_file(self, contents):
+        """Create a temporary file with the given contents.
+
+        Return the file path.
+        """
+        yaml_file = tempfile.NamedTemporaryFile(delete=False)
+        self.addCleanup(os.remove, yaml_file.name)
+        yaml_file.write(contents)
+        yaml_file.close()
+        return yaml_file.name
+
+    def test_resulting_contents(self):
+        # The YAML deserialized contents are correctly returned.
+        expected_data = {'myint': 42, 'mystring': 'foo'}
+        path = self.make_file(yaml.safe_dump(expected_data))
+        obtained_data = serializers.yaml_load_from_path(path)
+        self.assertEqual(expected_data, obtained_data)
+
+    def test_no_file(self):
+        # A ValueError is raised if the given file is not found.
+        expected_error = (
+            "unable to open file /no/such/file.yaml: "
+            "[Errno 2] No such file or directory: '/no/such/file.yaml'"
+        )
+        with self.assert_value_error(expected_error):
+            serializers.yaml_load_from_path('/no/such/file.yaml')
+
+    def test_invalid_yaml(self):
+        # A ValueError is raised if the given file is not a valid YAML.
+        path = self.make_file(':')
+        with self.assertRaises(ValueError) as context_manager:
+            serializers.yaml_load_from_path(path)
+        expected = 'unable to parse file {}'.format(path)
+        self.assertIn(expected, bytes(context_manager.exception))