Low/No Disk space caused the Agent to crash and not recover.

[truthbk]

It seems like a low/no disk space can cause the TemporaryFile calls made in subprocess_output to "fail" - and by fail we mean empty output that can lead to IndexErrors, etc, when parsing the output. In general we have to be ready to expect an empty string as output.

[JohnLZeller]

It seems like the default case when calling get_subprocess_output() is that we expect output. I think we should invert the option here.

[truthbk]

@JohnLZeller I agree that we normally do use it to run commands where we want an output, but from a library point of view - and this being a library - I don't really want to enforce that kind of behavior were empty output is automatically akin to an error.

[truthbk]

@JohnLZeller actually nevermind.... you're right, the function is get_subprocess_output after all!

[JohnLZeller]

nit: could change output == "" to not output. That way it'll catch "" and None

[truthbk]

I think read() always returns something... but I agree None is more pythonic.

[JohnLZeller]

Yeah, you're right :) definitely a nit

[LeoCavaille]

don't know if maybe we should .strip() before testing if the string is empty, some rude programs sometimes output newlines for instance.

[JohnLZeller]

Looking pretty good. I wonder if it'd be worth defining a small Exception class, and raising that. This way we can catch a specific exception rather than all of them.

Maybe something like this:
class SubprocessOutputEmptyError(Exception): pass

Then you can just:
raise SubprocessOutputEmptyError('get_subprocess_output expected output but had none.')

And later can catch SubprocessOutputEmptyError

[LeoCavaille]

not your code, but seems like this could be written as:

if Platform.is_linux(platf):
...
elif Platform.is_darwin(platf) or Platform.is_freebsd(platf):
...

[JohnLZeller]

Ditto, Could we do a catch SubprocessOutputEmptyError: here instead?

[JohnLZeller]

LGTM, other than the notes 👍