Add support for CheckRedirect in Client.StandardClient()

[nuttmeister]

Hello everyone!

This PR adds support for custom redirect behavior like the standard http.Client supports with the CheckRedirect function.
Before this behavior couldn't be changed, not even on the default client returned by StandardClient().

It works using both *Client and *http.Client from StandardClient().
For example, disabling follow redirect all together as follows.

    client := retryablehttp.NewClient()
    client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
        return http.ErrUseLastResponse
    }

[hashicorp-cla]

All committers have signed the CLA.

[nuttmeister]

To clarify the check in Do could possibly be removed by using a setter function instead of exported variable.
But my thinking is since setters are not used for the other configuration it would lead to confusion.

So that's why the check is there so you could create the client, do a request, change the CheckRedirect variable and the next request would work.

Cheers!

[ryanuber]

Hey @nuttmeister ! Thanks for opening this.

Before this behavior couldn't be changed, not even on the default client returned by StandardClient().

Could you clarify why you are unable to set the CheckRedirect on the standard *http.Client? If you configure it on retryablehttp's HTTPClient field of the *Client struct, it seems like that ought to take care of it.

c := retryablehttp.NewClient()
c.HTTPClient.CheckRedirect = ...

I could be missing something, let me know!

[nuttmeister]

@ryanuber hey! I tried setting it on both client.HTTPClient.CheckRedirect and client.StandardClient().CheckRediect.
Worth nothing that in both cases I'm using the standard client after (by using StandardClient()).

So totally possible some of this PR is not really necessary. Didn't think that one through 100%.
I will check it out and do an update if necessary removing that part if that is the case.

[nuttmeister]

@ryanuber I have narrowed it down basically (I think).
Sorry for not being more thorough on this first.

Using just the regular none standard Client it works by just setting Client.HTTPClient.CheckRedirect.

But if you use the standard http.Client by using Client.StandardClient() it will not work by first have set Client.HTTPClient.CheckRedirect. Neither does setting http.Client.CheckRedirect that is returned.

However, what DOES does work that I just found out is actually doing this:

client := NewClient()
client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
    return http.ErrUseLastResponse
}
stdClient := client.StandardClient()
stdClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
    return http.ErrUseLastResponse
}

Which is not really straight forward perhaps to get it working when using the standard client.

But yeah, I will refactor the PR to copy these settings from what has been set in the HTTPClient instead.
Thanks for putting me on the correct path, haha.

[nuttmeister]

Updated to just copy the the CheckRedirect from HTTPClient.

[nuttmeister]

Long time with not activity. Is this something we want or should I close it?

[jefferai]

The CLA isn't signed yet so we cannot do anything with this PR at the moment, apologies.

[nuttmeister]

Hello. It was signed before. However I have changed companies twice and this email. The question is if it's wanted or not. If so I can try and sort it out.

[jefferai]

Yes, I think it's a good idea.

[manicminer]

@nuttmeister Wondering if you're able to get this ready to merge? Thanks!

[nuttmeister]

@manicminer sure, I just totally forgot about this. I will have a look at it today.

[nuttmeister]

@manicminer there, force pushed with the now correct email that has signed the agreement.

[manicminer]

@nuttmeister Thanks, appreciate you picking up this old PR, I'm glad I can preserve your commit 🙂