-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add generic notification OAS file #41
Conversation
Add generic notification OAS file linked to issue camaraproject#8
@bigludo7 : Thanks for the PR contribution. Under the #8 I had suggested ( #8 (comment) ) the use of CloudEvents as we are now moving towards a more generic way on how to handle events in Camara. Is there a reason why we try to redefine Event in Camara and not use an industry standard like CloudEvents or a generic way to subscribe (https://github.com/cloudevents/spec/blob/main/subscriptions/subscriptions-openapi.yaml) It would be great to know if we have already identified shortcomings in the CloudEvents spec and hence made the decision to define our own new way in Camara. |
@shilpa-padgaonkar My proposal target is to be aligned with the guideline and particularly with already subscription/notification designed in our API. |
Hi @shilpa-padgaonkar, |
@bigludo7 @patrice-conil Please see first my comment on the issue #8. I don't want to comment on this PR here in detail as I think we shouldn't finalize it. But if then there are several open points which need to be clarified:
Especially solving the last point and the general need to have a future proof design which does not only fit to webhooks are points which are easier to solve by adapting an existing industry standard like CloudEvents then to spend here the time just to have a CAMARA proprietary "standard". Otherwise we are missing our aim for developer friendly APIs. |
description: | | ||
Notification received on subscription listener side. | ||
paths: | ||
/eventNotifications: |
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 endpoint is the one indicated vía {$request.body#/webhook/notificationUrl}.
Then, it is not really required to append "/eventNotifications"
So maybe we can agree on indicating like:
/{$request.body#/webhook/notificationUrl}
And in the description comment:
INFORMATIVE ENDPOINT. The value of this endpoint is freely declared by each client app by means of resource-based subscription or instance-based subscription. /{$request.body#/webhook/notificationUrl} is just a convention naming referring to an absolute URL, indeed the one indicated by API client in the triggering of the procedure (resource-based or instance-based).
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.
Hi @PedroDiez,
Thanks for your comments.
As this is a generic OAS, we can't refer to {$request.body#/webhook/notificationUrl} that doesn't exists in this context, I'll replace it by /your_webhook_notificationUrl and include your description in #43
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.
Hi @patrice-conil
Have seen the update in PR#43, I think it would apply in the same fashion in this PR, isn't?
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.
Hi @PedroDiez,
yes, but I'm waiting for a decision before reporting in this PR.
- Event Notification | ||
summary: "Notification endpoint to notify listener that an event had occurred" | ||
description: Notification endpoint to notify listener that an event occurred. This endpoint must be exposed on the listener side. | ||
operationId: postNotification |
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.
maybe rename to "sendNotification"
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.
Done 😄 in #43
@PedroDiez, @shilpa-padgaonkar, @rartych, |
Add explicit sentence for date time format in chapter 11.5
Close PR has the guideline proposal will promote use of Cloud event format |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Add generic notification OAS file
open for discussion.
Which issue(s) this PR fixes:
Fixes #8
Special notes for reviewers:
Changelog input
Additional documentation