Merge lp:~gandelman-a/charm-helpers/charm-helpers-sync into lp:charm-helpers

[Adam Gandelman]

This includes a little utility script for syncing parts of lp:charm-helpers into a local charm. It is meant to be used by charm authors while authoring a charm that depends on parts of charm-helpers. I've been using it heavily while working on a new charm lately, my workflow has been something like:

- Add a charm-helpers-sync.yaml to the top level of my charm, describing what I need included, pulling from upstream lp:charm-helpers.

- Realize I need new functionality in charm-helpers, create my own branch at lp:~gandelman-a/charm-helpers. Use this as a working branch alongisde my WIP charm. Commit changes there, point my charm-helpers-sync.yaml to that branch, sync them in as required.

- Propose lp:~gandelman-a/charm-helpers for merging to lp:charm-helpers. After its merged, update my charm-helpers-sync.yaml to point back to upstream. Re-sync, pulling in my changes and whatever else may have landed upstream.

It's worked well for me so far and really helps prevent me from having a heavily modified fork of charm-helpers embedded in my charm.

[Matthew Wedgwood (mew)]

Adam, this looks great, thanks for submitting it.

I'm a little unclear - is this intended for use within a charm or externally during development? If it's the former, I think we'd want to discourage folks from installing charm-helpers onto their units, especially with the amount of development on it. In either case, could you update the README to be a little more explicit about the use case?

The one other thing I'd love to see is tests. As it's outside the core package, I'd much rather get this into the tree now so I think it's OK to merge if you don't have the time for them.

Thanks!

review: Needs Information

[lp:~gandelman-a/charm-helpers/charm-helpers-sync]

33. By Adam Gandelman on 2013-06-27

Add tests, rename to charm_helpers_sync and make importable.

34. By Adam Gandelman on 2013-06-27

Update README with more a descriptive use case.

[Adam Gandelman (gandelman-a)]

Hey Matthew-

This is intended to be used by charm authors during their offline development, as a way of embeding a specific revision of a charm helpers branch into their charm. I've updated the README accordingly and added some tests. Needed to rename the script and its home to get it importable during tests.

Thanks.

review: Needs Resubmitting

[Matthew Wedgwood (mew)]

Thanks for the additions. +1

review: Approve

=== added directory 'tests/tools'
=== added file 'tests/tools/__init__.py'
=== added file 'tests/tools/test_charm_helper_sync.py'
--- tests/tools/test_charm_helper_sync.py	1970-01-01 00:00:00 +0000
+++ tests/tools/test_charm_helper_sync.py	2013-06-27 22:23:25 +0000
@@ -0,0 +1,197 @@
+import unittest
+from mock import call, patch
+import yaml
+
+import tools.charm_helpers_sync.charm_helpers_sync as sync
+
+INCLUDE = """
+include:
+    - core
+    - contrib.openstack
+    - contrib.storage
+    - contrib.hahelpers:
+        - utils
+        - ceph_utils
+        - cluster_utils
+        - haproxy_utils
+"""
+
+
+class HelperSyncTests(unittest.TestCase):
+    def test_clone_helpers(self):
+        '''It properly branches the correct helpers branch'''
+        with patch('subprocess.check_call') as check_call:
+            sync.clone_helpers(work_dir='/tmp/foo', branch='lp:charm-helpers')
+            check_call.assert_called_with(['bzr', 'branch',
+                                           'lp:charm-helpers',
+                                           '/tmp/foo/charm-helpers'])
+
+    def test_module_path(self):
+        '''It converts a python module path to a filesystem path'''
+        self.assertEquals(sync._module_path('some.test.module'),
+                          'some/test/module')
+
+    def test_src_path(self):
+        '''It renders the correct path to module within charm-helpers tree'''
+        path = sync._src_path(src='/tmp/charm-helpers',
+                              module='contrib.openstack')
+        self.assertEquals('/tmp/charm-helpers/charmhelpers/contrib/openstack',
+                          path)
+
+    def test_dest_path(self):
+        '''It correctly finds the correct install path within a charm'''
+        path = sync._dest_path(dest='/tmp/mycharm/hooks/charmhelpers',
+                               module='contrib.openstack')
+        self.assertEquals('/tmp/mycharm/hooks/charmhelpers/contrib/openstack',
+                          path)
+
+    @patch('__builtin__.open')
+    @patch('os.path.exists')
+    @patch('os.walk')
+    def test_ensure_init(self, walk, exists, _open):
+        '''It ensures all subdirectories of a parent are python importable'''
+        # os walk
+        # os.path.join
+        # os.path.exists
+        # open
+        def _walk(path):
+            yield ('/tmp/hooks/', ['helpers'], [])
+            yield ('/tmp/hooks/helpers', ['foo'], [])
+            yield ('/tmp/hooks/helpers/foo', [], [])
+        walk.side_effect = _walk
+        exists.return_value = False
+        sync.ensure_init('hooks/helpers/foo/')
+        ex = [call('/tmp/hooks/__init__.py', 'wb'),
+              call('/tmp/hooks/helpers/__init__.py', 'wb'),
+              call('/tmp/hooks/helpers/foo/__init__.py', 'wb')]
+        for c in ex:
+            self.assertIn(c, _open.call_args_list)
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.ensure_init')
+    @patch('os.path.isfile')
+    @patch('shutil.copy')
+    @patch('os.makedirs')
+    @patch('os.path.exists')
+    def test_sync_pyfile(self, exists, mkdirs, copy, isfile, ensure_init):
+        '''It correctly syncs a py src file from src to dest'''
+        exists.return_value = False
+        isfile.return_value = True
+        sync.sync_pyfile('/tmp/charm-helpers/core/host',
+                         'hooks/charmhelpers/core')
+        mkdirs.assert_called_with('hooks/charmhelpers/core')
+        copy_f = call('/tmp/charm-helpers/core/host.py',
+                      'hooks/charmhelpers/core')
+        copy_i = call('/tmp/charm-helpers/core/__init__.py',
+                      'hooks/charmhelpers/core')
+        self.assertIn(copy_f, copy.call_args_list)
+        self.assertIn(copy_i, copy.call_args_list)
+        ensure_init.assert_called_with('hooks/charmhelpers/core')
+
+    @patch('os.path.isdir')
+    @patch('os.path.isfile')
+    def test_filter_dir(self, isfile, isdir):
+        '''It filters non-python files and non-module dirs from sync source'''
+        files = {
+            'bad_file.bin': 'f',
+            'some_dir': 'd',
+            'good_helper.py': 'f',
+            'good_helper2.py': 'f',
+            'good_helper3.py': 'f',
+            'bad_file.img': 'f',
+        }
+
+        def _isfile(f):
+            try:
+                return files[f.split('/').pop()] == 'f'
+            except KeyError:
+                return False
+
+        def _isdir(f):
+            try:
+                return files[f.split('/').pop()] == 'd'
+            except KeyError:
+                return False
+
+        isfile.side_effect = _isfile
+        isdir.side_effect = _isdir
+        result = sync._filter(dir='/tmp/charm-helpers/core',
+                              ls=files.iterkeys())
+        ex = ['bad_file.bin', 'bad_file.img', 'some_dir']
+        self.assertEquals(ex, result)
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync._filter')
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.ensure_init')
+    @patch('shutil.copytree')
+    @patch('shutil.rmtree')
+    @patch('os.path.exists')
+    def test_sync_directory(self, exists, rmtree, copytree, ensure_init,
+                            _filter):
+        '''It correctly syncs src directory to dest directory'''
+        sync.sync_directory('/tmp/charm-helpers/charmhelpers/core',
+                            'hooks/charmhelpers/core')
+        exists.return_value = True
+        rmtree.assert_called_with('hooks/charmhelpers/core')
+        copytree.assert_called_with('/tmp/charm-helpers/charmhelpers/core',
+                                    'hooks/charmhelpers/core', ignore=_filter)
+        ensure_init.assert_called_with('hooks/charmhelpers/core')
+
+    @patch('os.path.isfile')
+    def test_is_pyfile(self, isfile):
+        '''It correctly identifies incomplete path to a py src file as such'''
+        sync._is_pyfile('/tmp/charm-helpers/charmhelpers/core/host')
+        isfile.assert_called_with(
+            '/tmp/charm-helpers/charmhelpers/core/host.py'
+        )
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.sync_directory')
+    @patch('os.path.isdir')
+    def test_syncs_directory(self, is_dir, sync_dir):
+        '''It correctly syncs a module directory'''
+        is_dir.return_value = True
+        sync.sync(src='/tmp/charm-helpers',
+                  dest='hooks/charmhelpers',
+                  module='contrib.openstack')
+
+        sync_dir.assert_called_with(
+            '/tmp/charm-helpers/charmhelpers/contrib/openstack',
+            'hooks/charmhelpers/contrib/openstack')
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.sync_pyfile')
+    @patch('tools.charm_helpers_sync.charm_helpers_sync._is_pyfile')
+    @patch('os.path.isdir')
+    def test_syncs_file(self, is_dir, is_pyfile, sync_pyfile):
+        '''It correctly syncs a module file'''
+        is_dir.return_value = False
+        is_pyfile.return_value = True
+        sync.sync(src='/tmp/charm-helpers',
+                  dest='hooks/charmhelpers',
+                  module='contrib.openstack.utils')
+
+        sync_pyfile.assert_called_with(
+            '/tmp/charm-helpers/charmhelpers/contrib/openstack/utils',
+            'hooks/charmhelpers/contrib/openstack')
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.sync')
+    @patch('os.path.isdir')
+    def test_sync_helpers_from_config(self, isdir, _sync):
+        '''It correctly syncs a list of included helpers'''
+        include = yaml.load(INCLUDE)['include']
+        isdir.return_value = True
+        sync.sync_helpers(include=include,
+                          src='/tmp/charm-helpers',
+
+                          dest='hooks/charmhelpers')
+        mods = [
+            'core',
+            'contrib.openstack',
+            'contrib.storage',
+            'contrib.hahelpers.utils',
+            'contrib.hahelpers.ceph_utils',
+            'contrib.hahelpers.cluster_utils',
+            'contrib.hahelpers.haproxy_utils'
+        ]
+
+        ex_calls = []
+        [ex_calls.append(call('/tmp/charm-helpers', 'hooks/charmhelpers', c))
+         for c in mods]
+        self.assertEquals(ex_calls, _sync.call_args_list)

=== added directory 'tools'
=== added file 'tools/__init__.py'
=== added directory 'tools/charm_helpers_sync'
=== added file 'tools/charm_helpers_sync/README'
--- tools/charm_helpers_sync/README	1970-01-01 00:00:00 +0000
+++ tools/charm_helpers_sync/README	2013-06-27 22:23:25 +0000
@@ -0,0 +1,89 @@
+Script for synchronizing charm-helpers into a charm branch.
+
+This script is intended to be used by charm authors during the development
+of their charm.  It allows authors to pull in bits of a charm-helpers source
+tree and embed directly into their charm, to be deployed with the rest of
+their hooks and charm payload.  This script is not intended to be called
+by the hooks themselves, but instead by the charm author while they are
+hacking on a charm offline.  Consider it a method of compiling specific
+revision of a charm-helpers branch into a given charm source tree.
+
+Some goals and benefits to using a sync tool to manage this process:
+
+    - Reduces the burden of manually copying in upstream charm helpers code
+      into a charm and helps ensure we can easily keep a specific charm's
+      helper code up to date.
+
+    - Allows authors to hack on their own working branch of charm-helpers,
+      easily sync into their WIP charm.  Any changes they've made to charm
+      helpers can be upstreamed via a merge of their charm-helpers branch
+      into lp:charm-helpers, ideally at the same time they are upstreaming
+      the charm itself into the charm store.  Separating charm helper
+      development from charm development can help reduce cases where charms
+      are shipping locally modified helpers.
+
+    - Avoids the need to ship the *entire* lp:charm-helpers source tree with
+      a charm.  Authors can selectively pick and choose what subset of helpers
+      to include to satisfy the goals of their charm.
+
+Loosely based on OpenStack's oslo-incubator:
+
+    https://github.com/openstack/oslo-incubator.git
+
+Allows specifying a list of dependencies to sync in from a charm-helpers
+branch.  Ideally, each charm should describe its requirements in a yaml
+config included in the charm, eg charm-helpers.yaml (NOTE: Example module
+layout as of 05/30/2013):
+
+    $ cd my-charm
+    $ cat >charm-helpers.yaml <<END
+    destination: hooks/helpers
+    branch: lp:charm-helpers
+    include:
+        - core
+        - contrib.openstack
+        - contrib.hahelpers:
+            - ceph_utils
+    END
+
+includes may be defined as entire module sub-directories, or as invidual
+.py files with in a module sub-directory.
+
+Charm author can then sync in and update helpers as needed.  The following
+import all of charmhelpers.core + charmhelpers.contrib.openstack, and only
+ceph_utils.py from charmhelpers.contrib.hahelpers:
+
+    $ charm-helper-sync -c charm-helpers.yaml
+    $ find hooks/helpers/
+    hooks/helpers/
+    hooks/helpers/contrib
+    hooks/helpers/contrib/openstack
+    hooks/helpers/contrib/openstack/openstack_utils.py
+    hooks/helpers/contrib/openstack/__init__.py
+    hooks/helpers/contrib/hahelpers
+    hooks/helpers/contrib/hahelpers/ceph_utils.py
+    hooks/helpers/contrib/hahelpers/__init__.py
+    hooks/helpers/contrib/__init__.py
+    hooks/helpers/core
+    hooks/helpers/core/hookenv.py
+    hooks/helpers/core/host.py
+    hooks/helpers/core/__init__.py
+    hooks/helpers/__init__.py
+
+
+Script will create missing __init__.py's to ensure each subdirectory is
+importable, assuming the script is run from the charm's top-level directory.
+
+You may also override configured destination directory and source bzr
+branch:
+
+    $ charm-helper-sync -b ~/src/bzr/charm-helpers-dev \
+            -d hooks/helpers-test \
+            -c charm-helpers.yaml
+
+Or not use a config file at all:
+    $ charm-helper-sync -b lp:~gandelman-a/charm-helpers/fixes \
+            -d hooks/helpers core contrib.openstack contrib.hahelpers
+
+Script will create missing __init__.py's to ensure each subdirectory is
+importable, assuming the script is run from the charm's top-level directory.

=== added file 'tools/charm_helpers_sync/__init__.py'
=== added file 'tools/charm_helpers_sync/charm_helpers_sync.py'
--- tools/charm_helpers_sync/charm_helpers_sync.py	1970-01-01 00:00:00 +0000
+++ tools/charm_helpers_sync/charm_helpers_sync.py	2013-06-27 22:23:25 +0000
@@ -0,0 +1,168 @@
+#!/usr/bin/python
+#
+# Copyright 2013 Canonical Ltd.
+
+# Authors:
+#   Adam Gandelman <[e-mail]>
+#
+
+import logging
+import optparse
+import os
+import subprocess
+import shutil
+import sys
+import tempfile
+import yaml
+
+CHARM_HELPERS_BRANCH = 'lp:charm-helpers'
+
+def parse_config(conf_file):
+    if not os.path.isfile(conf_file):
+        logging.error('Invalid config file: %s.' % conf_file)
+        return False
+    return yaml.load(open(conf_file).read())
+
+def clone_helpers(work_dir, branch):
+    dest = os.path.join(work_dir, 'charm-helpers')
+    logging.info('Checking out %s to %s.' % (branch, dest))
+    cmd = ['bzr', 'branch', branch, dest]
+    subprocess.check_call(cmd)
+    return dest
+
+def _filter(dir, ls):
+    _filter = []
+    for f in ls:
+        _f = os.path.join(dir, f)
+        if (os.path.isfile(_f) and not _f.endswith('.py')):
+            logging.debug('Not syncing files: %s' % f)
+            _filter.append(f)
+        elif (os.path.isdir(_f) and not
+              os.path.isfile(os.path.join(_f, '__init__.py'))):
+            logging.debug('Not syncing directory: %s' % f)
+            _filter.append(f)
+    return _filter
+
+def _module_path(module):
+    return os.path.join(*module.split('.'))
+
+def _src_path(src, module):
+    return os.path.join(src, 'charmhelpers', _module_path(module))
+
+def _dest_path(dest, module):
+    return os.path.join(dest, _module_path(module))
+
+def _is_pyfile(path):
+    return os.path.isfile(path + '.py')
+
+def ensure_init(path):
+    '''
+    ensure directories leading up to path are importable, omitting
+    parent directory, eg path='/hooks/helpers/foo'/:
+        hooks/
+        hooks/helpers/__init__.py
+        hooks/helpers/foo/__init__.py
+    '''
+    for d, dirs, files in os.walk(os.path.join(*path.split('/')[:2])):
+        _i = os.path.join(d, '__init__.py')
+        if not os.path.exists(_i):
+            logging.info('Adding missing __init__.py: %s' % _i)
+            open(_i, 'wb').close()
+
+def sync_pyfile(src, dest):
+    src = src + '.py'
+    src_dir = os.path.dirname(src)
+    logging.info('Syncing pyfile: %s -> %s.' % (src, dest))
+    if not os.path.exists(dest):
+        os.makedirs(dest)
+    shutil.copy(src, dest)
+    if os.path.isfile(os.path.join(src_dir, '__init__.py')):
+        shutil.copy(os.path.join(src_dir, '__init__.py'),
+                    dest)
+    ensure_init(dest)
+
+def sync_directory(src, dest):
+    if os.path.exists(dest):
+        logging.debug('Removing existing directory: %s' % dest)
+        shutil.rmtree(dest)
+    logging.info('Syncing directory: %s -> %s.' % (src, dest))
+    shutil.copytree(src, dest, ignore=_filter)
+    ensure_init(dest)
+
+def sync(src, dest, module):
+    if os.path.isdir(_src_path(src, module)):
+        sync_directory(_src_path(src, module), _dest_path(dest, module))
+    elif _is_pyfile(_src_path(src, module)):
+        sync_pyfile(_src_path(src, module),
+                    os.path.dirname(_dest_path(dest, module)))
+    else:
+        logging.warn('Could not sync: %s. Neither a pyfile or directory, '
+                     'does it even exist?' % module)
+
+def sync_helpers(include, src, dest):
+    if not os.path.isdir(dest):
+        os.mkdir(dest)
+
+    for inc in include:
+        if isinstance(inc, str):
+            sync(src, dest, inc)
+        elif isinstance(inc, dict):
+            # could also do nested dicts here.
+            for k, v in inc.iteritems():
+                if isinstance(v, list):
+                    [sync(src, dest, '%s.%s' % (k, m)) for m in v]
+
+if __name__ == '__main__':
+    parser = optparse.OptionParser()
+    parser.add_option('-c', '--config', action='store', dest='config',
+                      default=None, help='helper config file')
+    parser.add_option('-D', '--debug', action='store_true', dest='debug',
+                      default=False, help='debug')
+    parser.add_option('-b', '--branch', action='store', dest='branch',
+                      help='charm-helpers bzr branch (overrides config)')
+    parser.add_option('-d', '--destination', action='store', dest='dest_dir',
+                      help='sync destination dir (overrides config)')
+    (opts, args) = parser.parse_args()
+
+    if opts.debug:
+        logging.basicConfig(level=logging.DEBUG)
+    else:
+        logging.basicConfig(level=logging.INFO)
+
+    if opts.config:
+        logging.info('Loading charm helper config from %s.' % opts.config)
+        config = parse_config(opts.config)
+        if not config:
+            logging.error('Could not parse config from %s.' % opts.config)
+            sys.exit(1)
+    else:
+        config = {}
+
+    if 'branch' not in config:
+        config['branch'] = CHARM_HELPERS_BRANCH
+    if opts.branch:
+        config['branch'] = opts.branch
+    if opts.dest_dir:
+        config['destination'] = opts.dest_dir
+
+    if 'destination' not in config:
+        logging.error('No destination dir. specified as option or config.')
+        sys.exit(1)
+
+    if 'include' not in config:
+        if not args:
+            logging.error('No modules to sync specified as option or config.')
+            sys.exit(1)
+        config['include'] = []
+        [config['include'].append(a) for a in args]
+
+    tmpd = tempfile.mkdtemp()
+    try:
+        checkout = clone_helpers(tmpd, config['branch'])
+        sync_helpers(config['include'], checkout, config['destination'])
+    except Exception, e:
+        logging.error("Could not sync: %s" % e)
+        raise e
+    finally:
+        logging.debug('Cleaning up %s' % tmpd)
+        shutil.rmtree(tmpd)

=== added file 'tools/charm_helpers_sync/example-config.yaml'
--- tools/charm_helpers_sync/example-config.yaml	1970-01-01 00:00:00 +0000
+++ tools/charm_helpers_sync/example-config.yaml	2013-06-27 22:23:25 +0000
@@ -0,0 +1,14 @@
+# Import from remote bzr branch.
+branch: lp:charm-helpers
+# install helpers to ./hooks/charmhelpers
+destination: hooks/charmhelpers
+include:
+    # include all of charmhelpers.core
+    - core
+    # all of charmhelpers.payload
+    - payload
+    # and a subset of charmhelpers.contrib.hahelpers
+    - contrib.hahelpers:
+        - openstack_common
+        - ceph_utils
+        - utils