-
Notifications
You must be signed in to change notification settings - Fork 36
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
Additional AWS error functions #37
Additional AWS error functions #37
Conversation
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.
We had some ideas about renaming
awserr.go
Outdated
// IsAWSErrCode returns true if the error matches all these conditions: | ||
// * err is of type awserr.Error | ||
// * Error.Code() equals code | ||
func IsAWSErrCode(err error, code string) bool { |
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.
We did some workshopping on the functions, since some of the names like IsAWSErrCodeMessageContains()
didn't read smoothly.
We came up with moving them to a package tfawserr
and the following renaming:
IsAWSErrCode()
toErrCodeEquals()
IsAWSErrCodeContains()
toErrCodeContains()
IsAWSErrCodeMessageContains()
toErrMessageContains()
IsAWSErrRequestFailureStatusCode()
toErrStatusCodeEquals()
What do you think?
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.
Makes sense.
Do we still want to keep IsAWSErr
in the top-level package? Removing it would be a breaking change.
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.
We should probably leave it for backwards compatibility, even though we're pre-1.0 :)
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 feel comfortable removing IsAWSErr
as the only usage I can find external to this repo is in the AWS Provider.
And that's a single call.
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.
Please feel free to making breaking Go API changes in this library -- its noted in the README that its only intended for usage by the Terraform S3 Backend and Terraform AWS Provider and each of those should have a small usage footprint currently. 👍
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.
Have just done that 😄, with appropriate CHANGELOG entries.
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.
LGTM 🚀
Two cases where the error check can be simplified
Community Note
Reference: hashicorp/terraform-provider-aws#13036.
Additional AWS error code functions used within the AWS Provider to be migrated here so that they can be used in other contexts.
Relevant unit tests: