-
Notifications
You must be signed in to change notification settings - Fork 15
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
Create AEP-501: Webhook payloads #209
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks! I think there's a few questions I have around this, along with some general things that I think should align better with AEP style currently (unless we decide to change that too).
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.
why 501? considering the number of the AEP shouldn't matter too much in the future, I'd rather we just start grabbing the next unused integer. What do 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.
I just picked a number, happy to renumber it. I think 6 is the next available number right?
aep/webhooks/scope.yaml
Outdated
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.
why do we need a new directory for webhooks? this feels like a regular design pattern to me.
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.
We have three webhook-specific AIPs internally at Roblox (this is the most interesting one). If we were to have multiple webhook-specific AEPs would you still suggest not having a folder? (No strong feelings on my end either way.)
@@ -0,0 +1,8 @@ | |||
--- | |||
id: 5001 |
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.
501 or 5001?
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.
Ack but will let the other comment get resolved first.
Webhook payloads **must** be versioned, and API consumers **must** register | ||
callback URIs for a specific version of a payload. |
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.
is this different from a regular resource?
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.
Webhook payloads aren't resources at all, so I don't think we can treat guidance about resources as applying to them.
not** change which API is used to version the payload. | ||
|
||
Webhook payloads without resource references, and without any other | ||
relationship to a service API, **must** be versioned. |
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.
what do you mean by "service API"? do you mean "API service"? https://aep.dev/3#api-service.
If service API is intended to be a new term that differs, it would be good to add it to the Glossary.
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.
By "service API" I mean an API that accepts requests, as distinct from a webhook API where the client accepts requests from the API. Happy to add to the glossary if you agree this makes sense.
Breaking changes to a webhook payload **must not** be made within a major | ||
version. |
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.
Is this any different from aip.dev/180? (pointing to AIPs since we haven't imported it yet).
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.
Well...I don't think it's obvious to everyone (or at least it wasn't within Roblox) which rules of service APIs would also apply to webhooks. Thus a general pattern of restating stuff a bit (while staying as DRY as possible).
This includes standard headers such as "retry-count", "signature", and | ||
"request-origin". |
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.
Isn't this standard guidance regardless of webhooks? I feel like guidance around what goes in a side-channel vs not could be it's own AEP.
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.
Could be...this guidance might overfit specific questions that arose within Roblox. Happy to file a ticket for that. Think we should leave this here in the meantime?
Payloads **must** include only information specific to the event that triggered | ||
the webhook callback. Any additional metadata not pertaining to the event | ||
should be sent in a side channel and **must not** be included in the payload. |
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 don't know if this guidance is actionable. If I wanted to include something in the webhook payload, it's probably because some user needed it, and that's enough of a reason arguably to say it's information specific to the event.
Also if I include non-event related data, it wouldn't break any clients or doc generators. So is it really a must? If this was the only thing that an API did that wasn't AEP compliant, would we be ok with them not adopting the AEPs for this reason?
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.
Yeah I agree - this is probably a specific reaction to a question that came up at Roblox, but it was an odd question and likely wouldn't come up elsewhere.
Do you think we should change must to should here or just remove the guidance?
High level question: have you looked at other standards like https://www.standardwebhooks.com/? That guidance has some pros:
I imagine you're motivated by providing webhook guidance around protobuf, but it might be good to base it on an HTTP+JSON standard. |
Co-authored-by: Yusuke Tsutsumi <tsutsumi.yusuke@gmail.com>
Co-authored-by: Yusuke Tsutsumi <tsutsumi.yusuke@gmail.com>
Co-authored-by: Yusuke Tsutsumi <tsutsumi.yusuke@gmail.com>
Adds guidance for webhook payloads.
The referenced common components are added in aep-dev/aep-components#17.
🍱 Types of changes
What types of changes does your code introduce to AEP? Put an
x
in the boxesthat apply
📋 Your checklist for this pull request
Please review the AEP Style and Guidance for
contributing to this repository.
General
references AEPs
correctly.
(usually
prettier -w .
)Additional checklist for a new AEP
guidelines are met.
Document structure
guidelines are met.
💝 Thank you!