-
Notifications
You must be signed in to change notification settings - Fork 59
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
Aligned notifications with guideline #155
Aligned notifications with guideline #155
Conversation
Update of the notification event related fields based on the API design guideline
Have added some suggestions for reviews, especially @eric-murray @bigludo7 and @sfnuser who were involved within the definition of the chapter in the guideline document. |
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.
Very good aligned with subscription & notification guideline.
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.
@akoshunyadi Thanks. Looks good.
One comment - As we have added notificationAuthToken
, should we remove apiKey
security scheme? However, I wonder what could be the appropriate securitySchemes
we could specify for notification - perhaps {} & bearerAuth
?
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.
- Event has to be remodeled with discriminator at the right level
- We have to adapt callback to the new strcture:
paths:
/sessions:
post:
...
callbacks:
notifications:
"{$request.body#/weebhook/notificationUrl}":
...
I also think we are not specifying the callback correctly since the beginning. The POST /notification operation must not be at first level of /paths, but within the POST /sessions callbacks, as indicated in https://github.com/OAI/OpenAPI-Specification/blob/main/examples/v3.0/callback-example.yaml
@jlurien I think your both points are valid. Putting /notification below the POST /sessions decreases the chance for misinterpretation of the /notification endpoint |
Probably keep API Key but carried by header instead of query string. |
@patrice-conil any preference from Orange? |
@bigludo7 @patrice-conil the question at hand is if it is also from your perspective ok to omit apiKey completely from the spec (as we discussed in today's call). The notion is that this would be in line with the new design guidelines for notifications. |
According to the community discussion, please remove API key. |
@hdamker, we are ok to remove apiKey, notificationAuthToken will be fine. |
|
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.
Thanks @akoshunyadi. Updated comments
@akoshunyadi with the merge of #138 we have now some merge conflicts. May I ask you to rebase? |
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@bigludo7 @eric-murray @jlurien @sfnuser From my point of view we are done. Can you please see if your comments are done and confirm with an approval? |
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.
LGTM
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.
/LGTM - thanks @akoshunyadi for the rebase.
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Update of the notification event related fields based on the API design guideline
Which issue(s) this PR fixes:
Fixes #154
Special notes for reviewers:
Changelog input
Additional documentation