Merge lp:~nobuto/charm-helpers/execd into lp:charm-helpers

[Nobuto Murata]

Don't ignore errors in execd_run and record error messages by default

As execd_run is a part of hook execution, it should fail when it has errors so that users can clearly notice the error and re-run the hook.

[Stuart Bishop (stub)]

Yes, its unfortunate how much 'ignore errors by default' is used in charmhelpers when that is never the right thing to do for charms.

I've got one minor inline comment. We need at least one of the tests to call execd_run without the die_on_error argument so we can ensure the default is set correctly.

Note that reactive charms have this functionality built in (lib/charms/layer/execd.py), and its behaviour matches what is proposed here.

review: Needs Fixing

[Nobuto Murata (nobuto)]

Reflected review comments, but I found the test is failing with Python 3. I don't know how to handle this, but will look into it.

======================================================================
FAIL: tests.payload.test_execd.ExecDTestCase.test_execd_run_logs_exception
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/home/nobuto/dev/charm-helpers_execd/.venv3/lib/python3.5/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/nobuto/dev/charm-helpers_execd/tests/payload/test_execd.py", line 142, in test_execd_run_logs_exception
    log_.assert_called_with(expected_log)
  File "/home/nobuto/dev/charm-helpers_execd/.venv3/lib/python3.5/site-packages/mock.py", line 835, in assert_called_with
    raise AssertionError(msg)
AssertionError: Expected call: log('Error (1) running /tmp/tmp11nk64mu/exec.d/basenode/charm-pre-install. Output: stdout_from_pre_install\nstderr_from_pre_install\n')
Actual call: log("Error (1) running /tmp/tmp11nk64mu/exec.d/basenode/charm-pre-install. Output: b'stdout_from_pre_install\\nstderr_from_pre_install\\n'")

[Stuart Bishop (stub)]

execd_run() is doing "subprocess.check_output(submodule_path, stderr=stderr)", which returns a byte string rather than text. Change that to "subprocess.check_output(submodule_path, stderr=stderr, universal_newlines=True)" or similar to have it return the output as a standard text string (unicode).

[Nobuto Murata (nobuto)]

Thanks for the kind comment. I have pushed the change.

[Stuart Bishop (stub)]

review: Approve

=== modified file 'charmhelpers/payload/execd.py'
--- charmhelpers/payload/execd.py	2016-07-06 14:41:05 +0000
+++ charmhelpers/payload/execd.py	2016-09-26 10:10:41 +0000
@@ -47,11 +47,12 @@
             yield path
 
 
-def execd_run(command, execd_dir=None, die_on_error=False, stderr=None):
+def execd_run(command, execd_dir=None, die_on_error=True, stderr=subprocess.STDOUT):
     """Run command for each module within execd_dir which defines it."""
     for submodule_path in execd_submodule_paths(command, execd_dir):
         try:
-            subprocess.check_call(submodule_path, shell=True, stderr=stderr)
+            subprocess.check_output(submodule_path, stderr=stderr,
+                                    universal_newlines=True)
         except subprocess.CalledProcessError as e:
             hookenv.log("Error ({}) running  {}. Output: {}".format(
                 e.returncode, e.cmd, e.output))
 
=== modified file 'tests/payload/test_execd.py'
--- tests/payload/test_execd.py	2014-11-24 11:58:25 +0000
+++ tests/payload/test_execd.py	2016-09-26 10:10:41 +0000
@@ -43,13 +43,17 @@
         pre_install_success_path = os.path.join(module_path,
                                                 'charm-pre-install-success')
         with open(charm_pre_install_path, 'w+') as f:
-            f.write("#!/bin/bash\n"
-                    "/usr/bin/touch {}".format(pre_install_success_path))
-        perms = stat.S_IXUSR
-        # If the charm-pre-install should run without errors,
+            if not error_on_preinstall:
+                f.write("#!/bin/bash\n"
+                        "/usr/bin/touch {}".format(pre_install_success_path))
+            else:
+                f.write("#!/bin/bash\n"
+                        "echo stdout_from_pre_install\n"
+                        "echo stderr_from_pre_install >&2\n"
+                        "exit 1".format(pre_install_success_path))
+
         # ensure it is executable.
-        if not error_on_preinstall:
-            perms |= stat.S_IRUSR
+        perms = stat.S_IRUSR + stat.S_IXUSR
         os.chmod(charm_pre_install_path, perms)
 
     def assert_preinstall_called_for_mod(self, module_dir,
@@ -129,12 +133,12 @@
         self.make_preinstall_executable(module_dir='basenode',
                                         error_on_preinstall=True)
 
-        with open(os.devnull, 'wb') as devnull:
-            execd.execd_run('charm-pre-install', stderr=devnull)
+        execd.execd_run('charm-pre-install', die_on_error=False)
 
-        expected_log = ('Error (126) running  {}/exec.d/basenode/'
-                        'charm-pre-install. Output: None'.format(
-                            self.test_charm_dir))
+        expected_log = ('Error (1) running  {}/exec.d/basenode/'
+                        'charm-pre-install. Output: '
+                        'stdout_from_pre_install\n'
+                        'stderr_from_pre_install\n'.format(self.test_charm_dir))
         log_.assert_called_with(expected_log)
 
     @patch('charmhelpers.core.hookenv.log')
@@ -144,8 +148,6 @@
                                         error_on_preinstall=True)
 
         with open(os.devnull, 'wb') as devnull:
-            execd.execd_run('charm-pre-install', die_on_error=True,
-                            stderr=devnull)
+            execd.execd_run('charm-pre-install', stderr=devnull)
 
-        # 126: Command invoked cannot execute
-        exit_.assert_called_with(126)
+        exit_.assert_called_with(1)