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.

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

Add HttpHelper methods that accept options structs #1078

Merged
merged 14 commits into from
May 28, 2022

Conversation

tonerdo
Copy link
Contributor

@tonerdo tonerdo commented Mar 9, 2022

This PR adds option classes for the HttpHelper methods so that it can be more easily extended in a backwards compatible way.

Addresses #902

@dkuzmenok
Copy link

@denis256
@tonerdo
https://github.com/gruntwork-io/terratest/blob/master/modules/http-helper/http_helper.go#L211
There was a []byte used to pass request body, then it was used to create a byte reader and pass to http.Request constructor.
When we require to use byte reader directly from HttpDoOptions, then on request retry we will try to use a reader that is already read in a previous request attempt.

I do see 2 options to stay safe:

  1. Add additional field to HttpDoOptions to set BodyReader value, and Body to be able to use both a provided reader or the byte slice.
  2. Change Body to become []byte and create Reader from it, like it is already done.

Personally I would prefer 1) as it gives flexibility.

@tonerdo
Copy link
Contributor Author

tonerdo commented Apr 12, 2022

@dkuzmenok the changes in this PR already account for the scenario you mentioned. If you take a look at

data, err := io.ReadAll(options.Body)
if err != nil {
return "", err
}
options.Body = nil
we read the data out of the io.Reader and cache it for use in subsequent requests.

@dkuzmenok
Copy link

@tonerdo Strange I did not notice that, when I was tracing method calls. Then it looks fine. Thanks.

@dkuzmenok
Copy link

@denis256 @tonerdo Is there any timeline to merge this? There are no changes to existing method signatures, so should be safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants