-
Notifications
You must be signed in to change notification settings - Fork 1
download returns useful message when HTTP status code is 412 #637
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
base: main
Are you sure you want to change the base?
download returns useful message when HTTP status code is 412 #637
Conversation
d4be298 to
8c8f08e
Compare
3f6a3a9 to
50e6547
Compare
50e6547 to
8148824
Compare
8148824 to
a45f622
Compare
Co-authored-by: Joakim Bygdell <[email protected]>
d41c925 to
8d50c9f
Compare
| switch res.StatusCode { | ||
| case http.StatusOK: | ||
| return resBody, nil | ||
| case http.StatusPreconditionFailed: // Return the original message from the server in case of 412 | ||
| return nil, errors.New(strings.TrimSpace(string(resBody))) | ||
| default: | ||
| return nil, fmt.Errorf("server returned status %d", res.StatusCode) | ||
| } |
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.
| switch res.StatusCode { | |
| case http.StatusOK: | |
| return resBody, nil | |
| case http.StatusPreconditionFailed: // Return the original message from the server in case of 412 | |
| return nil, errors.New(strings.TrimSpace(string(resBody))) | |
| default: | |
| return nil, fmt.Errorf("server returned status %d", res.StatusCode) | |
| } | |
| if res.StatusCode != http.StatusOK { | |
| return nil, errors.New(strings.TrimSpace(string(resBody))) | |
| } | |
| return resBody, nil |
We shouldn't treat this as a special case. It's better to always return any error string to the user.
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'm not quite sure if it's better to always return any error string to the user. We had a previous issue to filter error messages returned from the API and just show the HTTP code. For this special case (412), I'm pretty sure error messages returned from the API are quite clean, but not sure for all the other cases.
| // Download function downloads files from the SDA by using the | ||
| // download's service APIs | ||
| func Download(args []string, configPath, version string) error { | ||
| appVersion = version |
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.
Not directly related to this PR, but to me if would be cleaner if we remove the appVersion variable, and instead update the functions that uses it to take the version as a argument, eg:
func datasetCase(token, version string) error {
...
}And we would pass the version from the Download invocation
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 agree. We should note this in the future refactoring issue.
Related issue(s) and PR(s)
This PR closes #623
Description
sda-cli returns the original message when the HTTP status code is 412. When the status code is others, still return just the status code to avoid exposing unnecessary information to the user
How to test
That unit test and integration tests pass.