-
Notifications
You must be signed in to change notification settings - Fork 33
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 documentation #65
Conversation
Add documentation for the API Solve isue #57
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 think we should include documentation regarding the use of the sync/async use of the API. Something along the lines of indicating that the response code will be implementation-dependent.
And I would complete the async behaviour a bit more by pointing out that a GET request to /event-subscriptions/{eventSubscriptionId} should be made after the creation/deletion of the subscription to check if the operation was successful.
@fernandopradocabrillo |
@fernandopradocabrillo (additionnally to @eric-murray comment) |
|
||
## Device status event subscription | ||
|
||
This endpoint allows the API consumer to create and delete event subscriptions for specific roaming device status events. The CAMARA subscription model is detailed in the [CAMARA API Design Guidelines](https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md). |
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 guideline describes multiple subscription models. Maybe it would be easier for the user of this API if relevant information was written here instead,
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 updated the links to be hash-links to the most appropriate numbered section in the design guidelines. Unfortunately the sections on instance-based and resource-based subscriptions are not numbered, so cannot be directly hash-linked. This could be changed.
My preference is not to copy over information from the design guidelines, because then we have a maintenance issue whenever the design guidelines are updated. Of course, by hash-linking there is still an issue if the section names or numbers change, but probably more manageable.
Anyway, that's my proposal, but happy to discuss.
|
||
- `ROAMING_OFF` - This event is only triggered when the device changes from roaming ON to roaming OFF | ||
|
||
- `ROAMING_CHANGE_COUNTRY` - This event is triggered whenever the mobile country code (MCC) of the network to which the device is connected changes. Changing netorks within a given country does not trigger this event. |
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.
Small typo missing a "w" in networks.
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.
Fixed
@bigludo7 Yes! I think it definitely should be well documented in the guidelines since it is going to be the same behaviour for all the APIs. But, don't you think we should mention it briefly here too? To make it easier for the implementer to understand how it works.
@eric-murray I was considering that it would be better if the documentation reflects the full behaviour of the API and what should be expected in each scenario. In this case indicating that a GET must be done to ensure the existence or successful deletion of the subscription is to point out that for asynchronous scenarios, there is no other way to check that the operation has been performed successfully. What do you think? |
- fix typo
- Updated links to design guidelines to be hash-links to the most appropriate sections
- Added note that the response will not include additional device identifiers over those provided by the API consumer
@fernandopradocabrillo |
This endpoint describes the event notification that will be received by the subscription listener (API consumer) side when a subscribed event has occurred. A detailed description of the event notification mechanism is provided in the [CAMARA API Design Guidelines](https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#122-event-notification). | ||
|
||
**WARNING**: This endpoint must be implemented on the listener (API consumer) side, as a `POST /notifications` request will be made by the device status resource server to the url provided by notification listener. | ||
|
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.
Is /notifications mandatory?
Doesn't the webhook/notificationUrl define the complete url?
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 @akoshunyadi
Good question. I had assumed that it was mandatory for the notification endpoint name to match the name defined in the API definition. So either the API consumer includes that in notificationUrl
, or it gets added on automatically to whatever is specified.
However, looking at the example in the CAMARA Design Guidelines, I see that the API consumer specified "notificationUrl": "https://application-server.com"
but the notification was sent to https://application-server.com/v0/notifications
.
So now I'm asking "where did the v0
come from?". Rules around constructing the full notification URL need to be clarified.
But, despite that, my assumption was that if the API consumer had instead specified "notificationUrl": "https://application-server.com/notification"
, then that is exactly where the notification would be sent (rather than "https://application-server.com/notification/notification"
).
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.
As per my understanding, when the API consumer registers its webhook/notification URL with the API provider, the provider should perform validation on the URL to ensure it meets the required format that should be properly documented at our end. If there are any issues, such as missing components or incorrect structure (missing “/notification”), the API provider should respond with an appropriate error message indicating what went wrong and how the API consumer can correct the URL.
Also it may create some confusion to have extra endpoint in our swagger file as if client try to access “Try it out” feature then this extra endpoint will take him/her to http://localhost:9091//device-status/v0/notifications) that can confuse him/her.
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.
In QoD we decided not to add or expect "/notifications", just use the provided callback url
notifications:
"{$request.body#/webhook/notificationUrl}":
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.
Given that we have an explicit /notifications
endpoint defined here, and given the resource-based subscription example used in the design guidelines, I think we have to assume that the notification endpoint must end with /notifications
.
The question is what we should do if the notificationUrl
provided by the API consumer does not end with /notifications
? The example in the design guidelines suggests (more or less) that we should just add /notifications
to the endpoint provided by the API consumer.
But I agree with @Sachinsiso that a better option might be to reject it and get the API consumer to try again to avoid the risk that the API consumer has misunderstood how to implement this endpoint. This interpretation would require a change in the design guidelines.
I'd suggest to wait until @bigludo7 returns before discussing further, and keep this PR open in the meantime.
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,
will also align this with @fernandopradocabrillo, anycase I think it is good to wait until Ludovic returns.
I consider the comment is valid, and probably this can generate misunderstanding. When sending notification from Telco Operator to client, the endpoint is settled by {$request.body#/webhook/notificationUrl}, so really not required any append like "/notifications". The notification is sent to the endpoint provided by the client.
Think it is good time to have this discussion given there is an issue where a "traversal" API is being discussed/defined to cover this flow: camaraproject/Commonalities#8
I've identified here 2 topics all related to events: 1. use of Additionally there is a third point (3.) raised in commonalities that could impact this API: should we keep current notification model (https://github./camaraproject/Commonalities/pull/41) or move to cloud event notification (camaraproject/Commonalities#43). I suggest to wait until commonalities outputs for 1. & 3. before to move forward for this PR. |
Align with current Notification proposal
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.
To provide clarification about Notification update
Aligned with camaraproject/Commonalities#56
I propose to close this issue as the discussion is not about the documentation but to the shift to CloudEvent. I will open another PR with the new yaml proposal. |
New PR will be created for #74 |
Add documentation for the API
Solve issue #57
What type of PR is this?
What this PR does / why we need it:
This PR add the documentation in the API swagger.
Which issue(s) this PR fixes:
Fixes #57
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.