Merge lp:~mew/charm-helpers/commandant into lp:charm-helpers

[Matthew Wedgwood]

Add CLI support to charm-helpers, allowing helpers to be used in charms written in other languages.

charmhelpers.cli adds a framework for building CLI commands from helpers. New commands are implemented by defining wrapper methods decorated with CommandLine.subcommand or CommandLine.subcommand_builder. Two examples from the hosts module are included.

[lp:~mew/charm-helpers/commandant]

30. By Matthew Wedgwood on 2013-08-01

Include CLI when charmhelpers is installed

[Nicola Larosa (teknico)]

All this looks well put together. The CommandLine class makes my head spin a little, but I guess it'll just take some getting accustomed to it. :-)

There is a bug in charmhelpers/cli/__init__.py . Line 137:

        positional_args = argspec.args[:len(argspec.defaults)]

should be:

        positional_args = argspec.args[:-len(argspec.defaults)]

It does not show up in the test_positional_arguments test in tests/cli/test_function_signature_analysis.py because the call to cli.describe_arguments has no default arguments, and the other ones only have one positional argument each.

Add "z=2" to the cli.describe_arguments call parameters in test_positional_arguments, and watch the "y" disappear from the test output. Add the minus sign above to the code, and watch the "y" appear again.

Getting one error when running tests, in tests.core.test_host.HelpersTest.test_installs_apt_packages_with_possible_errors . Same thing in trunk, though.

I also get this near the beginning of the test run:

    usage: nosetests [-h] [--format FMT | -r | -j | -p | -y | -c | -t]
                     {payload} ...
    nosetests: error: invalid choice: 'd' (choose from 'payload')

Add flake8 <https://pypi.python.org/pypi/flake8> to test requirements.

Fix lint errors as shown by "make lint".

Wrap lines in ReST docs, so that changes are readable in diffs.

review: Needs Fixing

[lp:~mew/charm-helpers/commandant]

31. By Matthew Wedgwood on 2013-08-21

Fixed indexing error in parameter inspection. Silenced parse_args output during tests.

Updated tests to catch indexing error in parameter inspection function, then
applied fix suggested by teknico. Also suppressed superfluous error message from
argparse during tests.

[Matthew Wedgwood (mew)]

Thanks for the review, and for catching that bug.

I've fixed the tests to catch the condition you spotted, then applied the fix. I've also suppressed the confusing output from parse_args during the invalid-args test.

review: Needs Resubmitting

[Nicola Larosa (teknico)]

Thank you. That's enough to approve: how about the other remarks, though?

Here's the error that also happens in trunk:

======================================================================
ERROR: tests.core.test_host.HelpersTest.test_installs_apt_packages_with_possible_errors
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/nl/canonical/Cloud/code/charm-helpers/commandant/tests/core/test_host.py", line 445, in test_installs_apt_packages_with_possible_errors
    fetch.apt_install(packages, options, fatal=True)
  File "/home/nl/canonical/Cloud/code/charm-helpers/commandant/charmhelpers/fetch/__init__.py", line 52, in apt_install
    options))
  File "/home/nl/canonical/Cloud/code/charm-helpers/commandant/charmhelpers/core/hookenv.py", line 65, in log
    subprocess.call(command)
  File "/usr/lib/python2.7/subprocess.py", line 524, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

And here's the lint output:

$ make lint
Checking for Python syntax...
charmhelpers/fetch/bzrurl.py:44:1: W391 blank line at end of file
charmhelpers/cli/commands.py:1:1: F401 'CommandLine' imported but unused
charmhelpers/cli/commands.py:2:1: F401 'host' imported but unused
charmhelpers/cli/host.py:10:1: E302 expected 2 blank lines, found 1
tests/fetch/test_bzrurl.py:45:5: E303 too many blank lines (2)
tests/fetch/test_bzrurl.py:54:5: E303 too many blank lines (2)
tests/fetch/test_bzrurl.py:69:5: E303 too many blank lines (2)
tests/fetch/test_bzrurl.py:80:1: W391 blank line at end of file
tests/core/test_host.py:451:5: E303 too many blank lines (2)
tests/core/test_host.py:457:78: E202 whitespace before ')'
tests/core/test_host.py:461:5: E303 too many blank lines (2)
tests/core/test_host.py:467:78: E202 whitespace before ')'
tests/core/test_host.py:471:5: E303 too many blank lines (2)
tests/core/test_host.py:482:5: E303 too many blank lines (2)
tests/core/test_host.py:494:5: E303 too many blank lines (2)
tests/contrib/hahelpers/test_ceph_utils.py:130:13: E128 continuation line under-indented for visual indent
tests/contrib/hahelpers/test_ceph_utils.py:131:1: W391 blank line at end of file
make: *** [lint] Error 1

Most of them are already in trunk, too.

review: Approve

[Matthew Wedgwood (mew)]

Ah, sorry. I've fixed the test failure in a separate MP (https://code.launchpad.net/~mew/charm-helpers/apt-test-fixes/+merge/181340, already merged)

I'll submit another for these lint cleanups. Thanks for being vigilant. I think we're getting close to a 1.0 release for charm-helpers, and at that point I'll loop in Tarmac to catch those issues in the future.

=== added file 'bin/chlp'
--- bin/chlp	1970-01-01 00:00:00 +0000
+++ bin/chlp	2013-08-21 21:41:57 +0000
@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+
+from charmhelpers.cli import cmdline
+from charmhelpers.cli.commands import *
+
+if __name__ == '__main__':
+    cmdline.run()

=== added directory 'charmhelpers/cli'
=== added file 'charmhelpers/cli/README.rst'
--- charmhelpers/cli/README.rst	1970-01-01 00:00:00 +0000
+++ charmhelpers/cli/README.rst	2013-08-21 21:41:57 +0000
@@ -0,0 +1,57 @@
+==========
+Commandant
+==========
+
+-----------------------------------------------------
+Automatic command-line interfaces to Python functions
+-----------------------------------------------------
+
+One of the benefits of ``libvirt`` is the uniformity of the interface: the C  API (as well as the bindings in other languages) is a set of functions that accept parameters that are nearly identical to the command-line arguments.  If you run ``virsh``, you get an interactive command prompt that supports all of the same commands that your shell scripts use as ``virsh`` subcommands.
+
+Command execution and stdio manipulation is the greatest common factor across all development systems in the POSIX environment.  By exposing your functions as commands that manipulate streams of text, you can make life easier for all the Ruby and Erlang and Go programmers in your life.
+
+Goals
+=====
+
+* Single decorator to expose a function as a command.
+  * now two decorators - one "automatic" and one that allows authors to manipulate the arguments for fine-grained control.(MW)
+* Automatic analysis of function signature through ``inspect.getargspec()``
+* Command argument parser built automatically with ``argparse``
+* Interactive interpreter loop object made with ``Cmd``
+* Options to output structured return value data via ``pprint``, ``yaml`` or ``json`` dumps.
+
+Other Important Features that need writing
+------------------------------------------
+
+* Help and Usage documentation can be automatically generated, but it will be important to let users override this behaviour
+* The decorator should allow specifying further parameters to the parser's add_argument() calls, to specify types or to make arguments behave as boolean flags, etc.
+    - Filename arguments are important, as good practice is for functions to accept file objects as parameters.
+    - choices arguments help to limit bad input before the function is called
+* Some automatic behaviour could make for better defaults, once the user can override them.
+    - We could automatically detect arguments that default to False or True, and automatically support --no-foo for foo=True.
+    - We could automatically support hyphens as alternates for underscores
+    - Arguments defaulting to sequence types could support the ``append`` action.
+
+
+-----------------------------------------------------
+Implementing subcommands
+-----------------------------------------------------
+
+(WIP)
+
+So as to avoid dependencies on the cli module, subcommands should be defined separately from their implementations. The recommmendation would be to place definitions into separate modules near the implementations which they expose.
+
+Some examples::
+
+    from charmhelpers.cli import CommandLine
+    from charmhelpers.payload import execd
+    from charmhelpers.foo import bar
+
+    cli = CommandLine()
+
+    cli.subcommand(execd.execd_run)
+
+    @cli.subcommand_builder("bar", help="Bar baz qux")
+    def barcmd_builder(subparser):
+        subparser.add_argument('argument1', help="yackety")
+        return bar

=== added file 'charmhelpers/cli/__init__.py'
--- charmhelpers/cli/__init__.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/cli/__init__.py	2013-08-21 21:41:57 +0000
@@ -0,0 +1,147 @@
+import inspect
+import itertools
+import argparse
+import sys
+
+
+class OutputFormatter(object):
+    def __init__(self, outfile=sys.stdout):
+        self.formats = (
+            "raw",
+            "json",
+            "py",
+            "yaml",
+            "csv",
+            "tab",
+        )
+        self.outfile = outfile
+
+    def add_arguments(self, argument_parser):
+        formatgroup = argument_parser.add_mutually_exclusive_group()
+        choices = self.supported_formats
+        formatgroup.add_argument("--format", metavar='FMT',
+                                 help="Select output format for returned data, "
+                                      "where FMT is one of: {}".format(choices),
+                                 choices=choices, default='raw')
+        for fmt in self.formats:
+            fmtfunc = getattr(self, fmt)
+            formatgroup.add_argument("-{}".format(fmt[0]),
+                                     "--{}".format(fmt), action='store_const',
+                                     const=fmt, dest='format',
+                                     help=fmtfunc.__doc__)
+
+    @property
+    def supported_formats(self):
+        return self.formats
+
+    def raw(self, output):
+        """Output data as raw string (default)"""
+        self.outfile.write(str(output))
+
+    def py(self, output):
+        """Output data as a nicely-formatted python data structure"""
+        import pprint
+        pprint.pprint(output, stream=self.outfile)
+
+    def json(self, output):
+        """Output data in JSON format"""
+        import json
+        json.dump(output, self.outfile)
+
+    def yaml(self, output):
+        """Output data in YAML format"""
+        import yaml
+        yaml.safe_dump(output, self.outfile)
+
+    def csv(self, output):
+        """Output data as excel-compatible CSV"""
+        import csv
+        csvwriter = csv.writer(self.outfile)
+        csvwriter.writerows(output)
+
+    def tab(self, output):
+        """Output data in excel-compatible tab-delimited format"""
+        import csv
+        csvwriter = csv.writer(self.outfile, dialect=csv.excel_tab)
+        csvwriter.writerows(output)
+
+    def format_output(self, output, fmt='raw'):
+        fmtfunc = getattr(self, fmt)
+        fmtfunc(output)
+
+
+class CommandLine(object):
+    argument_parser = None
+    subparsers = None
+    formatter = None
+
+    def __init__(self):
+        if not self.argument_parser:
+            self.argument_parser = argparse.ArgumentParser(description='Perform common charm tasks')
+        if not self.formatter:
+            self.formatter = OutputFormatter()
+            self.formatter.add_arguments(self.argument_parser)
+        if not self.subparsers:
+            self.subparsers = self.argument_parser.add_subparsers(help='Commands')
+
+    def subcommand(self, command_name=None):
+        """
+        Decorate a function as a subcommand. Use its arguments as the
+        command-line arguments"""
+        def wrapper(decorated):
+            cmd_name = command_name or decorated.__name__
+            subparser = self.subparsers.add_parser(cmd_name,
+                                                   description=decorated.__doc__)
+            for args, kwargs in describe_arguments(decorated):
+                subparser.add_argument(*args, **kwargs)
+            subparser.set_defaults(func=decorated)
+            return decorated
+        return wrapper
+
+    def subcommand_builder(self, command_name, description=None):
+        """
+        Decorate a function that builds a subcommand. Builders should accept a
+        single argument (the subparser instance) and return the function to be
+        run as the command."""
+        def wrapper(decorated):
+            subparser = self.subparsers.add_parser(command_name)
+            func = decorated(subparser)
+            subparser.set_defaults(func=func)
+            subparser.description = description or func.__doc__
+        return wrapper
+
+    def run(self):
+        "Run cli, processing arguments and executing subcommands."
+        arguments = self.argument_parser.parse_args()
+        argspec = inspect.getargspec(arguments.func)
+        vargs = []
+        kwargs = {}
+        if argspec.varargs:
+            vargs = getattr(arguments, argspec.varargs)
+        for arg in argspec.args:
+            kwargs[arg] = getattr(arguments, arg)
+        self.formatter.format_output(arguments.func(*vargs, **kwargs), arguments.format)
+
+
+cmdline = CommandLine()
+
+
+def describe_arguments(func):
+    """
+    Analyze a function's signature and return a data structure suitable for
+    passing in as arguments to an argparse parser's add_argument() method."""
+
+    argspec = inspect.getargspec(func)
+    # we should probably raise an exception somewhere if func includes **kwargs
+    if argspec.defaults:
+        positional_args = argspec.args[:-len(argspec.defaults)]
+        keyword_names = argspec.args[-len(argspec.defaults):]
+        for arg, default in itertools.izip(keyword_names, argspec.defaults):
+            yield ('--{}'.format(arg),), {'default': default}
+    else:
+        positional_args = argspec.args
+
+    for arg in positional_args:
+        yield (arg,), {}
+    if argspec.varargs:
+        yield (argspec.varargs,), {'nargs': '*'}

=== added file 'charmhelpers/cli/commands.py'
--- charmhelpers/cli/commands.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/cli/commands.py	2013-08-21 21:41:57 +0000
@@ -0,0 +1,2 @@
+from . import CommandLine
+import host

=== added file 'charmhelpers/cli/host.py'
--- charmhelpers/cli/host.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/cli/host.py	2013-08-21 21:41:57 +0000
@@ -0,0 +1,14 @@
+from . import cmdline
+from charmhelpers.core import host
+
+
[e-mail]()
+def mounts():
+    "List mounts"
+    return host.mounts()
+
[e-mail]_builder('service', description="Control system services")
+def service(subparser):
+    subparser.add_argument("action", help="The action to perform (start, stop, etc...)")
+    subparser.add_argument("service_name", help="Name of the service to control")
+    return host.service

=== modified file 'setup.py'
--- setup.py	2013-07-09 12:33:59 +0000
+++ setup.py	2013-08-21 21:41:57 +0000
@@ -16,6 +16,7 @@
     'url': "https://code.launchpad.net/charm-helpers",
     'packages': [
         "charmhelpers",
+        "charmhelpers.cli",
         "charmhelpers.core",
         "charmhelpers.fetch",
         "charmhelpers.payload",
@@ -28,6 +29,7 @@
         "charmhelpers.contrib.jujugui",
     ],
     'scripts': [
+        "bin/chlp",
         "bin/contrib/charmsupport/charmsupport",
         "bin/contrib/saltstack/salt-call",
     ],

=== added directory 'tests/cli'
=== added file 'tests/cli/__init__.py'
=== added file 'tests/cli/test_cmdline.py'
--- tests/cli/test_cmdline.py	1970-01-01 00:00:00 +0000
+++ tests/cli/test_cmdline.py	2013-08-21 21:41:57 +0000
@@ -0,0 +1,189 @@
+"""Tests for the commandant code that analyzes a function signature to
+determine the parameters to argparse."""
+
+from unittest import TestCase
+from mock import (
+    patch,
+    MagicMock,
+)
+try:
+    from cStringIO import StringIO
+except ImportError:
+    from StringIO import StringIO
+import json
+from pprint import pformat
+import yaml
+import csv
+
+from charmhelpers import cli
+
+
+class SubCommandTest(TestCase):
+    """Test creation of subcommands"""
+
+    def setUp(self):
+        super(SubCommandTest, self).setUp()
+        self.cl = cli.CommandLine()
+
+    @patch('sys.exit')
+    def test_subcommand_wrapper(self, _sys_exit):
+        """Test function name detection"""
+        @self.cl.subcommand()
+        def payload():
+            "A function that does work."
+            pass
+        args = self.cl.argument_parser.parse_args(['payload'])
+        self.assertEqual(args.func, payload)
+        self.assertEqual(_sys_exit.mock_calls, [])
+
+    @patch('sys.exit')
+    def test_subcommand_wrapper_bogus_arguments(self, _sys_exit):
+        """Test function name detection"""
+        @self.cl.subcommand()
+        def payload():
+            "A function that does work."
+            pass
+        with self.assertRaises(TypeError):
+            with patch("sys.argv", "tests deliberately bad input".split()):
+                with patch("sys.stderr"):
+                    self.cl.argument_parser.parse_args()
+        _sys_exit.assert_called_once_with(2)
+
+    @patch('sys.exit')
+    def test_subcommand_wrapper_cmdline_options(self, _sys_exit):
+        """Test detection of positional arguments and optional parameters."""
+        @self.cl.subcommand()
+        def payload(x, y=None):
+            "A function that does work."
+            return x
+        args = self.cl.argument_parser.parse_args(['payload', 'positional', '--y=optional'])
+        self.assertEqual(args.func, payload)
+        self.assertEqual(args.x, 'positional')
+        self.assertEqual(args.y, 'optional')
+        self.assertEqual(_sys_exit.mock_calls, [])
+
+    @patch('sys.exit')
+    def test_subcommand_builder(self, _sys_exit):
+        def noop(z):
+            pass
+
+        @self.cl.subcommand_builder('payload', description="A subcommand")
+        def payload_command(subparser):
+            subparser.add_argument('-z', action='store_true')
+            return noop
+
+        args = self.cl.argument_parser.parse_args(['payload', '-z'])
+        self.assertEqual(args.func, noop)
+        self.assertTrue(args.z)
+        self.assertFalse(_sys_exit.called)
+
+    def test_subcommand_builder_bogus_wrapped_args(self):
+        with self.assertRaises(TypeError):
+            @self.cl.subcommand_builder('payload', description="A subcommand")
+            def payload_command(subparser, otherarg):
+                pass
+
+    def test_run(self):
+        self.bar_called = False
+
+        @self.cl.subcommand()
+        def bar(x, y=None, *vargs):
+            "A function that does work."
+            self.bar_called = True
+            return "qux"
+
+        args = ['foo', 'bar', 'baz']
+        self.cl.formatter = MagicMock()
+        with patch("sys.argv", args):
+            self.cl.run()
+        self.assertTrue(self.bar_called)
+        self.assertTrue(self.cl.formatter.format_output.called)
+
+
+class OutputFormatterTest(TestCase):
+    def setUp(self):
+        super(OutputFormatterTest, self).setUp()
+        self.expected_formats = (
+            "raw",
+            "json",
+            "py",
+            "yaml",
+            "csv",
+            "tab",
+        )
+        self.outfile = StringIO()
+        self.of = cli.OutputFormatter(outfile=self.outfile)
+        self.output_data = {"this": "is", "some": 1, "data": dict()}
+
+    def test_supports_formats(self):
+        self.assertItemsEqual(self.expected_formats, self.of.supported_formats)
+
+    def test_adds_arguments(self):
+        ap = MagicMock()
+        arg_group = MagicMock()
+        add_arg = MagicMock()
+        arg_group.add_argument = add_arg
+        ap.add_mutually_exclusive_group.return_value = arg_group
+        self.of.add_arguments(ap)
+
+        self.assertTrue(add_arg.called)
+
+        for call_args in add_arg.call_args_list:
+            if "--format" in call_args[0]:
+                self.assertItemsEqual(call_args[1]['choices'], self.expected_formats)
+                self.assertEqual(call_args[1]['default'], 'raw')
+                break
+        else:
+            print arg_group.call_args_list
+            self.fail("No --format argument was created")
+
+        all_args = [c[0][0] for c in add_arg.call_args_list]
+        all_args.extend([c[0][1] for c in add_arg.call_args_list if len(c[0]) > 1])
+        for fmt in self.expected_formats:
+            self.assertIn("-{}".format(fmt[0]), all_args)
+            self.assertIn("--{}".format(fmt), all_args)
+
+    def test_outputs_raw(self):
+        self.of.raw(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), str(self.output_data))
+
+    def test_outputs_json(self):
+        self.of.json(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), json.dumps(self.output_data))
+
+    def test_outputs_py(self):
+        self.of.py(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), pformat(self.output_data) + "\n")
+
+    def test_outputs_yaml(self):
+        self.of.yaml(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), yaml.dump(self.output_data))
+
+    def test_outputs_csv(self):
+        sample = StringIO()
+        writer = csv.writer(sample)
+        writer.writerows(self.output_data)
+        sample.seek(0)
+        self.of.csv(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), sample.read())
+
+    def test_outputs_tab(self):
+        sample = StringIO()
+        writer = csv.writer(sample, dialect=csv.excel_tab)
+        writer.writerows(self.output_data)
+        sample.seek(0)
+        self.of.tab(self.output_data)
+        self.outfile.seek(0)
+        self.assertEqual(self.outfile.read(), sample.read())
+
+    def test_formats_output(self):
+        for format in self.expected_formats:
+            mock_f = MagicMock()
+            setattr(self.of, format, mock_f)
+            self.of.format_output(self.output_data, format)
+            mock_f.assert_called_with(self.output_data)

=== added file 'tests/cli/test_function_signature_analysis.py'
--- tests/cli/test_function_signature_analysis.py	1970-01-01 00:00:00 +0000
+++ tests/cli/test_function_signature_analysis.py	2013-08-21 21:41:57 +0000
@@ -0,0 +1,46 @@
+"""Tests for the commandant code that analyzes a function signature to
+determine the parameters to argparse."""
+
+from testtools import TestCase, matchers
+
+from charmhelpers import cli
+
+
+class FunctionSignatureTest(TestCase):
+    """Test a variety of function signatures."""
+
+    def test_positional_arguments(self):
+        """Finite number of order-dependent required arguments."""
+        argparams = tuple(cli.describe_arguments(lambda x, y, z: False))
+        self.assertEqual(3, len(argparams))
+        for argspec in ((('x',), {}), (('y',), {}), (('z',), {})):
+            self.assertIn(argspec, argparams)
+
+    def test_keyword_arguments(self):
+        """Function has optional parameters with default values."""
+        argparams = tuple(cli.describe_arguments(lambda x, y=3, z="bar": False))
+        self.assertEqual(3, len(argparams))
+        for argspec in ((('x',), {}),
+                        (('--y',), {"default": 3}),
+                        (('--z',), {"default": "bar"})):
+            self.assertIn(argspec, argparams)
+
+    def test_varargs(self):
+        """Function has a splat-operator parameter to catch an arbitrary number
+        of positional parameters."""
+        argparams = tuple(cli.describe_arguments(
+            lambda x, y=3, *z: False))
+        self.assertEqual(3, len(argparams))
+        for argspec in ((('x',), {}),
+                        (('--y',), {"default": 3}),
+                        (('z',), {"nargs": "*"})):
+            self.assertIn(argspec, argparams)
+
+    def test_keyword_splat_missing(self):
+        """Double-splat arguments can't be represented in the current version
+        of commandant."""
+        args = cli.describe_arguments(lambda x, y=3, *z, **missing: False)
+        for opts, _ in args:
+            # opts should be ('varname',) at this point
+            self.assertThat(opts, matchers.HasLength(1))
+            self.assertNotIn('missing', opts)