Conversation
…ight/python-sdk into webhooks-custom-payloads
brandon-wada
left a comment
brandon-wada
left a comment
There was a problem hiding this comment.
Rambled a bunch in my comments, but things look good as is
| payload_template=payload_template, | ||
| ) | ||
|
|
||
| def make_payload_template(self, template: str) -> PayloadTemplate: |
There was a problem hiding this comment.
I've left bad example code for you, but I appreciate that you followed the pattern
We shouldn't lean too hard into the make_* format. Most of the classes exist and it shouldn't be strange for a user to construct a payload as PayloadTemplate(template='mytr'). These generated classes are what we return via our custom SDK functions, so they shouldn't be strange to users. We do have make_* commands for alerts (specifically actions and conditions) because they have these horribly undiscoverable set of parameters, and the make_* commands give a convenient docstring for discovering those parameters.
The real fix is to make those parameters well defined and discoverable, but that's way down my priority stack.
This is all my ramblings, I don't actually have any requests from this comment, just that you know the background
|
|
||
| def make_webhook_action(self, url: str, include_image: bool) -> WebhookAction: | ||
| def make_webhook_action( | ||
| self, url: str, include_image: bool, payload_template: Optional[PayloadTemplate] = None |
There was a problem hiding this comment.
nit, if reasonable I think the payload_template should be allowed to be a string here as well.
There was a problem hiding this comment.
I'd prefer not to make that change -- currently payload_template only has one text field, but I think we might be adding more soon. Specifically, we might be adding a separate header template for auth or a field indicating what kind of payload template it is (we currently only support json).
2 participants
This PR adds the ability to provide custom payloads for webhook alerts. It will be included in the 0.22.0 SDK release.