camaraproject / Commonalities

Repository to describe, develop, document and test the common guidelines and assets for CAMARA APIs
Apache License 2.0
12 stars 25 forks source link

Create subscription-notification-template.yaml #189

Closed bigludo7 closed 4 months ago

bigludo7 commented 5 months ago

Create subscription-notification-template.yaml aligned with CloudEvents subscription model. Refer to https://github.com/camaraproject/Commonalities/issues/185

What type of PR is this?

Add one of the following kinds:

What this PR does / why we need it:

Create subscription-notification-template.yaml aligned with CloudEvents subscription model.

Refer to https://github.com/camaraproject/Commonalities/issues/185

Which issue(s) this PR fixes:

Fixes #185

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs
shilpa-padgaonkar commented 5 months ago

@bigludo7 : Looks like some copy paste error. Could you kindly check L757?

patrice-conil commented 5 months ago

Because I worked on this with @bigludo7, I'll let you approve or not this proposal

shilpa-padgaonkar commented 5 months ago

Have read PR#177 and I would like to propose some simplification in order to have a common guideline and avoid two potential models (using or not using subscription).

Therefore in summary, the exception in explicit-subscriptions APIs would be not to use the "resource", which is in spirit align with current commonalities (optional) and in the case of these APIs is common (all endpoints has "subscription" resource, so it could be avoided)

Probably it will need "little" rewording across PR#177. Key sentence (Line 1358):

To ensure consistency across Camara subprojects, it is necessary that explicit subscriptions are handled within separate API/s. It is recommended when possible to append the keyword "subscriptions" at the end of the API name. For e.g. device-roaming-subscriptions.yaml

Proposed to modify to:

To ensure consistency across Camara subprojects, it is required that explicit subscriptions are handled within separate API/s. It is mandatory to append the keyword "subscriptions" at the end of the API name. For e.g. device-roaming-subscriptions.yaml

Therefore in the context of PR#189 we could align to:

POST -> api-name:event-type:grant-level GET -> api-name:read DELETE -> api-name:delete

cc @bigludo7, @shilpa-padgaonkar WDYT?

@PedroDiez : That was originally the text I had put in the PR to mandate the keyword "subscriptions" in the API name :) But @bigludo7 mentioned that we could keep it open. I personally would like to mandate it and simplify things. @bigludo7 : Would it be ok for you as well?

bigludo7 commented 5 months ago

Have read PR#177 and I would like to propose some simplification in order to have a common guideline and avoid two potential models (using or not using subscription). Therefore in summary, the exception in explicit-subscriptions APIs would be not to use the "resource", which is in spirit align with current commonalities (optional) and in the case of these APIs is common (all endpoints has "subscription" resource, so it could be avoided) Probably it will need "little" rewording across PR#177. Key sentence (Line 1358):

To ensure consistency across Camara subprojects, it is necessary that explicit subscriptions are handled within separate API/s. It is recommended when possible to append the keyword "subscriptions" at the end of the API name. For e.g. device-roaming-subscriptions.yaml

Proposed to modify to:

To ensure consistency across Camara subprojects, it is required that explicit subscriptions are handled within separate API/s. It is mandatory to append the keyword "subscriptions" at the end of the API name. For e.g. device-roaming-subscriptions.yaml

Therefore in the context of PR#189 we could align to: POST -> api-name:event-type:grant-level GET -> api-name:read DELETE -> api-name:delete cc @bigludo7, @shilpa-padgaonkar WDYT?

@PedroDiez : That was originally the text I had put in the PR to mandate the keyword "subscriptions" in the API name :) But @bigludo7 mentioned that we could keep it open. I personally would like to mandate it and simplify things. @bigludo7 : Would it be ok for you as well?

@shilpa-padgaonkar @PedroDiez : Not a blocker for me so OK for me as well, as it has your preference.

shilpa-padgaonkar commented 5 months ago

@bigludo7 @PedroDiez , Gr8, then i will make changes accordingly in the scope PR

bigludo7 commented 4 months ago

Hi @shilpa-padgaonkar @PedroDiez
Are they still blocking point for the 0.4 release to merge this one? ( Let's try to target end of the week as I plan next to update accordingly the design accordingly. Thanks.

shilpa-padgaonkar commented 4 months ago

Thanks a lot @bigludo7 for this PR and @PedroDiez for your detailed review!

bigludo7 commented 4 months ago

Thanks a lot @PedroDiez & @shilpa-padgaonkar for your help - great team work.

I've committed last minor editorial from Pedro so this dismissed your approval Shilpa.

Once you'll both approved, I guess we can merge this @rartych ?

rartych commented 4 months ago

LGTM