Skip to content
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

Subscription & events for Commonalities 0.5.0 #313

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

bigludo7
Copy link
Collaborator

@bigludo7 bigludo7 commented Oct 10, 2024

What type of PR is this?

Add one of the following kinds:

  • documentation

What this PR does / why we need it:

This PR covers update to subscriptions & events part for Commonalities 0.5.0

Which issue(s) this PR fixes:

Fixes #243, #276, #289, #291, #300, #303, #304, #326

Special notes for reviewers:

Not ready for review

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

Add  SUBSCRIPTION_UNPROCESSABLE in terminationReason enum for subscription ends event.
Solve camaraproject#243
Explicit subscription-end termination reason usage.
Fix camaraproject#243
Update the "sink" property description
Fix camaraproject#289
Update the "sink" property descriptionFix camaraproject#289
Add information for exiresAT + limitation to subscriptionMaxEvent and subscriptionExpireTime.
Fixes camaraproject#303
Proposal for camaraproject#276

- POST /subscriptions response will have only `id`
- removed `sinkCredentials` from GET representation
Add a note on combined usage of initialEvent and subscriptionMaxEvents (camaraproject#299)
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

18/OCT:

Fine to me so far. Editorial suggestions only

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
@bigludo7
Copy link
Collaborator Author

18/OCT:

Fine to me so far. Editorial suggestions only

Thanks you Pedro !
Everything considered !!

PedroDiez
PedroDiez previously approved these changes Nov 22, 2024
Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

One editorial
LGTM in advance

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
- If 3-legged access token is used, the POST and GET responses must not provide any device identifier.
- If 2-legged access token is used, the presence of a device identifier in the response is mandatory and should be the same identifier type than the one provided in the request.

Application of data minimization design must be considered by the API working group for event structure definition.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we mandate above that in case of 2-legged access token response must include device identifier, how will the API working groups take decisions about data minimization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have discussed a bit this topic in Device Location sub projet.

If we use the 2-legs the device identifier must be mandatory but then in the subscription answer and in the notification itself we can avoid to send this identifier. I proposed some suggestion here: camaraproject/DeviceLocation#270 like adding a capability for the API consumer to indicate if the device & area information must be part of the event.

I think we probably cannot add more in commonalities for this release due to deadline but we can continue to explore the topic in API sub project. WDYT?

Copy link
Collaborator

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment