Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial Support for v3 Webhook Payloads #427
base: main
Are you sure you want to change the base?
Initial Support for v3 Webhook Payloads #427
Changes from 1 commit
4509fb3
6ca4799
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta admit I'm not digging this approach on my first read through. I think I understand why this was the path you took, but it's immediately got a few things that concern me about the UX of an API like this.
The first minor nit is that I believe that this should be a function that accepts an
OutboundEvent
(no pointer), as it's really acting on theoe
as an input and not mutating its internal state.The second larger thing is that I think, overall, this might "violate" one of the Go Proverbs, as it seems to be trying to be pretty clever:
It took me far too long to figure out how it's supposed to be used, and what the expected outcome of the call is when I pass in two values. It seems like a pretty confusing API, that I admit could be cleared up with a comment above the method.
The other is that it's a bit of a leaky abstraction, as consumers of this package still need to be intimately familiar with the JSON document structure of the webhook event payload. One of my goals in the suggested approach in #367 (comment) was to avoid consumers needing to be super familiar with the actual JSON, by offering a way to determine which type of payload it is and then have
struct
types that clearly enumerate the possible values within that payload.This would ultimately make this project more self-documenting, without the need for a consumer to also browse the PagerDuty docs.
I could maybe be convinced of this approach, and would love to hear your thoughts on the tradeoffs you see versus the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an oversight. It has been corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so I guess I should have provided a bit more color than I did in the PR description. A few items of background:
This method is not meant to be mutually exclusive with a set of methods that describe concrete types as described in #367 (comment). [I have actually started a bit of work on that as well.] Instead, this method is meant to allow consumers to work with V3 Webhooks in a generic way, particularly in situations where the full structured types may not have been implemented quite yet.
While there are examples of how to use this function in
examples/webhooks/webhook_server.go
as well as in the tests, this whole file was missing somegodoc
and that has been added. Have a look and let me know what you think...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that makes it a bit more clear to me. Although I do have one question:
How many of your client libraries are for statically typed languages? If they've been more focused towards the Perl, Ruby, Python, and JS communities, it doesn't surprise me that there haven't been static types defined.
With the cost of doing a major version bump in Go, especially because we're already at v1, we should be extra thoughtful before increasing the API surface area because we're probably going to be stuck with it for quite awhile. I say this because I don't know that I'm aligned with expanding the API surface area in this way, unrelated to maintenance cost of adding this to the public API.
Instead of expanding our API in this way, in cases where we've yet to define a
struct
type, I'd instead advocate for consumers to define their own struct (bonus points if they contribute it back to us). The reason I'm leaning towards this path, is that whether they want to define their own struct, or useGetEventDataValue
, they will need to inspect the API documentation to parse the data.Further compounding this,
GetEventDataValue
only returnsstring
values, but there are webhook fields that are non-string values. So not only will they need to know which field(s) to extract from the payload, but they will also need to know the value's type and handle any conversions themselves.Finally, if they want to use multiple values from within the webhook, they will need to extract those values individually, possibly convert some from a
string
to a more appropriate type (int
,bool
, etc.), and then put them inside of some container that allows them to be passed around. I suspect most folks would define astruct
type for this container, if one wasn't already present inpackage webhookv3
.So considering all of these factors that would drive a consumer to define a
struct
type anyway, I am not personally in favor of this API being added to thewebhookv3
package. Instead, I'd like to see us implement the most common types, commit to adding new types and fields as they are documented, and finally in cases where we have not defined the type yet encourage consumers to define their own struct types as needed and contribute them back as it makes sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I am not going to attempt to unpack all of this here, I would be happy to Zoom or exchange a few emails if you would like to discuss further.
We would really like to keep this method in place and would appreciate any feedback you might have around how to make it better.
That being said, see what you think about ChezCrawford#2 for also adding support for structured events into the mix at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One case I am still considering how we should handle is when this method is called using a set of
keys
that does not terminate in a string value. For example if the object was:and this function was called with
GetEventDataValue("service")
you would get the stringified version of theservice
object which isn't super useful.My gut feeling is to return an error for this case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the
io.Reader
interface contract, you should try processing the data inb
before you handle theio.ReadAll
error. I think this block of code should be something like:This version does a bit more to ensure the
*http.Request.Body
will contain the request body we received, even if we only got a partial read of the data. That way consumers can read that partial body themselves and inspect it.The one case this does not handle is when the
io.LimitReader
is violated. In that case they will get the.Body
that we read so far, and the remaining bytes will be lost meaning the caller couldn't choose to fully read it even if they wanted to. That could be accomplished by usingio.MultiReader()
, but would require some reworking of my snippet above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(IIRC, we got this code from the comments you provided in the previous PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those snippets were things I probably hammered out on my mobile phone while responding back to the comment(s). In hindsight, I should have done a better job at communicating that I hacked them together pretty rapidly as an example. I'm sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries but I would still like to get clarity on what is the "perfect" reusable HTTP body reader.
This is one of the reasons I was pushing for more focused functions that verify signatures and construct events in #326. I would like to focus my effort on providing solid support for V3 Webhooks rather than the details of how folks implement that functionality.