-
Notifications
You must be signed in to change notification settings - Fork 111
docs(webhooks): create webhooks API spec #308
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
|
This PR should be merged into a new branch named webhooks (or something like it), the same thing that has been done with processors. @enudler @NivLipetz Do you agree? if so you have the privilege to open a new branch on |
| items: | ||
| type: string | ||
| description: The url of where to send the webhook with the report information | ||
| format: uuid |
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.
there is two use cases for webhooks
- ad hoc webhook per specific test (what we have today)
- global webhooks for all predator tests (what we want to add)
regarding 1:
we can keep the current behavior, maybe add to the job resource also param which defines the 'format'
or maybe we can change it to use ids (although it's breaking change, although it's mainly internal use) and create ad-hoc webhook which will be one time (won't be returned at get all results)
if we do it need add index on this column (one_time)
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 wanted to allow the user to specify what webhooks he would like to attach to that job(that is a breaking change, true, but not hard to migrate),
maybe webhooks resource should have global property, which will define if want to apply them globally.
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 agree that we should change this to use UUIDs because URL, although keeps same functionality without breaking it, is just another way to use webhooks and we should merge the webhooks feature into only one way of use, and not have 2 different ones because of "legacy" code
I agree with the global property. As i see it, when users createsa webhook, they can either mark it as global and it will take into effect in all future test runs, or they can assign it to a specific test run (Job) - as is done here with a list of UUIDs
I don't have a problem with past webhooks functionality in versions 1.3 and below to be broken on 1.4 and anyone upgrading to 1.4 has to update their webhooks - once it'll be available in the UI it should be a matter of minutes and much more flexible than it is now
* docs(webhooks): create webhooks API spec
* docs(webhooks): create webhooks API spec
* docs(webhooks): create webhooks API spec
* docs(webhooks): create webhooks API spec
#307
Todo:
[X] Connect reports to webhooks
Pipeline broke since reports now hold a list of webhooks ids instead of urls