Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display stderr and stdout when run_command fails #40

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

benjamreis
Copy link
Contributor

@benjamreis benjamreis commented Sep 19, 2023

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

@benjamreis benjamreis force-pushed the fix-updater-error-handling branch 2 times, most recently from 4ef9e5a to 113c52e Compare September 19, 2023 09:12
Copy link
Contributor

@gthvn1 gthvn1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
after fixing the tests ;)

@benjamreis
Copy link
Contributor Author

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
Copy link
Member

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.

@benjamreis benjamreis force-pushed the fix-updater-error-handling branch 5 times, most recently from 6d09b2f to b0e56a3 Compare September 19, 2023 13:52
SOURCES/etc/xapi.d/plugins/xcpngutils/__init__.py Outdated Show resolved Hide resolved
tests/test_raid.py Outdated Show resolved Hide resolved
tests/test_raid.py Outdated Show resolved Hide resolved
@benjamreis benjamreis requested a review from stormi October 4, 2023 08:21
Copy link
Member

@stormi stormi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the last iteration of the PR was tested manually (especially the updater plugin)

@benjamreis benjamreis force-pushed the fix-updater-error-handling branch 2 times, most recently from 5cc3d89 to 657c6c3 Compare August 8, 2024 07:53
@ydirson
Copy link

ydirson commented Aug 8, 2024

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
Copy link
Contributor Author

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.

Comment on lines 66 to 70
class ProcessException(subprocess.CalledProcessError):
def __init__(self, code, command, stderr, stdout):
self.returncode = code
self.command = command
self.stderr = stderr
self.stdout = stdout
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should now rather use super.__init__ then

@benjamreis benjamreis requested a review from ydirson August 8, 2024 09:43
@benjamreis benjamreis force-pushed the fix-updater-error-handling branch 3 times, most recently from c1555d5 to e9168dc Compare August 8, 2024 10:02
Throwing a `CalledProcessError` would hide the
messages when converted into a `Xenapi.Failure`
use a generic `Exception` with a properly formed
detail instead.

Signed-off-by: BenjiReis <[email protected]>
Copy link

@ydirson ydirson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@benjamreis benjamreis merged commit 155fa07 into master Aug 8, 2024
4 checks passed
@benjamreis benjamreis deleted the fix-updater-error-handling branch August 8, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants