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

fix: close response body in http strategy #718

Merged
merged 2 commits into from
Dec 31, 2022
Merged

fix: close response body in http strategy #718

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
638c0d3
fix: close response body in http strategy
kruskall Dec 30, 2022
bfb296d
lint: assign error to empty var
kruskall Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions wait/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,11 @@ func (ws *HTTPStrategy) WaitUntilReady(ctx context.Context, target StrategyTarge
continue
}
if ws.StatusCodeMatcher != nil && !ws.StatusCodeMatcher(resp.StatusCode) {
resp.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should handle the error here too, as in L227

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the feedback!

the error is used to continue to the next iteration, we're already calling continue here. I think I'm missing something

Copy link
Member

Choose a reason for hiding this comment

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

I know that golangci-lint checks for non-handled errors. So I'd say that we need to handle them in case the Close call fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that golangci-lint checks for non-handled errors.

I'm not able to reproduce this. I'm running golangci-lint v1.50.1 and I can see errcheck is enabled but nothing is being reported.
I've looked at the codebase and it seems there are other places where the error is being assigned to an empty var. I've pushed some changes to do the same here. Let me know what you think 🏓

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the 🏓 (please see #653)

if golangci does not complain, I'm good with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, congratulations! 😄

continue
}
if ws.ResponseMatcher != nil && !ws.ResponseMatcher(resp.Body) {
resp.Body.Close()
continue
}
if err := resp.Body.Close(); err != nil {
Expand Down