Support webhook integrations#5039
Conversation
3a82850 to
e5dcb90
Compare
e5dcb90 to
4ef6ca7
Compare
63e35d3 to
1ef4939
Compare
|
@Json-Andriopoulos nice work on this - one request while the adapter design is still soft. We have concrete customer demand (can't name names here) for exactly this flow with Firecrawl's monitoring webhooks: a monitor detects a page change, posts a Two things in the current
Both are cheap to generalize now and annoying to retrofit once adapters exist downstream. Happy to contribute a |
cfb479a to
e0b8a43
Compare
e9c8445 to
2c8e2a2
Compare
Documentation Link Check Results✅ Absolute links check passed |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2c8e2a2 to
cda47ac
Compare
| ) | ||
| schema.update(update) | ||
| session.add(schema) | ||
| session.commit() |
There was a problem hiding this comment.
If the commit fails, shouldn't we delete the potentially newly created secret, similar to the create method?
There was a problem hiding this comment.
Hmm, valid but making this operation atomic is more complex that is sounds. Current code separate secret & metadata updates which is the simplest solution imo.
| try: | ||
| reference_id = UUID(values[_WEBHOOK_SECRET_REFERENCE_ID_KEY]) | ||
| reference_key = values[_WEBHOOK_SECRET_REFERENCE_VALUE_KEY] | ||
| referenced_values = self._get_secret_values(reference_id) |
There was a problem hiding this comment.
Not sure if this is going to hurt with a potentially large amount of events coming in (imagine a GH webhook for lots of events in a huge organization). Every authentication check would now require one extra secret call that could be avoided by not allowing referencing existing secrets (or allow referencing it on creation, but don't point to it but instead just duplicate the value).
Would also reduce some of the complexity in the rotate handling. WDYT?
There was a problem hiding this comment.
Hmm, duplicating the value won't work that easily. We would need to keep it updated when it is rotated. I like referencing the secret as UX but you are right unless a reliable cacheing layer is available we should avoid it for now.
There was a problem hiding this comment.
What I meant was essentially: We just copy it from their original secret, but from that point on it's completely disconnected. As in, rotating the original secret wouldn't work, they would need to rotate it via the webhook rotate request. But I guess that's also confusing.
In the current implementation though, the user wouldn't even know what secret they've connected to the webhook, which is also not ideal IMO.
There was a problem hiding this comment.
So, current code optimizes a bit the Db and Store I/O by reducing operations to a minimum:
- We drop support for more expensive secret reference
- We make webhook integration resolution at the intake endpoint super light
- We make stats update 1-pass (converted nested stats to flat table).
9926a9d to
b7da142
Compare
b7da142 to
1124d13
Compare
478527f to
0004e50
Compare
0004e50 to
7e976a0
Compare
- Flat stats table - single query atomic update (intake endpoint imp) - Drop support for secret ref (reduce secrets store calls) - Lightweight webhook resolution (intake endpoint imp)
d531372
into
feature/webhook-triggers
Describe changes
Introduces a new project-level component called webhook-integrations. Users can use the feature to create an integration point within ZenMl for events on extternal platforms (like Github).
The PR focuses on getting CRUD functionality in place (CLI, SDK, Rest, SQL). Integration with trigger code is out of scope and will be a follow-up branch.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes