Skip to content

Alignment with Commonalities Subscription Model - APIs Subscription#250

Merged
akoshunyadi merged 47 commits intocamaraproject:mainfrom
sachinvodafone:main
Jan 31, 2025
Merged

Alignment with Commonalities Subscription Model - APIs Subscription#250
akoshunyadi merged 47 commits intocamaraproject:mainfrom
sachinvodafone:main

Conversation

@sachinvodafone
Copy link
Collaborator

@sachinvodafone sachinvodafone commented Jan 24, 2025

What type of PR is this?

Add one of the following kinds:

  • correction

  • cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #235
Fixes #241

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

@sachinvodafone sachinvodafone changed the title Alignment with Commonalities Subscription Model - Roaming Status Subscription Alignment with Commonalities Subscription Model - APIs Subscription Jan 24, 2025
@akoshunyadi
Copy link
Collaborator

@sachinvodafone thanks for your effort! I'm going to review it this evening....

@sachinvodafone
Copy link
Collaborator Author

@sachinvodafone thanks for your effort! I'm going to review it this evening....

I am finding one error in Linter and trying to fix it

@sachinvodafone
Copy link
Collaborator Author

Hello @akoshunyadi @sachinvodafone I'm not sure we have a global decision on this topic. For me we have to keep the device in the event specialization provided in the yaml as we have no indication here: https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#event-notification-definition I note that in the guideline the subscriptionId is optional so it makes sense to keep the device identifier else it will be impossible to correlate the event to a device.

We should keep device, but as an optional parameter for both EventNetworkTypeChangeData and EventSubscriptionEnds. Currently it is required. If the subscription is created using an explicit device identifier, then it is OK to include it in the event notifications. Otherwise, it should not be included.

At some point we might consider returning the sub parameter of the ID token here when subscriptions are created using a 3-legged token, as the API consumer already knows the sub when the got their access token. But that is for a later version.

Marked it as optional.

eric-murray
eric-murray previously approved these changes Jan 29, 2025
Copy link
Collaborator

@eric-murray eric-murray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now reviewed this, made a few changes, and am happy. So I am approving. Please provide any additional review comments promptly.

Because these APIs make extensive use of discriminators, I'd advise using ReDoc to view the updated OAS definitions, rather than the Swagger editor.

Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akoshunyadi akoshunyadi merged commit 02accb6 into camaraproject:main Jan 31, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"device" should not be mandatory for subscription APIs Align subscription APIs with Commonalities 0.5.0

5 participants