add parameter to control redirects

[pinzon]

Motivation

Currently, the ELB service relies on this library; however, to ensure that the WordPress scenario functions correctly ( trying to go through the installation process), the 302 responses and other redirects should be returned to the client rather than being handled by the internal HTTP client.

Changes

• add new parameter to the request method

[simonrw]

I am not sure about this change. While I feel this solution addresses the actual problem that was uncovered (requests following redirects, where for a proxy we don't want that behaviour), I feel this solution is not very future proof.

For example, allowing **kwargs could be a nice approach to support any requests arguments.

However: I don't know if this goes against the philosophy of this client. Should it be

• a simple client that has sensible behaviour, or
• a simple client that has sensible defaults which can be overridden

What do you think @thrau?

[thrau]

generally with these sort of changes i would first ask where the new parameter should go: the generic HttpClient.request method (the contract), which needs to be added to all HttpClient implementations? or is this specific to the client implementation that is being used, i.e., the requests library client?

we could add a **kwargs argument to the function, but that's just a wildcard for passing implementation-specific parameters to what should be a contract. so what we're creating here is a leaky abstraction.

for this specific change, it's not clear me what allow_redirects means exactly in the context of other HttpClient implementations (like a Proxy). it seems it's very specific to the requests library.

unless we need to use the same client instance for making requests where allow_redirects is true/false, i think the safest option would be to add it to the constructor of SimpleRequestsClient, and keep it as part of the class. this we could generalize into something like default_request_kwargs and pass those to the self.session.request call.

if it absolutely needs to be specified per-request, then i suggest renaming it to follow_redirects (which seems a bit more specific), also add it to the HttpClient interface to not break the contract, and then implement the parameter for other client implementations.

[thrau]

thanks for making the changes, LGTM!

[thrau]

since we kept the parameter specific to requests, i would have kept the original name of allow_redirects, so it's consistent with the requests library :-) though follow_redirects is the better name to generalize if we added it to all client implementations