Code review comment for lp:~frankban/charms/precise/juju-gui/install-yaml

Reviewers: mp+195113_code.launchpad.net,

Message:
Please take a look.

Description:
Fix base Python dependencies.

The install hook imported charmhelpers
before installing python-yaml, and
charmhelpers imports from yaml.

This very old bug has been hidden by the
fact that python-yaml is automatically
installed in ec2/lxc instances, but showed
itself when using the manual provider.

Copy/pasting and integrating the QA from
my last branch, a double check can help
before the release.
QA:
- Bootstrap a Juju environment with --debug.
- Deploy and expose the GUI (make deploy).
- Check juju debug-log (or
   less ~/.juju/local/log/unit-juju-gui-0.log if you
   used lxc): you should see the following message
   at the beginning of the install hook log:
   "Installing base Python dependencies: python-apt,
   python-launchpadlib, python-tempita, python-yaml."
- Wait for the GUI to be ready/started.
- Deploy this bundle: http://pastebin.ubuntu.com/6411548/
- Check everything is ok, xy annotations work (
   the services are vertically aligned), wordpress has
   customized constraints, mysql customized options and
   two units.
- No try to deploy the same bundle again, you
   will see a "services already there" kind of error.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/install-yaml/+merge/195113

(do not edit description out of merge proposal)

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

Affected files (+30, -25 lines):
   A [revision details]
   M hooks/backend.py
   M hooks/install
   M hooks/utils.py
   M revision
   M server-requirements.pip
   M tests/test_backends.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: revision
=== modified file 'revision'
--- revision 2013-11-13 16:36:02 +0000
+++ revision 2013-11-13 18:01:03 +0000
@@ -1,1 +1,1 @@
-97
+98

Index: server-requirements.pip
=== modified file 'server-requirements.pip'
--- server-requirements.pip 2013-11-13 17:45:50 +0000
+++ server-requirements.pip 2013-11-13 18:01:03 +0000
@@ -18,6 +18,8 @@

  # Note: the order of the following dependencies is important! The last ones
  # depends on the previous.
+# The python-bzrlib and python-yaml dependencies are installed using apt in
+# the charm hooks.

  futures==2.1.4
  tornado==3.1.1

Index: hooks/backend.py
=== modified file 'hooks/backend.py'
--- hooks/backend.py 2013-10-08 07:46:11 +0000
+++ hooks/backend.py 2013-11-13 18:01:03 +0000
@@ -106,8 +106,7 @@

  class GoMixin(object):
      """Manage the real Go juju-core backend."""
-
- debs = ('python-yaml',)
+ pass

  class GuiMixin(object):

Index: hooks/install
=== modified file 'hooks/install'
--- hooks/install 2013-10-07 14:45:00 +0000
+++ hooks/install 2013-11-13 18:01:03 +0000
@@ -20,10 +20,6 @@
  import errno
  import os

-from charmhelpers import (
- get_config,
- log,
-)
  from shelltoolbox import (
      apt_get_install,
      run,
@@ -32,13 +28,21 @@

  # Python dependencies must be installed here so that the charm can import
and
  # use required libraries.
-PYTHON_DEPENDENCIES =
('python-apt', 'python-launchpadlib', 'python-tempita')
+PYTHON_DEPENDENCIES = (
+ 'python-apt', 'python-launchpadlib', 'python-tempita', 'python-yaml')

-log('Installing base Python dependnecies: {}.'.format(
+run('juju-log', '--', 'Installing base Python dependencies: {}.'.format(
      ', '.join(PYTHON_DEPENDENCIES)))
  apt_get_install(*PYTHON_DEPENDENCIES)

+# The charmhelpers module depends on python-yaml and must be imported only
+# after the installation og the Python dependencies above.
+from charmhelpers import (
+ get_config,
+ log,
+)
+
  from utils import (
      config_json,
      log_hook,

Index: hooks/utils.py
=== modified file 'hooks/utils.py'
--- hooks/utils.py 2013-11-12 12:23:25 +0000
+++ hooks/utils.py 2013-11-13 18:01:03 +0000
@@ -74,11 +74,20 @@
  import urlparse

  import apt
+from launchpadlib.launchpad import Launchpad
  import tempita
+import yaml

-from launchpadlib.launchpad import Launchpad
+from charmhelpers import (
+ get_config,
+ log,
+ RESTART,
+ service_control,
+ START,
+ STOP,
+ unit_get,
+)
  from shelltoolbox import (
- Serializer,
      apt_get_install,
      command,
      environ,
@@ -86,17 +95,9 @@
      run,
      script_name,
      search_file,
+ Serializer,
      su,
  )
-from charmhelpers import (
- RESTART,
- START,
- STOP,
- get_config,
- log,
- service_control,
- unit_get,
-)

  AGENT = 'juju-api-agent'
@@ -173,7 +174,6 @@
      # The JUJU_API_ADDRESSES environment variable is not included in the
hooks
      # context in older releases of juju-core. Retrieve it from the
machiner
      # agent file instead.
- import yaml # python-yaml is only installed if juju-core is used.
      if unit_dir is None:
          base_dir = os.path.join(CURRENT_DIR, '..', '..')
      else:

Index: tests/test_backends.py
=== modified file 'tests/test_backends.py'
--- tests/test_backends.py 2013-10-07 14:17:11 +0000
+++ tests/test_backends.py 2013-11-13 18:31:14 +0000
@@ -33,12 +33,10 @@

  EXPECTED_PYTHON_LEGACY_DEBS = ('apache2', 'curl', 'haproxy', 'openssl')
-EXPECTED_GO_LEGACY_DEBS = (
- 'apache2', 'curl', 'haproxy', 'openssl', 'python-yaml')
+EXPECTED_GO_LEGACY_DEBS = ('apache2', 'curl', 'haproxy', 'openssl')
  EXPECTED_PYTHON_BUILTIN_DEBS = (
      'curl', 'openssl', 'python-bzrlib', 'python-pip')
-EXPECTED_GO_BUILTIN_DEBS = (
- 'curl', 'openssl', 'python-bzrlib', 'python-pip', 'python-yaml')
+EXPECTED_GO_BUILTIN_DEBS =
('curl', 'openssl', 'python-bzrlib', 'python-pip')

  simulate_pyjuju = mock.patch('utils.legacy_juju',
mock.Mock(return_value=True))
  simulate_juju_core = mock.patch(