-
Notifications
You must be signed in to change notification settings - Fork 48
Fix base exception message #264
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 base exception message #264
Conversation
|
So this is why this code try {
awsResponse = proxyClient.injectCredentialsAndInvokeV2(awsRequest, proxyClient.client()::deleteMetricFilter);
} catch (ResourceNotFoundException e) {
logger.log("Resource does not exist and could not be deleted.");
throw new CfnNotFoundException(e);
}causes this to be printed in the handler logs... |
src/main/java/software/amazon/cloudformation/exceptions/BaseHandlerException.java
Show resolved
Hide resolved
| super(ERROR_CODE.getMessage(), ERROR_CODE); | ||
| } | ||
|
|
||
| public CfnInvalidCredentialsException(final Throwable cause) { |
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 don't see any reason to limit all of these exceptions to just having a cause constructor. Can we include a (cause) and a (message, cause) for all of them?
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.
Good suggestion. However, I'm afraid it will be other large change to add all exception to support this new constructor as well as make the BaseHandlerException correct.
I would prefer to add this new constructor as new feature, but this PR will focus to fix this known bug.
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.
can we create an issue to resolve this then, if we're not going to include it in this PR?
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.
Tracked in #276
This reverts commit 599aa56.
Issue #, if available: #243
Description of changes:. Fix the error message issue for BaseHandlerException.
Cloudformation service doesn't like empty message exception, use the cause exception as default exception message.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.