-
Notifications
You must be signed in to change notification settings - Fork 21
feat: implement destination filter #594
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Should this be opt-in the same way topics are? 🤔 |
Not 100% sure what you mean, I think it is opt-in. As in |
|
Some |
|
ah I see. Hmmm not sure, I don't see a strong reason why someone would prefer not to have this feature. Maybe performance concern? If someone is opinionated enough, maybe they already provide their own UI and can choose not to use this feature? Maybe we can support a flag to disable this in the portal only? I just think disabling this feature as a whole may be a bit complicated because of the Destination schema (Destination API). |
|
Yeah agree really that's a portal config if anything since if you implement your own UI then you get to choose that way. The problem if we don't give control over it is that a user may deploy the portal and later want to build their own UI or migrate to another system and be stuck with that backward compatibility. Given it does create some amount of "lock-in" with Outpost, it should be something you can decide to expose to your users or not.
Does that make sense? |
|
yup, all clear then. Please continue finalizing the UI changes the way you see fit. I'll take care of the portal flag for these 2 features. Will merge and release if you approve the PR |
alexbouchardd
left a comment
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 are missing validation that the filter have top level data or metadata and no other top level key since that would be invalid syntax. True both the for the API and UI validation
| type: object | ||
| nullable: true | ||
| additionalProperties: true | ||
| description: | |
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 {} doesn't make sense to me, a user should pass null to unset
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.
hey so quick note, i'll merge to a temp v0.10.0 branch now but we can discuss this further
good catch, I do agree that {} is not ideal. The reason tho is that the way we parse incoming request, in Golang null and undefined are the same -> nil, and for PATCH we treat nil as no update. We can still support null but it requires a bit more work which we can do, but I think this is an acceptable tradeoff for now. Do let me know tho. I agree, I don't love this DX and spent quite a bit of time thinking on this exact question
# Conflicts: # internal/portal/src/scenes/Destination/Destination.scss
* feat: port simple-json-match from js to golang * feat: destination model filter field * feat: destination filter api * refactor: centralize destination event matching * test: publishmq filter * test: e2e filter test * docs: openapi filter unset documentation * test: e2e destination api with filter * feat: portal filter ui * chore: Filter UI display improvements and syntax guide --------- Co-authored-by: Alexandre Bouchard <alexbouchardd@gmail.com>
implements #586
Destination Filter
Adds support for event filtering on destinations, enabling event routing based on event content—not just topic.
Features
filterfield on destinations allows filtering events based on JSON matching patterns$refoperator is not supported in this implementation.UI
CleanShot.2025-12-12.at.00.33.35.mp4