Webhooks for alerts #309
Webhooks for alerts #309
Conversation
…ht/python-sdk into webhooks-for-alerts-v2
…ht/python-sdk into webhooks-for-alerts-v2
| @@ -46,7 +48,7 @@ | |||
| from .client import DEFAULT_REQUEST_TIMEOUT, Groundlight, logger | |||
|
|
|||
|
|
|||
| class ExperimentalApi(Groundlight): | |||
| class ExperimentalApi(Groundlight): # pylint: disable=too-many-public-methods | |||
There was a problem hiding this comment.
Let me know if there's a different way you'd prefer I solve this -- it seems best to keep the method here so I just disabled it.
There was a problem hiding this comment.
I think I prefer this. The pylint suggestion is probably to group up the methods under subclasses, but I think that's unnecessary at this time
| @@ -46,7 +48,7 @@ | |||
| from .client import DEFAULT_REQUEST_TIMEOUT, Groundlight, logger | |||
|
|
|||
|
|
|||
| class ExperimentalApi(Groundlight): | |||
| class ExperimentalApi(Groundlight): # pylint: disable=too-many-public-methods | |||
There was a problem hiding this comment.
I think I prefer this. The pylint suggestion is probably to group up the methods under subclasses, but I think that's unnecessary at this time
| self, | ||
| detector: Union[str, Detector], | ||
| name, | ||
| condition: Condition, | ||
| actions: Union[Action, List[Action], ActionList], | ||
| actions: Optional[Union[Action, List[Action], ActionList]] = None, | ||
| webhook_actions: Optional[Union[WebhookAction, List[WebhookAction]]] = None, |
There was a problem hiding this comment.
A little bit of humming and hawing here.
This outcome of having webhook_actions being separate from actions is non-intuitive for users. The natural design here is that we have many types of actions that are subclasses of Action, webhook_actions among them.
The reason this is not laid out this way is because we didn't have that design to begin with and built significant infra around a different layout. That makes it unreasonable to add webhook_actions as a subclass of actions here in the SDK because the SDK by design is tightly coupled with our other infra.
TLDR; we started painting ourselves into this corner a long time ago, and I don't like this corner. It works though and nothing needs to change
There was a problem hiding this comment.
Yeah, I agree with all of that. It is confusing for the user, but it seems like this makes the most sense in terms of keeping the SDK in line with the backend infra.
If we do want to decouple the SDK from the rest of the infra in this way, I think it could be fine to create a convenience method for the users that allows them to submit a single list of actions, and we can split them into separate "actions" and "webhook actions" before sending to the back end.
Webhooks have been recently added as an action option for Groundlight, this PR makes them accessible through the SDK.