Merge lp:~lazypower/charm-helpers/revert-418 into lp:charm-helpers

[Charles Butler]

It was discussed to remove the work-around all together, and leave the current behavior as is, making hookenv dependent on Cli.

[Cory Johns (johnsca)]

+1 but it may be a good idea to send something to the mailing list or something to let people know of this new requirement.

review: Approve

[Matt Bruzek (mbruzek)]

Chuck made a good change here, but in my testing with a virtual environment I could not import charmhelpers.cli without first pip installing six and pyyaml. I think those should be added to the requirements file so we can install this from the directory.

[Cory Johns (johnsca)]

Install requirements addressed in https://code.launchpad.net/~johnsca/charm-helpers/contrib-setup-pypi/+merge/266903

[Cory Johns (johnsca)]

Ok, so here's a summary of the motivation for this and discussion so far, for reference:

Last week, I added some new helpers to hookenv that I wanted exposed via the CLI. Instead of creating thin wrappers around them under charmhelpers.cli, I imported and used the @cmdline.subcommand decorator directly in hookenv. This works fine if charmhelpers is installed via pip, but can break if it is partially sync'd in via charm-helpers-sync.yaml where core is included and cli is not.

I originally tried to make a quick work-around for it by making the import conditional, which was the commit that this was reverting. I apparently got a different import error message than was seen in the Open Stack charms, so the work-around didn't work. However, they added cli to their charm-helpers-sync.yaml and everything was fine for them w/o the work-around. Hence this revert.

However, it has been noted that charm-helpers-sync.yaml is (an unfortunately) common pattern, and most people expect sync'ing just core to work, which is entirely reasonable. So, we probably either need to re-institute the work-around and handle both import error messages, or follow the existing pattern of creating thin wrappers around the helpers to expose them via the CLI.

On the one hand, I'd prefer not to require thin wrappers, because I feel that it discourages exposing helpers via the CLI (as evinced by the dearth of helpers which are exposed). On the other hand, the conditional import is a little ugly.

Probably my ideal solution would be to split the cli package into a separate module, or even better, use an existing library such as begins (https://pypi.python.org/pypi/begins) cli_tools (https://pypi.python.org/pypi/cli_tools) or pyCLI (https://pypi.python.org/pypi/pyCLI).

[Matt Bruzek (mbruzek)]

After some discussion on this MP we decided to fix this problem in another one. I am going to close this MP to defer to the next more complete fix.

Unmerged revisions

420. By Charles Butler on 2015-08-04

Back out #418

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2015-07-31 19:43:43 +0000
+++ charmhelpers/core/hookenv.py	2015-08-04 15:48:21 +0000
@@ -34,22 +34,7 @@
 import tempfile
 from subprocess import CalledProcessError
 
-try:
-    from charmhelpers.cli import cmdline
-except ImportError as e:
-    # due to the anti-pattern of partially synching charmhelpers directly
-    # into charms, it's possible that charmhelpers.cli is not available;
-    # if that's the case, they don't really care about using the cli anyway,
-    # so mock it out
-    if str(e) == 'No module named cli':
-        class cmdline(object):
-            @classmethod
-            def subcommand(cls, *args, **kwargs):
-                def _wrap(func):
-                    return func
-                return _wrap
-    else:
-        raise
+from charmhelpers.cli import cmdline
 
 import six
 if not six.PY3: