search

xcp-ng / xcp-ng-xapi-plugins

XCP-ng's specific XAPI plugins
GNU Affero General Public License v3.0
7 stars 9 forks source link

Display stderr and stdout when run_command fails #40

Closed benjamreis closed 2 months ago

benjamreis commented 1 year ago

Throwing a CalledProcessError would hide the messages when converted into a Xenapi.Failure use a generic Exception with a properly formed detail instead.

benjamreis commented 1 year ago

The failing test is no longer relevant with this modification. However before changing(removing) it I want to be sure the implemantation won't change. So i'll fix the test once the changes are validated.

Wescoeur commented 1 year ago

Honestly, I think that is better to add a custom Exception class (ProcessException?) with these fields: stdout, stderr, command and returncode. It can help to format by ourself the output in an except handler in specific plugins. Also you can add a __str__ method to format a generic message using these fields.

ydirson commented 2 months ago

It would be interesting to describe a bit more the original problem - is it that CalledProcessError does not contains enough information?

From what I see, in python3 we have all the info we need in CalledProcessError, but in 2.7 it misses the stderr field. But then for the py2 case we could still do this and avoid adding an extra class:

>>> e = subprocess.CalledProcessError(1, "foo", output="i say this")
>>> e.stderr = "i shout that"
>>>

Or at least we could make that new error class derive from CalledProcessError (just for the time being, until switch to py3) to avoid touching the other code catching CalledProcessError already.

benjamreis commented 2 months ago

CalledProcessError lacks the stderr in py2 which is still used for XCP-ng 8.2 and so we have to keep the code compatible with too. But yeah the inheritance reduce the diff of master which is good.