Code review comment for lp:~gz/maas/detailed_poweractionfail_1155175

Thanks for the branch Martin, a definite improvement. I am a little concerned about one of the changes in [2] below so marking needs-fixing for that.

Cheers.

[1]

53 + output = subprocess.check_output(
54 + commands, shell=True, stderr=subprocess.STDOUT, close_fds=True)
55 + except Exception as e:

Catching a bare Exception is bad form, can you be more specific? The original caught OSError.

[2]

56 + raise PowerActionFail(self, e)
57 + # This output is only examined in tests, execute just ignores it
58 + return output

You've changed this from a PIPE to using the parent's FDs. I wrote the original code and although I can't remember why I used a PIPE I am pretty sure I had a good reason, so I reckon you should change it back to PIPE. I have a sneaking suspicion that some of the scripts caused a hang or something weird in the output from the celery process.
I should have added a comment about it :/

[3]

Our coding standards say to add a line break immediately after an opening parenthesis, so can you fix the following bits please:

86 + self.assertThat(lambda: pa.render_template(template),
87 + Raises(MatchesException(PowerActionFail,
88 + ".*name 'mac' is not defined at line \d+ column \d+ "
89 + "in file %s" % re.escape(template_name))))

snip....

95 def test_execute_raises_PowerActionFail_when_script_fails(self):
96 path = self._create_template_file("this_is_not_valid_shell")
97 - exception = self.assertRaises(PowerActionFail, self.run_action, path)
98 - self.assertEqual(
99 - "ether_wake failed with return code 127", exception.message)
100 + self.assertThat(lambda: self.run_action(path),
101 + Raises(MatchesException(PowerActionFail,
102 + "ether_wake failed.* return.* 127")))
103 +
104 + def test_execute_raises_PowerActionFail_with_output(self):
105 + path = self._create_template_file("echo reason for failure; exit 1")
106 + self.assertThat(lambda: self.run_action(path),
107 + Raises(MatchesException(PowerActionFail, ".*:\nreason for failure")))