Custom connection settings #110
Comments
|
I actually ran into this today, I'm including the event function into an app I am writing, and noticed there is no good way to set an http proxy that isn't global. the easiest way to do this would be to replace instances of I am working on a PR to do this now. Edit: Just saw there is already a PR to allow this. |
|
I’m just looking at writing tests for this project, so use case #2 is key. I’ll review the existing PR and see if it looks mergable as-is. |
I wonder if using the PagerDuty client for this is the best path. I think a better pattern would be to have your code accept an interface that matches the functionality exposed by the PagerDuty API client, you can then write your own mock implementation(s) that returns the data you want. That way instead of testing the PagerDuty API client, on top of your code, you're only testing your code. |
During INC-2176 we discovered two issues that contributed to partner notifications not being sent out for a Major incident. If either of these factors had been there in this case, the notifications would have been sent correctly. The first issue is that Bifrost rejects notification requests if there is no SPS or Signups impact. We weren't validating the state of the ticket to make sure that, if notifyPartners is true, that either SPS or Signup was also selected. During the creation of INC-2176, notifyPartners was enabled by SPS impact was missed. The first issue then caused the second issue to occur, which is that the Bifrost request was recorded as being successful and not an actual failure. Bifrost itself responded with a message indicating that the request was malformed, but the scorebot codepath didn't see it as such and marked INC-2176 as having had notifications sent. Then, when the SPS box was checked, scorebot believed there was no action to take because a notification had been sent. This change mitigates the first issue by validating that a ticket has either SPS or Signup impact before trying to notify partners. This will prevent a malformed request making its way to Bifrost. This doesn't, however, change the HTTP response handling from Bifrost to properly detect a failure as that's a bit more effort. Fixes #110 Signed-off-by: Tim Heckman <[email protected]>
This updates the package to no longer make use of the `http.DefaultClient`, and to instead use a package-local default client as well as to provide consumers the ability to provide their own client in the time of need. The reason for moving away from the `http.DefaultClient` is for two reasons: 1. It prevents other packages from being able to impact this one by making changes to the `http.DefaultClient`. 2. The isolation should help make this package more resilient, as we can tweak the client's transport settings to provide defaults appropriate for PagerDuty usage. The package-local default client is not directly exposed, so as to hopefully avoid people making out-of-band changes to its config and to instead provide their own client. This also supports the ability for consumers to provide their own HTTP client, in the situation where the default client doesn't have the settings they need. The ability to provide this client was done using an interface, `HTTPClient`, so that consumers aren't forced to make use of `*http.Client` if they don't want to. Fixes #110 Signed-off-by: Tim Heckman <[email protected]>
|
So I've raised a PR (#121) which should address usecase 1 in this issue. It's similar to PR #112, except it allows consumers to use more than I think as a project we should determine whether usecsase 2 is something we want to support. I'm of the personal opinion that it isn't something we should support, and should instead encourage consumers to use dependency injection via interfaces. This should allow consumers to mock the client's behavior while unit testing. |
|
Thank you @theckman! Your PR looks great to me, and I agree that unit testing via interfaces is probably a better solution for (2). 👍 |
Currently, the
Clientin this package only allows connections to PagerDuty's live service using the default Go connection settings. It would be extremely useful to me to be able to create a client with custom connection settings. Here are two specific use cases I had in mind:I would like to be able to use this package in a service that has to go through an HTTP proxy to get to the outside Internet. The standard
net/httppackage lets me set up connections through a proxy viaTransport.Proxy. But to make use of this setting, I need apagerduty.Clientthat can use an arbitrary*http.Clientinstead of the default.I would like to be able to test my code that uses a
Clientwithout actually operating on my real PagerDuty account. One way to achieve this goal is to write HTTP server stubs and have the client contact my testing server (e.g., fromnet/http/httptest). To do that, I need a client that can send its requests to an arbitrary endpoint, not alwaysapi.pagerduty.com.Are there any existing plans that address these needs? I'd be happy to send in a pull request if needed.
The text was updated successfully, but these errors were encountered: