Merge lp:~mew/charm-helpers/refactor-to-core into lp:charm-helpers

[Matthew Wedgwood]

Refactor charmsupport and charmhelpers(legacy) code in to core modules for charmhelpers

This doozy of an MP moves the bulk of library code into a main module structure:

core: low-level code for interacting with the charm environment
  hookenv: work with the charm's juju environment
  host: perform common configuration actions on the host
fetch: work with external sources such as apt, Launchpad, Github, and others
payload: work with charm payloads (code injected before deployment)

This release is NOT stable, as (for example) some code in host.py will eventually land in the fetch module.

[Brad Crittenden (bac)]

Hi Matthew,

Thanks for doing this reorganization. Lots of work and gets the project moving.

I couldn't get 'make test' to run until I installed
python-nose
python-mock
python-testtools

Could you add those to README.test?

The temporary mapping in __ALL__ for stuff moved from charmhelpers/__init__.py is a nice touch.

A lot of the things you marked 'Deprecated: client-only' are functions that we used in testing our charms. They are actually quite nice. I'd like to see the 'Deprecation' tag removed and try to find a place to put them. If you'd like to see them in use have a look at the buildbot-master charm.

Overall I like this direction.

review: Approve (code)

[Matthew Wedgwood (mew)]

Those client-only pieces don't really fit the objectives of charm-helpers to be a framework/library for use inside charms. I agree that they are useful, but they should live outside the project (probably in charm-tools).

[lp:~mew/charm-helpers/refactor-to-core]

20. By Matthew Wedgwood on 2013-05-23

[agy] Fix upstart detection, initscript tests, and README (for testing)

[Francesco Banconi (frankban)]

This is a very nice incremental step Matthew, thank you.

The tests runs well.
"make lint" requires flake8 to be installed (I did it with "sudo pip install flake8").
There are several lint/pep8 failures, and I understand that fixing them is not the goal of this branch.
For this reason feel free to ignore the minors below.

267 + config,
268 + local_unit,
269 + log,
270 + relation_ids,
271 + relation_set,
272 + )
Minor: these lines could be dedented.

395 + for key in ('provides','requires','peers'):
Missing whitespace after ','.

review: Approve

=== modified file 'README.test'
--- README.test	2013-05-13 13:06:50 +0000
+++ README.test	2013-05-23 20:50:31 +0000
@@ -2,3 +2,6 @@
 -----------------------------------
 python-shelltoolbox
 python-tempita
+python-nose
+python-mock
+python-testtools
 
=== modified file 'charmhelpers/contrib/charmhelpers/__init__.py'
--- charmhelpers/contrib/charmhelpers/__init__.py	2013-05-11 20:25:54 +0000
+++ charmhelpers/contrib/charmhelpers/__init__.py	2013-05-23 20:50:31 +0000
@@ -1,160 +1,60 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import warnings
+warnings.warn("contrib.charmhelpers is deprecated", DeprecationWarning)
+
 """Helper functions for writing Juju charms in Python."""
 
 __metaclass__ = type
-__all__ = ['get_config',
-           'log',
-           'log_entry',
-           'log_exit',
-           'relation_get',
-           'relation_set',
-           'relation_ids',
-           'relation_list',
-           'config_get',
-           'unit_get',
-           'open_port',
-           'close_port',
-           'service_control',
-           'unit_info',
-           'wait_for_machine',
-           'wait_for_page_contents',
-           'wait_for_relation',
-           'wait_for_unit',
+__all__ = [
+           #'get_config',             # core.hookenv.config()
+           #'log',                    # core.hookenv.log()
+           #'log_entry',              # core.hookenv.log()
+           #'log_exit',               # core.hookenv.log()
+           #'relation_get',           # core.hookenv.relation_get()
+           #'relation_set',           # core.hookenv.relation_set()
+           #'relation_ids',           # core.hookenv.relation_ids()
+           #'relation_list',          # core.hookenv.relation_units()
+           #'config_get',             # core.hookenv.config()
+           #'unit_get',               # core.hookenv.unit_get()
+           #'open_port',              # core.hookenv.open_port()
+           #'close_port',             # core.hookenv.close_port()
+           #'service_control',        # core.host.service()
+           'unit_info',              # client-side, NOT IMPLEMENTED
+           'wait_for_machine',       # client-side, NOT IMPLEMENTED
+           'wait_for_page_contents', # client-side, NOT IMPLEMENTED
+           'wait_for_relation',      # client-side, NOT IMPLEMENTED
+           'wait_for_unit',          # client-side, NOT IMPLEMENTED
            ]
 
-from collections import namedtuple
-import json
 import operator
 from shelltoolbox import (
     command,
-    script_name,
-    run,
 )
 import tempfile
 import time
 import urllib2
 import yaml
-from subprocess import CalledProcessError
-
 
 SLEEP_AMOUNT = 0.1
-Env = namedtuple('Env', 'uid gid home')
 # We create a juju_status Command here because it makes testing much,
 # much easier.
 juju_status = lambda: command('juju')('status')
 
-
-def log(message, juju_log=command('juju-log')):
-    return juju_log('--', message)
-
-
-def log_entry():
-    log("--> Entering {}".format(script_name()))
-
-
-def log_exit():
-    log("<-- Exiting {}".format(script_name()))
-
-
-def get_config():
-    _config_get = command('config-get', '--format=json')
-    return json.loads(_config_get())
-
-
-def relation_get(attribute=None, unit=None, rid=None):
-    cmd = command('relation-get')
-    if attribute is None and unit is None and rid is None:
-        return cmd().strip()
-    _args = []
-    if rid:
-        _args.append('-r')
-        _args.append(rid)
-    if attribute is not None:
-        _args.append(attribute)
-    if unit:
-        _args.append(unit)
-    return cmd(*_args).strip()
-
-
-def relation_set(**kwargs):
-    cmd = command('relation-set')
-    args = ['{}={}'.format(k, v) for k, v in kwargs.items()]
-    cmd(*args)
-
-
-def relation_ids(relation_name):
-    cmd = command('relation-ids')
-    args = [relation_name]
-    return cmd(*args).split()
-
-
-def relation_list(rid=None):
-    cmd = command('relation-list')
-    args = []
-    if rid:
-        args.append('-r')
-        args.append(rid)
-    return cmd(*args).split()
-
-
-def config_get(attribute):
-    cmd = command('config-get')
-    args = [attribute]
-    return cmd(*args).strip()
-
-
-def unit_get(attribute):
-    cmd = command('unit-get')
-    args = [attribute]
-    return cmd(*args).strip()
-
-
-def open_port(port, protocol="TCP"):
-    cmd = command('open-port')
-    args = ['{}/{}'.format(port, protocol)]
-    cmd(*args)
-
-
-def close_port(port, protocol="TCP"):
-    cmd = command('close-port')
-    args = ['{}/{}'.format(port, protocol)]
-    cmd(*args)
-
-START = "start"
-RESTART = "restart"
-STOP = "stop"
-RELOAD = "reload"
-
-
-def service_control(service_name, action):
-    cmd = command('service')
-    args = [service_name, action]
-    try:
-        if action == RESTART:
-            try:
-                cmd(*args)
-            except CalledProcessError:
-                service_control(service_name, START)
-        else:
-            cmd(*args)
-    except CalledProcessError:
-        log("Failed to perform {} on service {}".format(action, service_name))
-
-
-def configure_source(update=False):
-    source = config_get('source')
-    if ((source.startswith('ppa:') or
-         source.startswith('cloud:') or
-         source.startswith('http:'))):
-        run('add-apt-repository', source)
-    if source.startswith("http:"):
-        run('apt-key', 'import', config_get('key'))
-    if update:
-        run('apt-get', 'update')
-
-
+# re-implemented as charmhelpers.fetch.configure_sources()
+#def configure_source(update=False):
+#    source = config_get('source')
+#    if ((source.startswith('ppa:') or
+#         source.startswith('cloud:') or
+#         source.startswith('http:'))):
+#        run('add-apt-repository', source)
+#    if source.startswith("http:"):
+#        run('apt-key', 'import', config_get('key'))
+#    if update:
+#        run('apt-get', 'update')
+
+# DEPRECATED: client-side only
 def make_charm_config_file(charm_config):
     charm_config_file = tempfile.NamedTemporaryFile()
     charm_config_file.write(yaml.dump(charm_config))
@@ -165,6 +65,7 @@
     return charm_config_file
 
 
+# DEPRECATED: client-side only
 def unit_info(service_name, item_name, data=None, unit=None):
     if data is None:
         data = yaml.safe_load(juju_status())
@@ -189,10 +90,12 @@
     return item
 
 
+# DEPRECATED: client-side only
 def get_machine_data():
     return yaml.safe_load(juju_status())['machines']
 
 
+# DEPRECATED: client-side only
 def wait_for_machine(num_machines=1, timeout=300):
     """Wait `timeout` seconds for `num_machines` machines to come up.
 
@@ -232,6 +135,7 @@
     return num_machines, time.time() - start_time
 
 
+# DEPRECATED: client-side only
 def wait_for_unit(service_name, timeout=480):
     """Wait `timeout` seconds for a given service name to come up."""
     wait_for_machine(num_machines=1)
@@ -247,6 +151,7 @@
         raise RuntimeError('unit did not start, agent-state: ' + state)
 
 
+# DEPRECATED: client-side only
 def wait_for_relation(service_name, relation_name, timeout=120):
     """Wait `timeout` seconds for a given relation to come up."""
     start_time = time.time()
@@ -259,6 +164,7 @@
         time.sleep(SLEEP_AMOUNT)
 
 
+# DEPRECATED: client-side only
 def wait_for_page_contents(url, contents, timeout=120, validate=None):
     if validate is None:
         validate = operator.contains
 
=== modified file 'charmhelpers/contrib/charmsupport/nrpe.py'
--- charmhelpers/contrib/charmsupport/nrpe.py	2013-05-11 20:05:13 +0000
+++ charmhelpers/contrib/charmsupport/nrpe.py	2013-05-23 20:50:31 +0000
@@ -12,8 +12,14 @@
 import shlex
 import yaml
 
-from hookenv import config, local_unit, log, relation_ids, relation_set
-from host import service
+from charmhelpers.core.hookenv import (
+        config,
+        local_unit,
+        log,
+        relation_ids,
+        relation_set,
+    )
+from charmhelpers.core.host import service
 
 # This module adds compatibility with the nrpe-external-master and plain nrpe
 # subordinate charms. To use it in your charm:
 
=== modified file 'charmhelpers/contrib/jujugui/utils.py'
--- charmhelpers/contrib/jujugui/utils.py	2013-05-11 20:39:53 +0000
+++ charmhelpers/contrib/jujugui/utils.py	2013-05-23 20:50:31 +0000
@@ -64,11 +64,12 @@
     search_file,
     su,
 )
-from charmhelpers.contrib.charmhelpers import (
-    START,
-    get_config,
+from charmhelpers.core.host import (
+    service_start,
+)
+from charmhelpers.core.hookenv import (
     log,
-    service_control,
+    config,
     unit_get,
 )
 
@@ -263,9 +264,9 @@
     global results_log
     if results_log is not None:
         return
-    config = get_config()
+    cfg = config()
     logging.basicConfig(
-        filename=config['command-log-file'],
+        filename=cfg['command-log-file'],
         level=logging.INFO,
         format="%(asctime)s: %(name)s@%(levelname)s %(message)s")
     results_log = logging.getLogger('juju-gui')
@@ -295,7 +296,7 @@
     render_to_file('config/juju-api-improv.conf.template', context, config_path)
     log('Starting the staging backend.')
     with su('root'):
-        service_control(IMPROV, START)
+        service_start(IMPROV)
 
 
 def start_agent(
@@ -317,7 +318,7 @@
     render_to_file('config/juju-api-agent.conf.template', context, config_path)
     log('Starting API agent.')
     with su('root'):
-        service_control(AGENT, START)
+        service_start(AGENT)
 
 
 def start_gui(
 
=== added directory 'charmhelpers/core'
=== added file 'charmhelpers/core/__init__.py'
=== renamed file 'charmhelpers/contrib/charmsupport/hookenv.py' => 'charmhelpers/core/hookenv.py'
--- charmhelpers/contrib/charmsupport/hookenv.py	2013-05-11 20:24:29 +0000
+++ charmhelpers/core/hookenv.py	2013-05-23 20:50:31 +0000
@@ -1,8 +1,8 @@
 "Interactions with the Juju environment"
-# Copyright 2012 Canonical Ltd.
+# Copyright 2013 Canonical Ltd.
 #
 # Authors:
-#  Matthew Wedgwood <[e-mail]>
+#  Charm Helpers Developers <[e-mail]>
 
 import os
 import json
@@ -62,8 +62,11 @@
     """A convenient bundling of the current execution context"""
     context = {}
     context['conf'] = config()
+    context['reltype'] = relation_type()
+    context['relid'] = relation_id()
     context['unit'] = local_unit()
-    context['rel'] = relations_of_type()
+    context['rels'] = relations()
+    context['rel'] = relation_get()
     context['env'] = os.environ
     return context
 
@@ -112,11 +115,13 @@
     if rid:
         _args.append('-r')
         _args.append(rid)
-    if attribute is not None:
-        _args.append(attribute)
+    _args.append(attribute or '-')
     if unit:
         _args.append(unit)
-    return json.loads(subprocess.check_output(_args))
+    try:
+        return json.loads(subprocess.check_output(_args))
+    except ValueError:
+        return None
 
 
 def relation_set(relation_id=None, **kwargs):
@@ -134,7 +139,8 @@
     relid_cmd_line = ['relation-ids', '--format=json']
     if reltype is not None:
         relid_cmd_line.append(reltype)
-    return json.loads(subprocess.check_output(relid_cmd_line))
+        return json.loads(subprocess.check_output(relid_cmd_line))
+    return []
 
 
 def related_units(relid=None):
@@ -179,6 +185,53 @@
     return relation_data
 
 
+def relation_types():
+    "Get a list of relation types supported by this charm"
+    charmdir = os.environ.get('CHARM_DIR', '')
+    mdf = open(os.path.join(charmdir, 'metadata.yaml'))
+    md = yaml.safe_load(mdf)
+    rel_types = []
+    for key in ('provides','requires','peers'):
+        section = md.get(key)
+        if section:
+            rel_types.extend(section.keys())
+    mdf.close()
+    return rel_types
+
+
+def relations():
+    rels = {}
+    for reltype in relation_types():
+        relids = {}
+        for relid in relation_ids(reltype):
+            units = {}
+            for unit in related_units(relid):
+                reldata = relation_get(unit=unit, rid=relid)
+                units[unit] = reldata
+            relids[relid] = units
+        rels[reltype] = relids
+    return rels
+
+
+def open_port(port, protocol="TCP"):
+    "Open a service network port"
+    _args = ['open-port']
+    _args.append('{}/{}'.format(port, protocol))
+    subprocess.check_call(_args)
+
+
+def close_port(port, protocol="TCP"):
+    "Close a service network port"
+    _args = ['close-port']
+    _args.append('{}/{}'.format(port, protocol))
+    subprocess.check_call(_args)
+
+
+def unit_get(attribute):
+    _args = ['unit-get', attribute]
+    return subprocess.check_output(_args).strip()
+
+
 class UnregisteredHookError(Exception):
     pass
 
 
=== renamed file 'charmhelpers/contrib/charmsupport/host.py' => 'charmhelpers/core/host.py'
--- charmhelpers/contrib/charmsupport/host.py	2013-05-11 20:05:13 +0000
+++ charmhelpers/core/host.py	2013-05-23 20:50:31 +0000
@@ -23,7 +23,7 @@
 
 def service(action, service_name):
     cmd = None
-    if os.path.exists(os.path.join('/etc/init', service_name)):
+    if os.path.exists(os.path.join('/etc/init', '%s.conf' % service_name)):
         cmd = ['initctl', action, service_name]
     elif os.path.exists(os.path.join('/etc/init.d', service_name)):
         cmd = [os.path.join('/etc/init.d', service_name), action]
 
=== added directory 'charmhelpers/fetch'
=== added file 'charmhelpers/fetch/__init__.py'
--- charmhelpers/fetch/__init__.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/fetch/__init__.py	2013-05-23 20:50:31 +0000
@@ -0,0 +1,46 @@
+from yaml import safe_load
+from core.hookenv import config_get
+from subprocess import check_call
+
+
+def add_source(source, key=None):
+    if ((source.startswith('ppa:') or
+         source.startswith('cloud:') or
+         source.startswith('http:'))):
+        check_call('add-apt-repository', source)
+    if key:
+        check_call('apt-key', 'import', key)
+
+
+class SourceConfigError(Exception):
+    pass
+
+
+def configure_sources(update=False,
+                      sources_var='install_sources',
+                      keys_var='install_keys'):
+    """
+    Configure multiple sources from charm configuration
+
+    Example config:
+        install_sources:
+          - "ppa:foo"
+          - "http://example.com/repo precise main"
+        install_keys:
+          - null
+          - "a1b2c3d4"
+
+    Note that 'null' (a.k.a. None) should not be quoted.
+    """
+    sources = safe_load(config_get(sources_var))
+    keys = safe_load(config_get(keys_var))
+    if isinstance(sources, basestring) and isinstance(keys, basestring):
+        add_source(sources, keys)
+    else:
+        if not len(sources) == len(keys):
+            msg = 'Install sources and keys lists are different lengths'
+            raise SourceConfigError(msg)
+        for src_num in range(len(sources)):
+            add_source(sources[src_num], sources[src_num])
+    if update:
+        check_call(('apt-get', 'update'))
 
=== added directory 'charmhelpers/payload'
=== added file 'charmhelpers/payload/__init__.py'
--- charmhelpers/payload/__init__.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/payload/__init__.py	2013-05-23 20:50:31 +0000
@@ -0,0 +1,1 @@
+"Tools for working with files injected into a charm just before deployment."
 
=== renamed file 'charmhelpers/contrib/charmsupport/execd.py' => 'charmhelpers/payload/execd.py'
--- charmhelpers/contrib/charmsupport/execd.py	2013-05-11 20:05:13 +0000
+++ charmhelpers/payload/execd.py	2013-05-23 20:50:31 +0000
@@ -3,32 +3,37 @@
 import os
 import sys
 import subprocess
-import hookenv
+from charmhelpers.core import hookenv
 
 
 def default_execd_dir():
     return os.path.join(os.environ['CHARM_DIR'],'exec.d')
 
 
-def execd_modules(execd_dir=None):
+def execd_module_paths(execd_dir=None):
     if not execd_dir:
         execd_dir = default_execd_dir()
     for subpath in os.listdir(execd_dir):
         module = os.path.join(execd_dir, subpath)
         if os.path.isdir(module):
-                yield module
-
-
-def execd_run(submodule, execd_dir=None, die_on_error=False):
-    for module_path in execd_modules(execd_dir):
+            yield module
+
+
+def execd_submodule_paths(submodule, execd_dir=None):
+    for module_path in execd_module_paths(execd_dir):
         path = os.path.join(module_path, submodule)
         if os.access(path, os.X_OK) and os.path.isfile(path):
-            try:
-                subprocess.check_call(path, shell=True)
-            except subprocess.CalledProcessError as e:
-                hookenv.log(e.output)
-                if die_on_error:
-                    sys.exit(e.returncode)
+            yield path
+
+
+def execd_run(submodule, execd_dir=None, die_on_error=False):
+    for submodule_path in execd_submodule_paths(submodule, execd_dir):
+        try:
+            subprocess.check_call(submodule_path, shell=True)
+        except subprocess.CalledProcessError as e:
+            hookenv.log(e.output)
+            if die_on_error:
+                sys.exit(e.returncode)
 
 
 def execd_preinstall(execd_dir=None):
 
=== modified file 'tests/contrib/charmhelpers/test_charmhelpers.py'
--- tests/contrib/charmhelpers/test_charmhelpers.py	2013-05-11 20:30:34 +0000
+++ tests/contrib/charmhelpers/test_charmhelpers.py	2013-05-23 20:50:31 +0000
@@ -3,7 +3,6 @@
 import unittest
 import yaml
 
-from simplejson import dumps
 from StringIO import StringIO
 from testtools import TestCase
 
@@ -14,8 +13,6 @@
 sys.path.insert(0, 'helpers/python')
 from charmhelpers.contrib import charmhelpers
 
-from subprocess import CalledProcessError
-
 
 class CharmHelpersTestCase(TestCase):
     """A basic test case for Python charm helpers."""
@@ -73,145 +70,6 @@
             self._make_juju_status_dict(
                 num_units, service_name, unit_state, machine_state))
 
-    def test_get_config(self):
-        # get_config returns the contents of the current charm
-        # configuration, as returned by config-get --format=json.
-        mock_config = {'key': 'value'}
-
-        # Monkey-patch shelltoolbox.command to avoid having to call out
-        # to config-get.
-        self._patch_command(lambda: dumps(mock_config))
-        self.assertEqual(mock_config, charmhelpers.get_config())
-
-    def test_config_get(self):
-        # config_get is used to retrieve individual configuration elements
-        mock_config = {'key': 'value'}
-
-        # Monkey-patch shelltoolbox.command to avoid having to call out
-        # to config-get.
-        self._patch_command(lambda *args: mock_config[args[0]])
-        self.assertEqual(mock_config['key'], charmhelpers.config_get('key'))
-
-    def test_unit_get(self):
-        # unit_get is used to retrieve individual configuration elements
-        mock_config = {'key': 'value'}
-
-        # Monkey-patch shelltoolbox.command to avoid having to call out
-        # to unit-get.
-        self._patch_command(lambda *args: mock_config[args[0]])
-        self.assertEqual(mock_config['key'], charmhelpers.unit_get('key'))
-
-    def test_relation_get(self):
-        # relation_get returns the value of a given relation variable,
-        # as returned by relation-get $VAR.
-        mock_relation_values = {
-            'foo': 'bar',
-            'spam': 'eggs'}
-        self._patch_command(lambda *args: mock_relation_values[args[0]])
-        self.assertEqual('bar', charmhelpers.relation_get('foo'))
-        self.assertEqual('eggs', charmhelpers.relation_get('spam'))
-
-        self._patch_command(lambda *args: mock_relation_values[args[2]])
-        self.assertEqual('bar',
-                         charmhelpers.relation_get('foo', 'test', 'test:1'))
-        self.assertEqual('eggs',
-                         charmhelpers.relation_get('spam', 'test', 'test:1'))
-
-        self._patch_command(lambda *args: '%s' % mock_relation_values)
-        self.assertEqual("{'foo': 'bar', 'spam': 'eggs'}",
-                         charmhelpers.relation_get())
-
-    def test_relation_set(self):
-        # relation_set calls out to relation-set and passes key=value
-        # pairs to it.
-        items_set = {}
-
-        def mock_relation_set(*args):
-            for arg in args:
-                key, value = arg.split("=")
-                items_set[key] = value
-        self._patch_command(mock_relation_set)
-        charmhelpers.relation_set(foo='bar', spam='eggs')
-        self.assertEqual('bar', items_set.get('foo'))
-        self.assertEqual('eggs', items_set.get('spam'))
-
-    def test_relation_ids(self):
-        # relation_ids returns a list of relations id for the given
-        # named relation
-        mock_relation_ids = {'test': 'test:1 test:2'}
-        self._patch_command(lambda *args: mock_relation_ids[args[0]])
-        self.assertEqual(mock_relation_ids['test'].split(),
-                         charmhelpers.relation_ids('test'))
-
-    def test_relation_list(self):
-        # relation_list returns a list of unit names either for the current
-        # context or for the provided relation ID
-        mock_unit_names = {'test:1': 'test/0 test/1 test/2',
-                           'test:2': 'test/3 test/4 test/5'}
-
-        # Patch command for current context use base - context = test:1
-        self._patch_command(lambda: mock_unit_names['test:1'])
-        self.assertEqual(mock_unit_names['test:1'].split(),
-                         charmhelpers.relation_list())
-        # Patch command for provided relation-id
-        self._patch_command(lambda *args: mock_unit_names[args[1]])
-        self.assertEqual(mock_unit_names['test:2'].split(),
-                         charmhelpers.relation_list(rid='test:2'))
-
-    def test_open_close_port(self):
-        # expose calls open-port with port/protocol parameters
-        ports_set = []
-
-        def mock_open_port(*args):
-            for arg in args:
-                ports_set.append(arg)
-
-        def mock_close_port(*args):
-            if args[0] in ports_set:
-                ports_set.remove(args[0])
-        # Monkey patch in the open-port mock
-        self._patch_command(mock_open_port)
-        charmhelpers.open_port(80, "TCP")
-        charmhelpers.open_port(90, "UDP")
-        charmhelpers.open_port(100)
-        self.assertTrue("80/TCP" in ports_set)
-        self.assertTrue("90/UDP" in ports_set)
-        self.assertTrue("100/TCP" in ports_set)
-        # Monkey patch in the close-port mock function
-        self._patch_command(mock_close_port)
-        charmhelpers.close_port(80, "TCP")
-        charmhelpers.close_port(90, "UDP")
-        charmhelpers.close_port(100)
-        # ports_set should now be empty
-        self.assertEquals(len(ports_set), 0)
-
-    def test_service_control(self):
-        # Collect commands that have been run
-        commands_set = {}
-
-        def mock_service(*args):
-            service = args[0]
-            action = args[1]
-            if service not in commands_set:
-                commands_set[service] = []
-            if ((len(commands_set[service]) > 1 and
-                 commands_set[service][-1] == 'stop' and
-                 action == 'restart')):
-                # Service is stopped - so needs 'start'
-                # action as restart will fail
-                commands_set[service].append(action)
-                raise CalledProcessError(1, repr(args))
-            else:
-                commands_set[service].append(action)
-
-        result = ['start', 'stop', 'restart', 'start']
-
-        # Monkey patch service command
-        self._patch_command(mock_service)
-        charmhelpers.service_control('myservice', 'start')
-        charmhelpers.service_control('myservice', 'stop')
-        charmhelpers.service_control('myservice', 'restart')
-        self.assertEquals(result, commands_set['myservice'])
 
     def test_make_charm_config_file(self):
         # make_charm_config_file() writes the passed configuration to a
@@ -428,48 +286,6 @@
             RuntimeError, charmhelpers.wait_for_page_contents,
             'http://example.com', "This will error", timeout=0)
 
-    def test_log(self):
-        # The "log" function forwards a string on to the juju-log command.
-        logged = []
-
-        def juju_log(*args):
-            logged.append(args)
-        charmhelpers.log('This is a log message', juju_log)
-        # Since we only logged one message, juju-log was only called once..
-        self.assertEqual(len(logged), 1)
-        # The message was included in the arguments passed to juju-log.
-        self.assertIn('This is a log message', logged[0])
-
-    def test_log_escapes_message(self):
-        # The Go version of juju-log interprets any string begining with two
-        # hyphens ("--") as a command-line switch, even if the third character
-        # is non-alphanumeric.  This is different behavior than the Python
-        # version of juju-log.  Therefore we signfiy the end of options by
-        # inserting the string " -- " just before the log message.
-        logged = []
-
-        def juju_log(*args):
-            logged.append(args)
-        charmhelpers.log('This is a log message', juju_log)
-        # The call to juju-log includes the " -- " string before the message.
-        self.assertEqual([('--', 'This is a log message')], logged)
-
-    def test_log_entry(self):
-        # The log_entry function logs a message about the script starting.
-        logged = []
-        self.patch(charmhelpers, 'log', logged.append)
-        self.patch(charmhelpers, 'script_name', lambda: 'SCRIPT-NAME')
-        charmhelpers.log_entry()
-        self.assertEqual(['--> Entering SCRIPT-NAME'], logged)
-
-    def test_log_exit(self):
-        # The log_exit function logs a message about the script ending.
-        logged = []
-        self.patch(charmhelpers, 'log', logged.append)
-        self.patch(charmhelpers, 'script_name', lambda: 'SCRIPT-NAME')
-        charmhelpers.log_exit()
-        self.assertEqual(['<-- Exiting SCRIPT-NAME'], logged)
-
 
 if __name__ == '__main__':
     unittest.main()
 
=== modified file 'tests/contrib/jujugui/test_utils.py'
--- tests/contrib/jujugui/test_utils.py	2013-05-11 20:39:53 +0000
+++ tests/contrib/jujugui/test_utils.py	2013-05-23 20:50:31 +0000
@@ -468,17 +468,17 @@
 class TestCmdLog(unittest.TestCase):
 
     def setUp(self):
-        # Patch the charmhelpers 'command', which powers get_config.  The
+        # Patch the utils 'config', which powers get_config.  The
         # result of this is the mock_config dictionary will be returned.
         # The monkey patch is undone in the tearDown.
-        self.command = charmhelpers.command
+        self.config = utils.config
         fd, self.log_file_name = tempfile.mkstemp()
         os.close(fd)
         mock_config = {'command-log-file': self.log_file_name}
-        charmhelpers.command = lambda *args: lambda: dumps(mock_config)
+        utils.config = lambda *args: mock_config
 
     def tearDown(self):
-        charmhelpers.command = self.command
+        utils.config = self.config
         os.unlink(self.log_file_name)
 
     def test_contents_logged(self):
@@ -491,16 +491,14 @@
 
     def setUp(self):
         self.service_names = []
-        self.actions = []
         self.svc_ctl_call_count = 0
         self.fake_zk_address = '192.168.5.26'
         # Monkey patches.
         self.command = charmhelpers.command
 
-        def service_control_mock(service_name, action):
+        def service_start_mock(service_name):
             self.svc_ctl_call_count += 1
             self.service_names.append(service_name)
-            self.actions.append(action)
 
         def noop(*args):
             pass
@@ -522,7 +520,7 @@
                 self.files[os.path.basename(dest)] = fp.read()
 
         self.functions = dict(
-            service_control=(utils.service_control, service_control_mock),
+            service_start=(utils.service_start, service_start_mock),
             log=(utils.log, noop),
             su=(utils.su, su),
             run=(utils.run, noop),
@@ -555,7 +553,6 @@
         self.assertTrue(self.ssl_cert_path in conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-api-improv'])
-        self.assertEqual(self.actions, [charmhelpers.START])
 
     def test_start_agent(self):
         start_agent(self.ssl_cert_path, 'config')
@@ -565,7 +562,6 @@
         self.assertTrue(self.ssl_cert_path in conf)
         self.assertEqual(self.svc_ctl_call_count, 1)
         self.assertEqual(self.service_names, ['juju-api-agent'])
-        self.assertEqual(self.actions, [charmhelpers.START])
 
     def test_start_gui(self):
         ssl_cert_path = '/tmp/certificates/'
 
=== added directory 'tests/core'
=== added file 'tests/core/__init__.py'
=== renamed file 'tests/contrib/charmsupport/test_hookenv.py' => 'tests/core/test_hookenv.py'
--- tests/contrib/charmsupport/test_hookenv.py	2013-05-11 20:24:29 +0000
+++ tests/core/test_hookenv.py	2013-05-23 20:50:31 +0000
@@ -1,11 +1,26 @@
 import json
 
-from mock import patch, call, MagicMock
+from mock import patch, call, mock_open
+from StringIO import StringIO
+from mock import MagicMock
 from testtools import TestCase
 import yaml
 
-from charmhelpers.contrib.charmsupport import hookenv
+from charmhelpers.core import hookenv
 
+CHARM_METADATA = """name: testmock
+summary: test mock summary
+description: test mock description
+requires:
+    testreqs:
+        interface: mock
+provides:
+    testprov:
+        interface: mock
+peers:
+    testpeer:
+        interface: mock
+"""
 
 class SerializableTest(TestCase):
     def test_serializes_object_to_json(self):
@@ -105,14 +120,14 @@
         check_output.assert_called_with(['config-get', 'baz', '--format=json'])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.log')
+    @patch('charmhelpers.core.hookenv.log')
     def test_logs_and_reraises_on_config_error(self, log, check_output):
         error = 'some error'
         check_output.side_effect = ValueError(error)
 
         self.assertRaisesRegexp(ValueError, error, hookenv.config)
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_local_unit(self, os_):
         os_.environ = {
             'JUJU_UNIT_NAME': 'foo',
@@ -120,7 +135,7 @@
 
         self.assertEqual(hookenv.local_unit(), 'foo')
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_checks_that_is_running_in_relation_hook(self, os_):
         os_.environ = {
             'JUJU_RELATION': 'foo',
@@ -128,7 +143,7 @@
 
         self.assertTrue(hookenv.in_relation_hook())
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_checks_that_is_not_running_in_relation_hook(self, os_):
         os_.environ = {
             'bar': 'foo',
@@ -136,7 +151,7 @@
 
         self.assertFalse(hookenv.in_relation_hook())
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_relation_type(self, os_):
         os_.environ = {
             'JUJU_RELATION': 'foo',
@@ -144,13 +159,13 @@
 
         self.assertEqual(hookenv.relation_type(), 'foo')
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_relation_type_none_if_not_in_environment(self, os_):
         os_.environ = {}
         self.assertEqual(hookenv.relation_type(), None)
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.relation_type')
     def test_gets_relation_ids(self, relation_type, check_output):
         ids = [1, 2, 3]
         check_output.return_value = json.dumps(ids)
@@ -164,7 +179,7 @@
                                          reltype])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.relation_type')
     def test_relation_ids_no_relation_type(self, relation_type, check_output):
         ids = [1, 2, 3]
         check_output.return_value = json.dumps(ids)
@@ -172,11 +187,10 @@
 
         result = hookenv.relation_ids()
 
-        self.assertEqual(result, ids)
-        check_output.assert_called_with(['relation-ids', '--format=json'])
+        self.assertEqual(result, [])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.relation_type')
     def test_gets_relation_ids_for_type(self, relation_type, check_output):
         ids = [1, 2, 3]
         check_output.return_value = json.dumps(ids)
@@ -190,7 +204,7 @@
         self.assertFalse(relation_type.called)
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_id')
+    @patch('charmhelpers.core.hookenv.relation_id')
     def test_gets_related_units(self, relation_id, check_output):
         relid = 123
         units = ['foo', 'bar']
@@ -204,7 +218,7 @@
                                          '-r', relid])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_id')
+    @patch('charmhelpers.core.hookenv.relation_id')
     def test_related_units_no_relation(self, relation_id, check_output):
         units = ['foo', 'bar']
         relation_id.return_value = None
@@ -216,7 +230,7 @@
         check_output.assert_called_with(['relation-list', '--format=json'])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_id')
+    @patch('charmhelpers.core.hookenv.relation_id')
     def test_gets_related_units_for_id(self, relation_id, check_output):
         relid = 123
         units = ['foo', 'bar']
@@ -229,7 +243,7 @@
                                          '-r', relid])
         self.assertFalse(relation_id.called)
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_remote_unit(self, os_):
         os_.environ = {
             'JUJU_REMOTE_UNIT': 'foo',
@@ -237,8 +251,8 @@
 
         self.assertEqual(hookenv.remote_unit(), 'foo')
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.remote_unit')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_get')
+    @patch('charmhelpers.core.hookenv.remote_unit')
+    @patch('charmhelpers.core.hookenv.relation_get')
     def test_gets_relation_for_unit(self, relation_get, remote_unit):
         unit = 'foo-unit'
         raw_relation = {
@@ -254,8 +268,8 @@
         self.assertEqual(getattr(result, 'baz-list'), ['1', '2', '3'])
         relation_get.assert_called_with(unit=unit, rid=None)
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.remote_unit')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_get')
+    @patch('charmhelpers.core.hookenv.remote_unit')
+    @patch('charmhelpers.core.hookenv.relation_get')
     def test_gets_relation_for_specific_unit(self, relation_get, remote_unit):
         unit = 'foo-unit'
         raw_relation = {
@@ -271,9 +285,9 @@
         relation_get.assert_called_with(unit=unit, rid=None)
         self.assertFalse(remote_unit.called)
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_ids')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.related_units')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_for_unit')
+    @patch('charmhelpers.core.hookenv.relation_ids')
+    @patch('charmhelpers.core.hookenv.related_units')
+    @patch('charmhelpers.core.hookenv.relation_for_unit')
     def test_gets_relations_for_id(self, relation_for_unit, related_units,
                                    relation_ids):
         relid = 123
@@ -298,9 +312,9 @@
             call('bar', relid),
         ])
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_ids')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.related_units')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_for_unit')
+    @patch('charmhelpers.core.hookenv.relation_ids')
+    @patch('charmhelpers.core.hookenv.related_units')
+    @patch('charmhelpers.core.hookenv.relation_for_unit')
     def test_gets_relations_for_specific_id(self, relation_for_unit,
                                             related_units, relation_ids):
         relid = 123
@@ -325,10 +339,10 @@
         ])
         self.assertFalse(relation_ids.called)
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.in_relation_hook')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_type')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relation_ids')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relations_for_id')
+    @patch('charmhelpers.core.hookenv.in_relation_hook')
+    @patch('charmhelpers.core.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.relation_ids')
+    @patch('charmhelpers.core.hookenv.relations_for_id')
     def test_gets_relations_for_type(self, relations_for_id, relation_ids,
                                      relation_type, in_relation_hook):
         reltype = 'foo-type'
@@ -366,27 +380,66 @@
             call(234),
         ])
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.config')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.local_unit')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.relations_of_type')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
-    def test_gets_execution_environment(self, os_, relations_of_type,
-                                        local_unit, config):
+    @patch('charmhelpers.core.hookenv.relation_types')
+    @patch('charmhelpers.core.hookenv.relation_ids')
+    @patch('charmhelpers.core.hookenv.related_units')
+    @patch('charmhelpers.core.hookenv.relation_get')
+    def test_gets_relations(self, relation_get, related_units,
+                            relation_ids, relation_types):
+        relation_types.return_value = ['t1','t2']
+        relation_ids.return_value = ['i1']
+        related_units.return_value = ['u1','u2']
+        relation_get.return_value = {'key': 'val'}
+
+        result = hookenv.relations()
+
+        self.assertEqual(result, {
+            't1': {
+                'i1': {
+                    'u1': {'key': 'val'},
+                    'u2': {'key': 'val'},
+                 },
+            },
+            't2': {
+                'i1': {
+                    'u1': {'key': 'val'},
+                    'u2': {'key': 'val'},
+                 },
+            },
+       })
+
+
+    @patch('charmhelpers.core.hookenv.config')
+    @patch('charmhelpers.core.hookenv.relation_type')
+    @patch('charmhelpers.core.hookenv.local_unit')
+    @patch('charmhelpers.core.hookenv.relation_id')
+    @patch('charmhelpers.core.hookenv.relations')
+    @patch('charmhelpers.core.hookenv.relation_get')
+    @patch('charmhelpers.core.hookenv.os')
+    def test_gets_execution_environment(self, os_, relations_get,
+                                        relations, relation_id, local_unit,
+                                        relation_type, config):
         config.return_value = 'some-config'
+        relation_type.return_value = 'some-type'
         local_unit.return_value = 'some-unit'
-        relations_of_type.return_value = 'some-relations'
+        relation_id.return_value = 'some-id'
+        relations.return_value = 'all-relations'
+        relations_get.return_value = 'some-relations'
         os_.environ = 'some-environment'
 
         result = hookenv.execution_environment()
 
         self.assertEqual(result, {
             'conf': 'some-config',
+            'reltype': 'some-type',
             'unit': 'some-unit',
+            'relid': 'some-id',
             'rel': 'some-relations',
+            'rels': 'all-relations',
             'env': 'some-environment',
         })
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_relation_id(self, os_):
         os_.environ = {
             'JUJU_RELATION_ID': 'foo',
@@ -394,7 +447,7 @@
 
         self.assertEqual(hookenv.relation_id(), 'foo')
 
-    @patch('charmhelpers.contrib.charmsupport.hookenv.os')
+    @patch('charmhelpers.core.hookenv.os')
     def test_relation_id_none_if_no_env(self, os_):
         os_.environ = {}
         self.assertEqual(hookenv.relation_id(), None)
@@ -407,7 +460,7 @@
         result = hookenv.relation_get()
 
         self.assertEqual(result['foo'], 'BAR')
-        check_output.assert_called_with(['relation-get', '--format=json'])
+        check_output.assert_called_with(['relation-get', '--format=json', '-'])
 
     @patch('subprocess.check_output')
     def test_gets_relation_with_scope(self, check_output):
@@ -443,6 +496,57 @@
         check_output.assert_called_with(['relation-get', '--format=json', '-r',
                                          123, 'baz-scope', 'baz-unit'])
 
+    @patch('subprocess.check_call')
+    def test_sets_relation(self, check_call_):
+        hookenv.relation_set(foo="bar")
+        check_call_.assert_called_with(['relation-set','foo=bar'])
+
+
+    @patch('subprocess.check_call')
+    def test_sets_relation_with_relation_id(self, check_call_):
+        hookenv.relation_set(relation_id="foo", bar="baz")
+        check_call_.assert_called_with(['relation-set', '-r', 'foo',
+                                         'bar=baz'])
+
+
+    def test_lists_relation_types(self):
+        open_ = mock_open()
+        open_.return_value = StringIO(CHARM_METADATA)
+        with patch('charmhelpers.core.hookenv.open', open_, create=True):
+            with patch.dict('os.environ', {'CHARM_DIR': '/var/empty'}):
+                reltypes = set(hookenv.relation_types())
+        open_.assert_called_once_with('/var/empty/metadata.yaml')
+        self.assertEqual(set(('testreqs','testprov','testpeer')), reltypes)
+
+
+    @patch('subprocess.check_call')
+    def test_opens_port(self, check_call_):
+        hookenv.open_port(443, "TCP")
+        hookenv.open_port(80)
+        hookenv.open_port(100, "UDP")
+        calls = [call(['open-port','443/TCP']),
+                 call(['open-port','80/TCP']),
+                 call(['open-port','100/UDP']),
+                ]
+        check_call_.assert_has_calls(calls)
+
+
+    @patch('subprocess.check_call')
+    def test_closes_port(self, check_call_):
+        hookenv.close_port(443, "TCP")
+        hookenv.close_port(80)
+        hookenv.close_port(100, "UDP")
+        calls = [call(['close-port','443/TCP']),
+                 call(['close-port','80/TCP']),
+                 call(['close-port','100/UDP']),
+                ]
+        check_call_.assert_has_calls(calls)
+
+    @patch('subprocess.check_output')
+    def test_gets_unit_attribute(self, check_output_):
+        hookenv.unit_get('foo')
+        check_output_.assert_called_with(['unit-get', 'foo'])
+
 
 class HooksTest(TestCase):
     def test_runs_a_registered_function(self):
@@ -488,3 +592,4 @@
         self.assertRaises(hookenv.UnregisteredHookError, hooks.execute,
                           ['brew'])
         self.assertEqual(execs, [True])
+
 
=== renamed file 'tests/contrib/charmsupport/test_host.py' => 'tests/core/test_host.py'
--- tests/contrib/charmsupport/test_host.py	2013-05-11 20:24:29 +0000
+++ tests/core/test_host.py	2013-05-23 20:50:31 +0000
@@ -5,7 +5,7 @@
 from mock import patch, call, MagicMock
 from testtools import TestCase
 
-from charmhelpers.contrib.charmsupport import host
+from charmhelpers.core import host
 
 
 MOUNT_LINES = ("""
@@ -52,7 +52,7 @@
         self.assertTrue(result)
         mock_call.assert_called_with(['initctl', action, service_name])
         exists.assert_has_calls([
-            call(os.path.join('/etc/init', service_name)),
+            call(os.path.join('/etc/init', '%s.conf' % service_name)),
         ])
 
     @patch('subprocess.call')
@@ -71,7 +71,6 @@
         mock_call.assert_called_with([os.path.join('/etc/init.d',
                                                    service_name), action])
         exists.assert_has_calls([
-            call(os.path.join('/etc/init', service_name)),
             call(os.path.join('/etc/init.d', service_name)),
         ])
 
 
=== added directory 'tests/payload'
=== added file 'tests/payload/__init__.py'
=== renamed file 'tests/contrib/charmsupport/test_execd.py' => 'tests/payload/test_execd.py'
--- tests/contrib/charmsupport/test_execd.py	2013-05-11 20:24:29 +0000
+++ tests/payload/test_execd.py	2013-05-23 20:50:31 +0000
@@ -2,10 +2,11 @@
 from mock import patch, call
 import os
 import shutil
+from subprocess import CalledProcessError
 
 from tempfile import mkdtemp
 
-from charmhelpers.contrib.charmsupport import execd
+from charmhelpers.payload import execd
 
 class ExecDBaseTestCase(TestCase):
     def setUp(self):
@@ -30,7 +31,7 @@
 
         self.assertEqual(expected, default_dir)
 
-    @patch('charmhelpers.contrib.charmsupport.execd.execd_run')
+    @patch('charmhelpers.payload.execd.execd_run')
     def test_execd_preinstall_calls_charm_pre_install(self, mock_execd_run):
         execd_dir = 'testdir'
         execd.execd_preinstall(execd_dir)
@@ -38,7 +39,7 @@
         mock_execd_run.assert_called_with(execd_dir, 'charm-pre-install')
 
 
-    @patch('charmhelpers.contrib.charmsupport.execd.default_execd_dir', return_value='foo')
+    @patch('charmhelpers.payload.execd.default_execd_dir', return_value='foo')
     @patch('os.listdir', return_value=['a','b','c'])
     @patch('os.path.isdir', return_value=True)
     def test_execd_module_list_from_env(self, mock_isdir, mock_listdir,
@@ -46,7 +47,7 @@
         module_names = ['a','b','c']
         mock_listdir.return_value = module_names
 
-        modules = list(execd.execd_modules())
+        modules = list(execd.execd_module_paths())
 
         expected = [os.path.join('foo', d) for d in module_names]
         self.assertEqual(modules, expected)
@@ -55,7 +56,7 @@
         mock_isdir.assert_has_calls([call(d) for d in expected])
 
 
-    @patch('charmhelpers.contrib.charmsupport.execd.default_execd_dir')
+    @patch('charmhelpers.payload.execd.default_execd_dir')
     @patch('os.listdir', return_value=['a','b','c'])
     @patch('os.path.isdir', return_value=True)
     def test_execd_module_list_with_dir(self, mock_isdir, mock_listdir,
@@ -63,7 +64,7 @@
         module_names = ['a','b','c']
         mock_listdir.return_value = module_names
 
-        modules = list(execd.execd_modules('foo'))
+        modules = list(execd.execd_module_paths('foo'))
 
         expected = [os.path.join('foo', d) for d in module_names]
         self.assertEqual(modules, expected)
@@ -73,16 +74,52 @@
         mock_isdir.assert_has_calls([call(d) for d in expected])
 
 
-    @patch('subprocess.check_call')
-    @patch('charmhelpers.contrib.charmsupport.hookenv.log')
     @patch('os.path.isfile', return_value=True)
     @patch('os.access', return_value=True)
-    @patch('charmhelpers.contrib.charmsupport.execd.execd_modules', return_value=['a','b'])
-    def test_execd_run(self, mock_modules, mock_access, mock_isfile,
-                       mock_log, mock_call):
-        submodule = 'charm-foo'
-
-        execd.execd_run(submodule)
-
-        paths = [os.path.join(m, submodule) for m in ('a','b')]
-        mock_call.assert_has_calls([call(d, shell=True) for d in paths])
+    @patch('charmhelpers.payload.execd.execd_module_paths')
+    def test_execd_submodule_list(self, modlist_, access_, isfile_):
+        module_list = ['a','b','c']
+        modlist_.return_value = module_list
+        submodules = [s for s in execd.execd_submodule_paths('sm')]
+
+        expected = [os.path.join(d, 'sm') for d in module_list]
+        self.assertEqual(submodules, expected)
+
+
+    @patch('subprocess.check_call')
+    @patch('charmhelpers.payload.execd.execd_submodule_paths')
+    def test_execd_run(self, submods_, call_):
+        submod_list = ['a','b','c']
+        submods_.return_value = submod_list
+        execd.execd_run('foo')
+
+        submods_.assert_called_with('foo', None)
+        call_.assert_has_calls([call(d, shell=True) for d in submod_list])
+
+
+    @patch('subprocess.check_call')
+    @patch('charmhelpers.payload.execd.execd_submodule_paths')
+    @patch('charmhelpers.core.hookenv.log')
+    def test_execd_run_logs_exception(self, log_, submods_, check_call_):
+        submod_list = ['a','b','c']
+        submods_.return_value = submod_list
+        err_msg = 'darn'
+        check_call_.side_effect = CalledProcessError(1, 'cmd', err_msg)
+
+        execd.execd_run('foo')
+        log_.assert_called_with(err_msg)
+
+
+    @patch('subprocess.check_call')
+    @patch('charmhelpers.payload.execd.execd_submodule_paths')
+    @patch('charmhelpers.core.hookenv.log')
+    @patch('sys.exit')
+    def test_execd_run_dies_with_return_code(self, exit_, log_, submods_,
+                                             check_call_):
+        submod_list = ['a','b','c']
+        submods_.return_value = submod_list
+        retcode = 9
+        check_call_.side_effect = CalledProcessError(retcode, 'cmd')
+
+        execd.execd_run('foo', die_on_error=True)
+        exit_.assert_called_with(retcode)