-
Notifications
You must be signed in to change notification settings - Fork 60
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
Fix the SecureVault bundle activator exception handling issue #55
Conversation
"secure vault configuration."))); | ||
logger.debug("Secure vault config successfully initialized"); | ||
} catch (Throwable throwable) { | ||
logger.error("Error occurred when initializing secure vault " + throwable.getMessage(), |
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.
Appending throwable.getMessage()
doesn't add any value. Exception message will be automatically printed by the logger when printing the throwable
. So let's remove that.
logger.error("Cannot initialize secure vault.", throwable);
} catch (Throwable throwable) { | ||
logger.error("Error occurred when initializing secure vault " + throwable.getMessage(), | ||
throwable); | ||
throw new SecureVaultException("Error occurred when initializing secure vault " + throwable.getMessage(), |
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.
Above applies here too.
throw new SecureVaultException("Cannot initialize secure vault.", throwable);
(secureVaultYAMLPath).orElseThrow(() -> new SecureVaultException("Error occurred when obtaining " + | ||
"secure vault configuration."))); | ||
logger.debug("Secure vault config successfully initialized"); | ||
} catch (Throwable throwable) { |
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.
Also, do we really need to catch Throwable
here. Any reason why Exception
(or any other exception) is not sufficient.
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.
Why we shouldn't catch Throwable
: https://stackoverflow.com/a/6083271/1577286
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.
Fixes apply: ff567a8
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.
Reg: Throwable, yes it may be best practise to catch the Exception only. But here the problem is there is issue in carbon logging framework where it coudn't catch the eclipse equinox frameowrk logs and becouse of the it may hidden the Throwable causes without logging to carbon log file.
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning