-
Notifications
You must be signed in to change notification settings - Fork 44.6k
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
feat(platform, blocks): Webhook-triggered blocks #8358
feat(platform, blocks): Webhook-triggered blocks #8358
Conversation
59b467e
to
74c3047
Compare
74c3047
to
2f405c3
Compare
7df8e68
to
f117d3f
Compare
…igger block - Improve input descriptions for `repo`, `payload`, and `pull_request` - Add `<SchemaTooltip>` in cases where `NodeHandle` isn't used for a block input - Fix spacing within `<CredentialsInput>`
c415bd0
to
397ae0b
Compare
Resolves #8357 - Add user flow to explicitly confirm deleting credentials linked to in-use webhooks - Add logic to deregister, remove, and unlink webhooks before deleting parent credentials - backend: Add `NeedConfirmation` exception - frontend: Add `AlertDialog` UI component
Resolves #8738 - Disable webhook blocks if `PLATFORM_BASE_URL` is not set - Throw `MissingConfigError` when trying to set up a node-webhook-link if `PLATFORM_BASE_URL` is not set - Add `MissingConfigError` - Add field validator to `Config.platform_base_url` and `Config.frontend_base_url`
c2dd80b
to
6eb643d
Compare
``` | ||
</details> | ||
|
||
2. **Define event filter input** in your block's Input schema. |
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 is an event filter input? What does it do? Do I need one? what if I can't filter my webhooks
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.
It currently is required, as it fits all the existing implementations. Making it optional wouldn't be hard, but also currently isn't necessary.
The angle to this PR is: it's the first version of the webhook system and will be iterated on further as we add support for more providers.
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've amended the description. Making this input optional can be done when it becomes necessary in a follow-up PR.
docs/content/platform/new_blocks.md
Outdated
- The name of the input field (`events` in this case) must match `webhook_config.event_filter_input`. | ||
- The event filter itself must be a Pydantic model with only boolean fields. | ||
|
||
3. **Include payload field** in your block's Input schema. |
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.
Can this be a pydantic model?
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.
In theory, sure. I'm not sure the existing implementation of webhook ingress endpoint + executor supports it though.
</details> | ||
|
||
4. **Define `credentials` input** in your block's Input schema. | ||
- Its scopes must be sufficient to manage a user's webhooks through the provider's API |
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.
How do we handle when there is no provider api?
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.
(use this to make follow up ticket)
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.
That's a question I left to answer when we actually have to deal with that
</details> | ||
|
||
- The name of the input field (`events` in this case) must match `webhook_config.event_filter_input`. | ||
- The event filter itself must be a Pydantic model with only boolean fields. |
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 this is kinda a weirdly restricting thing
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.
The restriction makes the rest of the implementation easier. Also, this structure allows adding individual descriptions to all the options, which I can definitely see being necessary for good UX if event names are not self-explanatory.
- A `credentials` input with the scopes needed to manage webhooks | ||
- Logic to turn the webhook payload into outputs for the webhook block | ||
- The `WebhooksManager` for the corresponding webhook service provider, which handles: | ||
- (De)registering webhooks with the provider | ||
- Parsing and validating incoming webhook payloads | ||
- The credentials system for the corresponding service provider, which may include an `OAuthHandler` | ||
|
||
There is more going on under the hood, e.g. to store and retrieve webhooks and their | ||
links to nodes, but to add a webhook-triggered block you shouldn't need to make changes | ||
to those parts of the system. | ||
|
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 makes a heavily flawed assumption of all webhooks must be registered via an API. Provide a simple webhook manager that does not require this.
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.
#8750 is my attempt at this for context
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.
Let's fix that in your PR as a follow-up.
- Resolves #8743 - Follow-up to #8358 ### Demo https://github.com/user-attachments/assets/f983dfa2-2dc2-4ab0-8373-e768ba17e6f7 ### Changes 🏗️ - feat(frontend): Add webhook status indicator on `CustomNode` - Add `webhookId` to frontend node data model - fix(backend): Fix webhook ping endpoint - Remove `provider` path parameter - Fix return values and error handling - Fix `WebhooksManager.trigger_ping(..)` - Add `credentials` parameter - Fix usage of credentials - Fix `.data.integrations.wait_for_webhook_event(..)` - Add `AsyncRedisEventBus.wait_for_event(..)` - feat(frontend): Add `BackendAPIProvider` + `useBackendAPI` - feat(frontend): Improve layout of node header Before: ![image](https://github.com/user-attachments/assets/17a33b94-65f0-4e34-a47d-2dd321edecae) After: ![image](https://github.com/user-attachments/assets/64afb1e4-e3f2-4ca9-8961-f1245f25477f) - refactor(backend): Clean up `.data.integrations` - refactor(backend): Fix naming in `.data.queue` for understandability ### Checklist 📋 #### For code changes: - [x] I have clearly listed my changes in the PR description - [x] I have made a test plan - [x] I have tested my changes according to the test plan: <!-- Put your test plan here: --> - [x] Add webhook block, save -> gray indicator - [x] Add necessary info to webhook block, save -> green indicator - [x] Remove necessary info, save -> gray indicator --------- Co-authored-by: Nicholas Tindle <[email protected]>
…ame` globally (#8725) - Resolves #8931 - Follow-up to #8358 ### Changes 🏗️ - Avoid double specifying provider and cred types on `credentials` inputs - Move `credentials` sub-schema validation from `CredentialsField` to `CredentialsMetaInput.validate_credentials_field_schema(..)`, which is called in `BlockSchema.__pydantic_init_subclass__` - Use `ProviderName` enum globally
Changes 🏗️
feat(platform): Add support for Webhook-triggered blocks
PLATFORM_BASE_URL
settingAdd webhook config option and
BlockType.WEBHOOK
toBlock
Block.__init__
to enforce type and shape of webhook event filterBlock.__init__
to enforcepayload
input on webhook blocksBlock.__init__
to disable webhook blocks ifPLATFORM_BASE_URL
is not setAdd
Webhook
model + CRUD functions inbackend.data.integrations
to represent webhooks created by our systemIntegrationWebhook
to DB schema + referenceAgentGraphNode.webhook_id
set_node_webhook(..)
inbackend.data.graph
Add webhook-related endpoints:
POST /integrations/{provider}/webhooks/{webhook_id}/ingress
endpoint, to receive webhook payloads, and for all associated nodes create graph executionsNode.is_triggered_by_event_type(..)
helper methodPOST /integrations/{provider}/webhooks/{webhook_id}/ping
endpoint, to allow testing a webhookWebhookEvent
+ pub/sub functions inbackend.data.integrations
Add
backend.integrations.webhooks
module, including:graph_lifecycle_hooks
, e.g.on_graph_activate(..)
, to handle corresponding webhook creation etc.BaseWebhooksManager
+GithubWebhooksManager
to handle creating + registering, removing + deregistering, and retrieving existing webhooks, and validating incoming payloadsOther improvements
NodeHandle
payload
) withSchemaField(hidden=True)
MultiSelector
component stylingAlertDialog
UI componentNodeMultiSelectInput
componentNodeModel
withgraph_id
,graph_version
;GraphModel
withuser_id
make_graph_model(..)
helper function inbackend.data.graph
RedisEventQueue
generic and move tobackend.data.execution
generateInputHandles(..)
inCustomNode
MissingConfigError
,NeedConfirmation
exceptionHow it works
on_graph_activate
andon_node_activate
hooks are called on the graph and its nodeson_node_activate
will get/create a suitable webhook and link it by settingAgentGraphNode.webhook_id
on_node_activate
useswebhook_manager.get_suitable_webhook(..)
, which tries to find a suitable webhook (with matching requirements) or creates it if none exists yeton_graph_deactivate
andon_node_deactivate
are called on the graph and its nodes to clean up webhooks that are no longer in usewebhooks/{webhook_id}/{event_type}
TODO
dict[str, bool]
block input as collapsible multi-select #8538GitHubPullRequestTriggerBlock
#8737PLATFORM_BASE_URL
is not set #8738repo
input of webhook blocks that the credentials used must have the right permissions for the given organization/repoFollow-up