Return HTTP response codes as typed errors #161
Conversation
rest-datasource.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| ErrNotFound = errors.New("not found") |
There was a problem hiding this comment.
Maybe these could go into rest-common.go or something? 🤔
|
Thanks for the feedback. I'll be back with some updates to this PR. |
suhlig
force-pushed
the
datasources-use-status-code
branch
from
99de677
to
4f854eb
Compare
|
I amended my commit with the changes you proposed. Test are green on my workstation; not sure how I could run the GitHub actions. |
|
It seems like linters are failing. Could you please take a look? |
suhlig
force-pushed
the
datasources-use-status-code
branch
from
4f854eb
to
0e3e593
Compare
|
Should be fixed now. |
|
Still, some errors are happening. AFAICT the issue is here: We should ignore 404 errors here (: @suhlig could you please take a look? |
|
Sure! Thanks for catching these. I ran |
|
We also have integration tests. For that, you need a locally running Grafana, most likely in a Docker container, plus environment variable |
This allows to differentiate a datasource not found from other errors, e.g.
```go
_, err = client.GetDatasource(ctx, dsID)
if errors.Is(err, sdk.ErrNotFound) {
fmt.Fprintf(os.Stderr, "Creating new datasource %s (id=%d)\n", ds.Name, ds.ID)
}
```
suhlig
force-pushed
the
datasources-use-status-code
branch
from
0e3e593
to
66a7bce
Compare
|
The integration test failure turned out to be a valuable one. I had overlooked a few error cases for I changed the integration test to consistently use I also added instructions to the README on how to run local tests. |
|
Thanks, I will take a look tomorrow |
rest-dashboard.go
Outdated
| err = dec.Decode(&result) | ||
| boardBytes = []byte(result.Board) | ||
| case http.StatusNotFound: | ||
| err = fmt.Errorf("dashboard with path %q %w", path, ErrNotFound) |
There was a problem hiding this comment.
Maybe we could move these switches into a function and then consistently use it when the HTTP status code is not equal to 200? The function could accept some string indicating the action or extra data. Then we could do:
return httpError(fmt.Sprintf("searching for dashboard with path %q", path))
There was a problem hiding this comment.
Thanks for the suggestions. I will look into this when I am back from vacation.
There was a problem hiding this comment.
👍 I really think that this improvement is worthwhile because the users will be able to use errors.Is(err, ErrNotFound) and others consistently with all methods
There was a problem hiding this comment.
I just added a second commit that extracts httpStatusCodeError as suggested. Feel free to squash both commits if you are happy with the result.
| return StatusMessage{}, err | ||
| } | ||
| if code != http.StatusOK { | ||
| return StatusMessage{}, fmt.Errorf("HTTP error %d: returns %s", code, raw) |
There was a problem hiding this comment.
Why not use httpStatusCodeError everywhere in all of these functions (in this and others)?
|
Bump ... anything I need to do before the PR can be merged? |
This allows to differentiate a datasource not found from other errors, e.g.