Adding support for switchable HTTP Clients

[beckjake]

Hi @guusvw, thanks for your contribution!

I'm sorry this has sat for so long, we do not frequently check our github repositories for PRs.

Can you please clarify on what a user would have to do to make use of this code? It appears to me that someone importing the jcapi package would need to set jcapi.HTTPClientFn to their own custom value, is that accurate?

[antonu17]

Hi @guusvw, @beckjake! As I see, the main problem is that V1 API libray does not allow to set a timeout on API calls. There is an article explaining the problem: https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779 And this patch makes it possible to provide custom http.Client (with timeout configured, for instance). I tried this out like this:

jcapi.HTTPClientFn = func(_ context.Context) *http.Client {
    return &http.Client{
        Transport: myTransport,
        Timeout:   myTimeout,
    }
}
jcAPIv1 := jcapi.NewJCAPI(myAPIKey, jumpCloudAPIURL)

And it worked as expected. The only thing unclear here is introducing a context, which is not used anywhere. But it can be kindly ignored :smiley: Is it possible to merge this PR so users don't have to fork it and implement timeouts on client side?

[beckjake]

Hi Anton,

That is a good reason and I'd do it if I could, but unfortunately I no longer work at JumpCloud. I'm not sure anyone who works there really looks at these things at all unfortunately, Github makes it pretty invisible and I recall only seeing the original issue because I explicitly was looking through our open issues/PRs on this repository.

You might have better luck escalating through support or whoever manages your account, or something like that. Sorry I can't help!

On Tue, Aug 14, 2018 at 7:12 AM Anton Ustyuzhanin notifications@github.com wrote:

Hi @guusvw https://github.com/guusvw, @beckjake https://github.com/beckjake! As I see, the main problem is that V1 API libray does not allow to set a timeout on API calls. There is an article explaining the problem: https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779 And this patch makes it possible to provide custom http.Client (with timeout configured, for instance). I tried this out like this:

jcapi.HTTPClientFn = func(_ context.Context) *http.Client { return &http.Client{ Transport: myTransport, Timeout: myTimeout, } } jcAPIv1 := jcapi.NewJCAPI(myAPIKey, jumpCloudAPIURL)

And it worked as expected. The only thing unclear here is introducing a context, which is not used anywhere. But I can be kindly ignored 😃 Is it possible to merge this PR so users don't have to fork it and implement timeouts on client side?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/TheJumpCloud/jcapi/pull/38#issuecomment-412866715, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbmzjnZXt0Wz9tGv_3q0jCzP36ibRKYks5uQsy0gaJpZM4Mif27 .

[jpvajda]

@antonu17 Hello! I'm reviewing this PR now with a few of our Engineers here at Jumpcloud. We are focusing more attention on our SDKs now as a company, so I apologize for the delays you've had in the past. We'll let you know if there are any questions / comments on this PR shortly.

[jpvajda]

@antonu17 we just updated our JCAPI Go Library to support v1 endpoints. I'm curious if you can use that SDK for your needs, instead of needing this PR merged into this SDK Library. We have technically discontinued support on JCAPI as it's out dated with our current set of API functionality. Just let me know if this option works for you or if you still need this PR merged into this older library.

https://github.com/TheJumpCloud/jcapi-go

[antonu17]

Ah, perfect. Thank you for the update, @jpvajda. I'm gonna switch on it now!

[jpvajda]

Closing PR as customer is switching to the new JCAPI-Go client.