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

[Adam Gandelman]

charm_helpers_sync: Allows adding hints to charm sync config to selectively include non-.py files.

We'll (hopefully) soon be including a collection of common template files with the OpenStack helpers. I'd like to be able to sync these into my charms as part of my current workflow, using the charm_helpers_sync tool. This updates the script to allow passing additional config to sync sources, to include files that would currently be excluded from the sync.

[James Page (james-page)]

I think this looks OK - but I did notice that the tests for this tool are not running by default AFAICT.

Running:

python /usr/bin/nosetests --nologcapture tests/tools

Generates alot of output right now but the tests do pass.

review: Needs Information

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

49. By Adam Gandelman on 2013-07-16

setup.cfg: Include tools/ in coverage

[Adam Gandelman (gandelman-a)]

Hmm. Looks like they are running by default:

nosetests -v tests/
...
tests.payload.test_execd.ExecDTestCase.test_execd_submodule_list ... ok
It properly branches the correct helpers branch ... ok
It correctly finds the correct install path within a charm ... ok
It ensures all subdirectories of a parent are python importable ... ok
It extracts multiple options from an included item ... ok
...
Ran 324 tests in 3.461s (with tests/tools/)

vs.

Ran 306 tests in 1.600s (after removing tests/tools)

However, tools/ was not being included in the coverage report, though. I've updated setup.cfg to include it.

[Adam Gandelman (gandelman-a)]

review: Needs Resubmitting

[James Page (james-page)]

review: Approve

=== modified file 'setup.cfg'
--- setup.cfg	2013-05-11 20:24:29 +0000
+++ setup.cfg	2013-07-16 21:31:23 +0000
@@ -1,4 +1,4 @@
 [nosetests]
 with-coverage=1
 cover-erase=1
-cover-package=charmhelpers
+cover-package=charmhelpers,tools

=== modified file 'tests/tools/test_charm_helper_sync.py'
--- tests/tools/test_charm_helper_sync.py	2013-06-27 22:02:19 +0000
+++ tests/tools/test_charm_helper_sync.py	2013-07-16 21:31:23 +0000
@@ -87,10 +87,8 @@
         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'''
+    def _test_filter_dir(self, opts, isfile, isdir):
+        '''It filters non-python files and non-module dirs from source'''
         files = {
             'bad_file.bin': 'f',
             'some_dir': 'd',
@@ -114,12 +112,34 @@
 
         isfile.side_effect = _isfile
         isdir.side_effect = _isdir
-        result = sync._filter(dir='/tmp/charm-helpers/core',
-                              ls=files.iterkeys())
+        result = sync.get_filter(opts)(dir='/tmp/charm-helpers/core',
+                                       ls=files.iterkeys())
+        return result
+
+    @patch('os.path.isdir')
+    @patch('os.path.isfile')
+    def test_filter_dir_no_opts(self, isfile, isdir):
+        '''It filters out all non-py files by default'''
+        result = self._test_filter_dir(opts=None, isfile=isfile, isdir=isdir)
         ex = ['bad_file.bin', 'bad_file.img', 'some_dir']
         self.assertEquals(ex, result)
 
-    @patch('tools.charm_helpers_sync.charm_helpers_sync._filter')
+    @patch('os.path.isdir')
+    @patch('os.path.isfile')
+    def test_filter_dir_with_include(self, isfile, isdir):
+        '''It includes non-py files if specified as an include opt'''
+        result = self._test_filter_dir(opts=['inc=*.img'],
+                                       isfile=isfile, isdir=isdir)
+        ex = ['bad_file.bin', 'some_dir']
+        self.assertEquals(ex, result)
+
+    @patch('os.path.isdir')
+    @patch('os.path.isfile')
+    def test_filter_dir_include_all(self, isfile, isdir):
+        '''It does not filter anything if option specified to include all'''
+        self.assertEquals(sync.get_filter(opts=['inc=*']), None)
+
+    @patch('tools.charm_helpers_sync.charm_helpers_sync.get_filter')
     @patch('tools.charm_helpers_sync.charm_helpers_sync.ensure_init')
     @patch('shutil.copytree')
     @patch('shutil.rmtree')
@@ -127,12 +147,13 @@
     def test_sync_directory(self, exists, rmtree, copytree, ensure_init,
                             _filter):
         '''It correctly syncs src directory to dest directory'''
+        _filter.return_value = None
         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)
+                                    'hooks/charmhelpers/core', ignore=None)
         ensure_init.assert_called_with('hooks/charmhelpers/core')
 
     @patch('os.path.isfile')
@@ -154,7 +175,7 @@
 
         sync_dir.assert_called_with(
             '/tmp/charm-helpers/charmhelpers/contrib/openstack',
-            'hooks/charmhelpers/contrib/openstack')
+            'hooks/charmhelpers/contrib/openstack', None)
 
     @patch('tools.charm_helpers_sync.charm_helpers_sync.sync_pyfile')
     @patch('tools.charm_helpers_sync.charm_helpers_sync._is_pyfile')
@@ -192,6 +213,37 @@
         ]
 
         ex_calls = []
-        [ex_calls.append(call('/tmp/charm-helpers', 'hooks/charmhelpers', c))
-         for c in mods]
+        [ex_calls.append(
+            call('/tmp/charm-helpers', 'hooks/charmhelpers', c, [])
+        ) for c in mods]
         self.assertEquals(ex_calls, _sync.call_args_list)
+
+    def test_extract_option_no_globals(self):
+        '''It extracts option from an included item with no global options'''
+        inc = 'contrib.openstack.templates|inc=*.template'
+        result = sync.extract_options(inc)
+        ex = ('contrib.openstack.templates', ['inc=*.template'])
+        self.assertEquals(ex, result)
+
+    def test_extract_option_with_global_as_string(self):
+        '''It extracts option for include with global options as str'''
+        inc = 'contrib.openstack.templates|inc=*.template'
+        result = sync.extract_options(inc, global_options='inc=foo.*')
+        ex = ('contrib.openstack.templates',
+              ['inc=*.template', 'inc=foo.*'])
+        self.assertEquals(ex, result)
+
+    def test_extract_option_with_globals(self):
+        '''It extracts option from an included item with global options'''
+        inc = 'contrib.openstack.templates|inc=*.template'
+        result = sync.extract_options(inc, global_options=['inc=*.cfg'])
+        ex = ('contrib.openstack.templates', ['inc=*.template', 'inc=*.cfg'])
+        self.assertEquals(ex, result)
+
+    def test_extract_multiple_options_with_globals(self):
+        '''It extracts multiple options from an included item'''
+        inc = 'contrib.openstack.templates|inc=*.template,inc=foo.*'
+        result = sync.extract_options(inc, global_options=['inc=*.cfg'])
+        ex = ('contrib.openstack.templates',
+              ['inc=*.template', 'inc=foo.*', 'inc=*.cfg'])
+        self.assertEquals(ex, result)

=== modified file 'tools/charm_helpers_sync/README'
--- tools/charm_helpers_sync/README	2013-06-27 22:21:29 +0000
+++ tools/charm_helpers_sync/README	2013-07-16 21:31:23 +0000
@@ -74,6 +74,31 @@
 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.
 
+By default, only directories that look like python modules and associated
+.py source files will be synced.  If you need to include other files in
+the sync (for example, template files), you can add include hints to
+your config.  This can be done either on a per-module basis using standard
+UNIX filename patterns, eg:
+
+    destination: hooks/helpers
+    branch: lp:charm-helpers
+    include:
+        - core|inc=* # include all extra files from this module.
+        - contrib.openstack|inc=*.template # include only .template's
+        - contrib.hahelpers:
+            - ceph_utils|inc=*.cfg # include .cfg files
+
+Or globally for all included assets:
+
+    destination: hooks/helpers
+    branch: lp:charm-helpers
+    options: inc=*.template,inc=*.cfg # include .templates and .cfgs globally
+    include:
+        - core
+        - contrib.openstack
+        - contrib.hahelpers:
+            - ceph_utils
+
 You may also override configured destination directory and source bzr
 branch:
 

=== modified file 'tools/charm_helpers_sync/charm_helpers_sync.py'
--- tools/charm_helpers_sync/charm_helpers_sync.py	2013-06-27 22:02:19 +0000
+++ tools/charm_helpers_sync/charm_helpers_sync.py	2013-07-16 21:31:23 +0000
@@ -15,14 +15,18 @@
 import tempfile
 import yaml
 
+from fnmatch import fnmatch
+
 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))
@@ -30,31 +34,23 @@
     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
@@ -69,6 +65,7 @@
             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)
@@ -81,17 +78,51 @@
                     dest)
     ensure_init(dest)
 
-def sync_directory(src, dest):
+
+def get_filter(opts=None):
+    opts = opts or []
+    if 'inc=*' in opts:
+        # do not filter any files, include everything
+        return None
+
+    def _filter(dir, ls):
+        incs = [opt.split('=').pop() for opt in opts if 'inc=' in opt]
+        _filter = []
+        for f in ls:
+            _f = os.path.join(dir, f)
+
+            if not os.path.isdir(_f) and not _f.endswith('.py') and incs:
+                if True not in [fnmatch(_f, inc) for inc in incs]:
+                    logging.debug('Not syncing %s, does not match include '
+                                  'filters (%s)' % (_f, incs))
+                    _filter.append(f)
+                else:
+                    logging.debug('Including file, which matches include '
+                                  'filters (%s): %s' % (incs, _f))
+            elif (os.path.isfile(_f) and not _f.endswith('.py')):
+                logging.debug('Not syncing file: %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
+    return _filter
+
+
+def sync_directory(src, dest, opts=None):
     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)
+
+    shutil.copytree(src, dest, ignore=get_filter(opts))
     ensure_init(dest)
 
-def sync(src, dest, module):
+
+def sync(src, dest, module, opts=None):
     if os.path.isdir(_src_path(src, module)):
-        sync_directory(_src_path(src, module), _dest_path(dest, module))
+        sync_directory(_src_path(src, module), _dest_path(dest, module), opts)
     elif _is_pyfile(_src_path(src, module)):
         sync_pyfile(_src_path(src, module),
                     os.path.dirname(_dest_path(dest, module)))
@@ -99,18 +130,40 @@
         logging.warn('Could not sync: %s. Neither a pyfile or directory, '
                      'does it even exist?' % module)
 
-def sync_helpers(include, src, dest):
+
+def parse_sync_options(options):
+    if not options:
+        return []
+    return options.split(',')
+
+
+def extract_options(inc, global_options=None):
+    global_options = global_options or []
+    if global_options and isinstance(global_options, basestring):
+        global_options = [global_options]
+    if '|' not in inc:
+        return (inc, global_options)
+    inc, opts = inc.split('|')
+    return (inc, parse_sync_options(opts) + global_options)
+
+
+def sync_helpers(include, src, dest, options=None):
     if not os.path.isdir(dest):
         os.mkdir(dest)
 
+    global_options = parse_sync_options(options)
+
     for inc in include:
         if isinstance(inc, str):
-            sync(src, dest, inc)
+            inc, opts = extract_options(inc, global_options)
+            sync(src, dest, inc, opts)
         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]
+                    for m in v:
+                        inc, opts = extract_options(m, global_options)
+                        sync(src, dest, '%s.%s' % (k, inc), opts)
 
 if __name__ == '__main__':
     parser = optparse.OptionParser()
@@ -156,10 +209,14 @@
         config['include'] = []
         [config['include'].append(a) for a in args]
 
+    sync_options = None
+    if 'options' in config:
+        sync_options = config['options']
     tmpd = tempfile.mkdtemp()
     try:
         checkout = clone_helpers(tmpd, config['branch'])
-        sync_helpers(config['include'], checkout, config['destination'])
+        sync_helpers(config['include'], checkout, config['destination'],
+                     options=sync_options)
     except Exception, e:
         logging.error("Could not sync: %s" % e)
         raise e