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: avoid wrapping RawError twice #718
fix: avoid wrapping RawError twice #718
Conversation
@feiskyer any reason we don't embed the ServiceError type instead of RawError? It seems that every code-path should return this. |
cebcf5c
to
7d447e2
Compare
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.
looks good
/retest
ideally, every azure api should report ServiceError, but we're not actually sure about that. So even with a ServiceError, we would still need another RawError if the responded error type couldn't be serialized to ServiceError. |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, marwanad The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cherry-pick of #718: avoid wrapping RawError twice
cherry-pick of #718: avoid wrapping RawError twice
better categorize errors on CA operations (depednent on: kubernetes-sigs/cloud-provider-azure#718 - will bump vendor/cherry-pick in a follow-up)
better categorize errors on CA operations (depednent on: kubernetes-sigs/cloud-provider-azure#718 - will bump vendor/cherry-pick in a follow-up)
What type of PR is this?
What this PR does / why we need it:
The sender decorator builds a local
Error
type that embeds the RawError returned from the Azure service. However, in the client'ssendRequest
method it wraps this error again under theError
type.This would return a double-wrapped error the consumers:
Initially was thinking of safe guarding the behaviour but because
Error
doesn't implement the error interface, it seemed to be a bit of hassle.With the fix in place, the top-level consumers will get the expected error body:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: