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 34 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 #

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 !!

@rartych
Copy link
Collaborator

rartych commented Oct 28, 2024

I agree with @PedroDiez proposals
#295 needs to be resolved separately as it can cover not only subscriptions.

Remove 403_INVALID_TOKEN_CONTEXT
adding GENERIC_422_UNNECESSARY_IDENTIFIER
adding GENERIC_422_MISSING_IDENTIFIER
@bigludo7 bigludo7 changed the title (WIP) Subscription & events for Commonalities 0.5.0 Subscription & events for Commonalities 0.5.0 Oct 29, 2024
@bigludo7
Copy link
Collaborator Author

Hello team (in particular @PedroDiez @rartych @shilpa-padgaonkar )
PR ready for a first review - Thank you for your help

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.

Minor comment

LGTM so far

PedroDiez
PedroDiez previously approved these changes Oct 30, 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.

LGTM at this point

NOTE:

Suggest to set -> x-camara-commonalities: wip as it is ongoing work within the MetaRelease

@rartych
Copy link
Collaborator

rartych commented Nov 5, 2024

We have :

       types:
          description: |
            Camara Event types eligible to be delivered by this subscription.
            Note: for the Commonalities meta-release v0.4 we enforce to have only event type per subscription then for following meta-release use of array MUST be decided
             at API project level.
          type: array

I propose to change the note to:
Note: for the current Commonalities version (v0.5) only one event type per subscription is allowed, yet in the following releases use of array of event types SHALL be specified without changing this definition

I am also thinking about including in the template the pattern for event types: org.camaraproject.<api-name>.<api-version>.<event-name>
In Geofencing API we have "#/components/schemas/SubscriptionEventType" and "#/components/schemas/NotificationEventType" used to enumerate event types. But I am not sure if this way should be mandated in the template.
WYDT?

@rartych
Copy link
Collaborator

rartych commented Nov 5, 2024

We have:

      discriminator:
        propertyName: "type"
        mapping:
          org.camaraproject.api-name.v0.event-type: "#/components/schemas/Event-typeEvent"
          org.camaraproject.api-name.v0.subscription-ends: "#/components/schemas/EventSubscriptionEnds"

and

    Event-typeEvent:
      description: event structure for event-type event
      allOf:
        - $ref: "#/components/schemas/CloudEvent"
        - type: object

Event-typeEvent is not pure camelCase, so I propose to change it to something like ApiSpecificEvent or EventApiSpecific (compatible with EventSubscriptionEnds) or similar.
Then in mapping we would have something like:

        mapping:
          org.camaraproject.api-name.v0.api-event: "#/components/schemas/EventApiSpecific"

I would be good to indicate that multiple API specific event types can be used.

Add 2 exemples of event type
Add a note for event type name convention
Aligned event type example name
Event type in subscription managed in an enumeration
@bigludo7
Copy link
Collaborator Author

bigludo7 commented Nov 6, 2024

Thanks @rartych for your review.
I've took all you comments:

  • Add 2 exemples of event type instead of 1 to explicit that an API can manage several event type
  • Add a note for event type name convention
  • Aligned event type example name
  • Event type in subscription managed in an enumeration

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.

NOTES:

  • Pending to remove 415, 500, 503 Errors

Removed code 415, 500 & 503
@bigludo7
Copy link
Collaborator Author

NOTES:

  • Pending to remove 415, 500, 503 Errors

Done ! 415, 500 & 503 removed.

PedroDiez
PedroDiez previously approved these changes Nov 13, 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.

LGTM

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

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Nov 18, 2024

@rartych @PedroDiez Not sure what I did previously but seem fix for #326 was not correctly done. I've done it again and this time look good.

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.

5 participants