Conversation
@@ -94,7 +94,7 @@ def vmdk_is_a_descriptor(filepath): | |||
line = f.readline() | |||
return line.startswith('# Disk DescriptorFile') | |||
except: | |||
logging.warning("Failed to open %s for descriptor check", filepath) | |||
logging.exception("Failed to open %s for descriptor check", filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we aren't catching a specific exception any idea what message we get in this case? Can we catch a generic exception that will get logged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this specific one in PR description. What do you mean by catch generic exception? "except:" will cause any exception in this case rt?
import logging to vsan_policy.py log exceptions in find_child make golint happy by adding comment Log exceptions in ESX service
a66f509
to
5c488b9
Compare
LGTM |
@@ -87,6 +89,7 @@ def delete(name): | |||
try: | |||
os.remove(policy_path(name)) | |||
except: | |||
logging.exception("Failed to remove %s policy file", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure this one is needed , if the file does not exist silent fail is better. But it's good as is as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thought about it and decided to leave it there just in case for other OSErrors.
Thanks for taking on this ! LGTM |
LGTM |
Thanks everyone for quick review! |
Log exceptions to make debugging easier.
Testing Done:
Manually verified exceptions in log file for e.g.