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.

Sign up for GitHub

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

Jump to bottom

service/route53: Add batched error support for ChangeResourceRecordSets #593

Merged
merged 3 commits into from
Mar 16, 2016
Merged

service/route53: Add batched error support for ChangeResourceRecordSets #593

merged 3 commits into from
Mar 16, 2016

Conversation

jasdel
Copy link
Contributor

@jasdel jasdel commented Mar 15, 2016

service/route53: Add batched error support for ChangeResourceRecordSets
Adds support for batched errors that ChangeResourceRecordSets can
optionally return.

Replaces PR #438

Also deprecate BatchError in favor of BatchedErrors
BatchError did not satisfy the awserr.Error interface like it should of. This
update adds a new type BatchedErrors which satisfies awserr.Error.

@jasdel jasdel mentioned this pull request Mar 15, 2016
Closed
xibz
xibz reviewed Mar 16, 2016
View reviewed changes
aws/awserr/types.go
// needed, use BatchError
// OrigErr returns the original error if one was set. Nil is returned if no
// error was set. This only returns the first element in the list. If the full
//list is needed, use BatchedErrors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd spacing on comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks will update that.

@xibz
Copy link
Contributor

xibz commented Mar 16, 2016

Was the BatchedErrors.OrigError() not going to concatenate the error strings? Or just return the first one in the list?

@xibz
Copy link
Contributor

xibz commented Mar 16, 2016

Should probably add a leak test to the unit tests

@jasdel
Copy link
Contributor Author

jasdel commented Mar 16, 2016

I was considering how best to represent BatchedErrors.OrigError() other than just another batched error. I'm thinking as a error type which also has a OrigErrs method. What do you think.

For the leak test, not sure what to test there, there are no io.Read/Writers nor channels. What were you thinking should be tested?

@xibz
Copy link
Contributor

xibz commented Mar 16, 2016

Yea, that makes sense, was just curious because it seems there would be more use in OrigErrs than in the OrigErr. So, I was thinking of concate the error messages together and returning that as a single error. However, after some thought, it may not be useful, since you can just call OrigErrs.

Yea, I agree for the leak test there wouldn't be much to test there, but was thinking of having complete coverage of the services that have custom marshalers may be nice.

@jasdel
Copy link
Contributor Author

jasdel commented Mar 16, 2016

Ah I see, for some reason I was thinking you meant for the error change. Yeah that makes sense for the route53 unmarshaller. I'll add that.

@jasdel
aws/awserr: Deprecate BatchError in favor of BatchedErrors
629f172 
BatchError did not satisfy the awserr.Error interface like it should of. This
update adds a new type BatchedErrors which satisfies awserr.Error.
@jasdel
service/route53: Add batched error support for ChangeResourceRecordSets
3265860 
Adds support for batched errors that ChangeResourceRecordSets can
optionally return.

Fix #438

Unmarshaling and tests provided by @abustany
@jasdel
Fix integration tests
481a64c
@jasdel jasdel force-pushed the pr438Route53UnmarshalError branch from c0c8442 to 481a64c Compare March 16, 2016 23:33
@jasdel
Copy link
Contributor Author

jasdel commented Mar 16, 2016

  • Added more robust baseError.OrigErr handling so it can return more comprehensive values.
  • Added leak tests for custom unmarshaller

@xibz
Copy link
Contributor

xibz commented Mar 16, 2016

LGTM, :shipit:

jasdel added a commit that referenced this pull request Mar 16, 2016
@jasdel
Merge pull request #593 from aws/pr438Route53UnmarshalError
6913213 
service/route53: Add batched error support for ChangeResourceRecordSets
@jasdel jasdel merged commit 6913213 into master Mar 16, 2016
@jasdel jasdel deleted the pr438Route53UnmarshalError branch March 16, 2016 23:48
jasdel added a commit that referenced this pull request Mar 18, 2016
@jasdel
Tag release v1.1.12
4da0bec 
References:
  #331, #438, #559, #590, #591, #592, #593, #595, #596, #597
xibz pushed a commit to xibz/aws-sdk-go that referenced this pull request Apr 18, 2016
@jasdel @xibz
Tag release v1.1.12
3e4095f 
References:
  aws#331, aws#438, aws#559, aws#590, aws#591, aws#592, aws#593, aws#595, aws#596, aws#597
xibz pushed a commit to xibz/aws-sdk-go that referenced this pull request Apr 18, 2016
@jasdel @xibz
Tag release v1.1.12
e008d15 
References:
  aws#331, aws#438, aws#559, aws#590, aws#591, aws#592, aws#593, aws#595, aws#596, aws#597
skotambkar pushed a commit to skotambkar/aws-sdk-go that referenced this pull request May 20, 2021
@jasdel
Fixup example documentation for region constants (aws#593)
72211bc 
Fixes aws#557 by removing out dated example documentation from the SDK's
endpoints package and README. The feature to enumerate regions and
services was removed in aws#512 pending further design and development of
service specific endpoint metadata.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@xibz xibz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasdel @xibz