-
Notifications
You must be signed in to change notification settings - Fork 128
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
Don't try to parse body if HTTP status is not 200 #81
base: master
Are you sure you want to change the base?
Don't try to parse body if HTTP status is not 200 #81
Conversation
bb74298
to
803af85
Compare
Not sure yet seems that test |
803af85
to
9f69534
Compare
Removing |
@masterzen Could you please take a look? |
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.
Sorry for the long delay in looking at your PR, and thanks for sending it.
I understand what you're trying to fix here, but unfortunately the proposed change breaks the fundamental feature of supporting transient Operation Timeout server errors (which are just to tell the client that the command hasn't produced any output in the given time), which are usually just ignored and retried.
I'm all in favor of DRYing the code base by reusing ParseSoapResponse
as you did, but for the moment, leave the the forked azure-sdk-for-go
imports until I've made clear that we're not supporting older Go environments (since this is a library this has the potential of breaking upstream things).
Would you mind amending your PR so that we still support the Operation Timeout error, but still report straight http errors?
Many thanks,
Brice
"strings" | ||
"time" | ||
|
||
"github.com/masterzen/azure-sdk-for-go/core/http" |
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.
Removing this means we're breaking the compatibility with Go < 1.8.
But maybe we don't care anymore nowadays...
body, err := body(resp) | ||
// error in case of incorrect exit code | ||
if resp.StatusCode != 200 { | ||
return "", fmt.Errorf("http unexpected status: %s", resp.Status) |
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.
The problem here is that if you get an operation timeout (which is also an HTTP 500 error), you'll return an error here that the upper layer will not "ignore" anymore (see command.go:134
and you'll break long running commands.
Note that OperationTimeout
are SOAP errors, so they need to be parsed.
That's the reason your code breaks the TestOperationTimeoutSupport
test (the test is designed to show that getting a timeout doesn't mean the command is aborted, it still runs on the server until there are more output to display).
So, I think we first have to check if the body is a soap+xml
, then error as before, but we also have to carry over correctly the error message if it is a straight http error (and not a soap+xml
error).
Improves error message from
http response error: 401 - invalid content type
intohttp unexpected status: 401 Unauthorized
Also unifies imports, code reuse.