Fix httpclient test to allow passing in a customer client #79
Conversation
| // Allow https:// prefix on HostPath even though it's the only scheme we allow | ||
| // as it's more natural to support the URL. Any other scheme we don't strip | ||
| // which will fail validation. | ||
| if strings.HasPrefix(strings.ToLower(c.HostPath), "https://") { | ||
| c.HostPath = c.HostPath[8:] | ||
| } |
There was a problem hiding this comment.
If anyone disagrees with this change I don't have a strong opinion and happy to revert but it seems convenient to allow this - it could be more intuitive for users to not need to remember to specify a protocol in one ENV var but not in the other for example...
There was a problem hiding this comment.
One other question that puzzled me: is there a reason that we set defaults for these two configs here in Canonicalize but then we set the oauth secrets from ENV in Validate? It seems like those should maybe be in here too if they mutate the config?
I didn't do that in this PR but happy to if that would be helpful!
There was a problem hiding this comment.
That's a good question. Can't remember if that was intentional or not, but in any case I'd agree that it makes sense to set the auth secrets in Canonicalize as well.
There was a problem hiding this comment.
I'll leave it in this PR to keep things constrained. Would you like me to create another with that change?
| // Allow https:// prefix on HostPath even though it's the only scheme we allow | ||
| // as it's more natural to support the URL. Any other scheme we don't strip | ||
| // which will fail validation. | ||
| if strings.HasPrefix(strings.ToLower(c.HostPath), "https://") { | ||
| c.HostPath = c.HostPath[8:] | ||
| } |
There was a problem hiding this comment.
That's a good question. Can't remember if that was intentional or not, but in any case I'd agree that it makes sense to set the auth secrets in Canonicalize as well.
Co-authored-by: Brenna Hewer-Darroch <21015366+bcmdarroch@users.noreply.github.com>
|
Thanks @bcmdarroch. Not sure on etiquette here as I'm not the code owner! I have merge access but just checking you're happy for me to merge now? |
🛠️ Description
While trying to work out how to test my code that calls the SDK I came looking for an option to allow it to use non-TLS or trust self-signed TLS so I could use a mock server.
Then I got really confused because the test here already does exactly that already 😕 .
Eventually after some head scratching, I realised the test was not actually work as expected! Because the API call error was not being checked, the http client was failing on TLS validation which mean none of the assertions in the server handler were ever being hit.
The good news is that once I fixed that, they did all pass, so the actual library behaviour has always worked, just the test didn't exercise what it seemed to!
🔗 External Links
🚢 Release Note
Release note for CHANGELOG:
👍 Definition of Done
httptest.Server.Client()which will validate the server TLS certificate and allows testing SDK-dependent code against a simple mock of the API.code .