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

Proposed by Nobuto Murata
Status: Merged
Merged at revision: 636
Proposed branch: lp:~nobuto/charm-helpers/execd
Merge into: lp:charm-helpers
Diff against target: 75 lines (+20/-17)
2 files modified
charmhelpers/payload/execd.py (+3/-2)
tests/payload/test_execd.py (+17/-15)
To merge this branch: bzr merge lp:~nobuto/charm-helpers/execd
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+306700@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

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
Revision history for this message
Nobuto Murata (nobuto) wrote :

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'")

Revision history for this message
Stuart Bishop (stub) wrote :

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).

Revision history for this message
Nobuto Murata (nobuto) wrote :

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

Revision history for this message
Stuart Bishop (stub) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/payload/execd.py'
2--- charmhelpers/payload/execd.py 2016-07-06 14:41:05 +0000
3+++ charmhelpers/payload/execd.py 2016-09-26 10:10:41 +0000
4@@ -47,11 +47,12 @@
5 yield path
6
7
8-def execd_run(command, execd_dir=None, die_on_error=False, stderr=None):
9+def execd_run(command, execd_dir=None, die_on_error=True, stderr=subprocess.STDOUT):
10 """Run command for each module within execd_dir which defines it."""
11 for submodule_path in execd_submodule_paths(command, execd_dir):
12 try:
13- subprocess.check_call(submodule_path, shell=True, stderr=stderr)
14+ subprocess.check_output(submodule_path, stderr=stderr,
15+ universal_newlines=True)
16 except subprocess.CalledProcessError as e:
17 hookenv.log("Error ({}) running {}. Output: {}".format(
18 e.returncode, e.cmd, e.output))
19
20=== modified file 'tests/payload/test_execd.py'
21--- tests/payload/test_execd.py 2014-11-24 11:58:25 +0000
22+++ tests/payload/test_execd.py 2016-09-26 10:10:41 +0000
23@@ -43,13 +43,17 @@
24 pre_install_success_path = os.path.join(module_path,
25 'charm-pre-install-success')
26 with open(charm_pre_install_path, 'w+') as f:
27- f.write("#!/bin/bash\n"
28- "/usr/bin/touch {}".format(pre_install_success_path))
29- perms = stat.S_IXUSR
30- # If the charm-pre-install should run without errors,
31+ if not error_on_preinstall:
32+ f.write("#!/bin/bash\n"
33+ "/usr/bin/touch {}".format(pre_install_success_path))
34+ else:
35+ f.write("#!/bin/bash\n"
36+ "echo stdout_from_pre_install\n"
37+ "echo stderr_from_pre_install >&2\n"
38+ "exit 1".format(pre_install_success_path))
39+
40 # ensure it is executable.
41- if not error_on_preinstall:
42- perms |= stat.S_IRUSR
43+ perms = stat.S_IRUSR + stat.S_IXUSR
44 os.chmod(charm_pre_install_path, perms)
45
46 def assert_preinstall_called_for_mod(self, module_dir,
47@@ -129,12 +133,12 @@
48 self.make_preinstall_executable(module_dir='basenode',
49 error_on_preinstall=True)
50
51- with open(os.devnull, 'wb') as devnull:
52- execd.execd_run('charm-pre-install', stderr=devnull)
53+ execd.execd_run('charm-pre-install', die_on_error=False)
54
55- expected_log = ('Error (126) running {}/exec.d/basenode/'
56- 'charm-pre-install. Output: None'.format(
57- self.test_charm_dir))
58+ expected_log = ('Error (1) running {}/exec.d/basenode/'
59+ 'charm-pre-install. Output: '
60+ 'stdout_from_pre_install\n'
61+ 'stderr_from_pre_install\n'.format(self.test_charm_dir))
62 log_.assert_called_with(expected_log)
63
64 @patch('charmhelpers.core.hookenv.log')
65@@ -144,8 +148,6 @@
66 error_on_preinstall=True)
67
68 with open(os.devnull, 'wb') as devnull:
69- execd.execd_run('charm-pre-install', die_on_error=True,
70- stderr=devnull)
71+ execd.execd_run('charm-pre-install', stderr=devnull)
72
73- # 126: Command invoked cannot execute
74- exit_.assert_called_with(126)
75+ exit_.assert_called_with(1)

Subscribers

People subscribed via source and target branches