camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
37 stars 60 forks source link

Aligned notifications with guideline #155

Closed akoshunyadi closed 1 year ago

akoshunyadi commented 1 year ago

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

Update of the notification event related fields based on the API design guideline

Additional documentation

hdamker commented 1 year ago

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.

akoshunyadi commented 1 year ago
  • 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

emil-cheung commented 1 year ago

@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?

Probably keep API Key but carried by header instead of query string. I have created an issue about it https://github.com/camaraproject/QualityOnDemand/issues/157

akoshunyadi commented 1 year ago

@patrice-conil any preference from Orange?

hdamker commented 1 year ago

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.

emil-cheung commented 1 year ago

@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?

Probably keep API Key but carried by header instead of query string. I have created an issue about it #157

According to the community discussion, please remove API key.

patrice-conil commented 1 year ago

@hdamker, we are ok to remove apiKey, notificationAuthToken will be fine.

eric-murray commented 1 year ago

apiKey definition in components remains, but is now unused. This should also be deleted.

hdamker commented 1 year ago

@akoshunyadi with the merge of #138 we have now some merge conflicts. May I ask you to rebase?

hdamker commented 1 year ago

@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?