Skip to content

Update event-subscription-template.yaml: Attribute startsAt set optional, info and servers object, sinkCredential description #267

Merged
PedroDiez merged 3 commits intocamaraproject:mainfrom
rartych:startsat-optional
Aug 7, 2024
Merged

Update event-subscription-template.yaml: Attribute startsAt set optional, info and servers object, sinkCredential description #267
PedroDiez merged 3 commits intocamaraproject:mainfrom
rartych:startsat-optional

Conversation

@rartych
Copy link
Copy Markdown
Contributor

@rartych rartych commented Aug 2, 2024

What type of PR is this?

  • correction

What this PR does / why we need it:

Attribute startsAt is optional in API Design Guidelines, so it should be in event subscription template file .

Corrections made in info and servers object according to API Design Guidelines.

In SubscriptionRequest description moved to SinkCredential schema; as it is generic enough to be reused in the 2 $ref to this schema, and the allOf above removed.

Which issue(s) this PR fixes:

Fixes #262 #271

Special notes for reviewers:

Changelog input

Attribute startsAt set optional, info and servers object corrected in event-subscription-template.yaml

In Subscription Request description moved to SinkCredential schema; as it is generic enough to be reused in the 2 $ref to this schema, and the allOf removed.
@rartych rartych requested a review from jlurien August 5, 2024 12:54
@rartych rartych changed the title Update event-subscription-template.yaml: Attribute startsAt set optional, info and servers object Update event-subscription-template.yaml: Attribute startsAt set optional, info and servers object, sinkCredential description Aug 5, 2024
@PedroDiez
Copy link
Copy Markdown
Contributor

PedroDiez commented Aug 6, 2024

@rartych initial comment. It is also needed to update https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#121-subscription

image

Suggestion:
"Date when the event subscription has started. This attribute must not be present in the POST request as it is provided by API server. It ONLY must be present in GET endpoints once the subscription is effectively operative/working."

CLARIFICATION about my comment: Avoiding the use of "ACTIVE" status wording as in MetaRelease v0.4 the implementation of STATUS is optional

@PedroDiez PedroDiez mentioned this pull request Aug 6, 2024
@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Aug 6, 2024

As Cloudevents Subscriptions OpenAPI was used for the template, it defines 2 components: SubscriptionRequest for POST request and Subscription present only in responses. API Design Guidelines - as you indicated - should be corrected, but I would prefer to do it with the final release, and possibly split the table into 2 tables for request and response parameters in the next version of Commonalities. It would be good to have the input from @bigludo7 for that.
My initial proposal for attribute description:
"Date when the event subscription has started. This attribute must be present in the response when subscription creation process is completed"

The rationale for changing startsAt to optional in Subscription component is the asynchronous subscription creation , when its starting moment is not known yet (#262). It should be decided by subprojects if the asynchronous case is covered by the API.

Copy link
Copy Markdown
Collaborator

@Kevsy Kevsy left a comment

Choose a reason for hiding this comment

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

Line 626: terminationDescription needs a description to avoid a spectral linter warning

@Kevsy
Copy link
Copy Markdown
Collaborator

Kevsy commented Aug 6, 2024

Line 453: Spectral warning, "Event-typeNotification should be pascal case (UpperCamelCase)"

@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Aug 7, 2024

Line 626: terminationDescription needs a description to avoid a spectral linter warning

Let's discuss this attribute within #243

'Event-typeNotification' changed to 'EventTypeNotification'
@PedroDiez
Copy link
Copy Markdown
Contributor

As Cloudevents Subscriptions OpenAPI was used for the template, it defines 2 components: SubscriptionRequest for POST request and Subscription present only in responses. API Design Guidelines - as you indicated - should be corrected, but I would prefer to do it with the final release, and possibly split the table into 2 tables for request and response parameters in the next version of Commonalities. It would be good to have the input from @bigludo7 for that. My initial proposal for attribute description: "Date when the event subscription has started. This attribute must be present in the response when subscription creation process is completed"

The rationale for changing startsAt to optional in Subscription component is the asynchronous subscription creation , when its starting moment is not known yet (#262). It should be decided by subprojects if the asynchronous case is covered by the API.

It is fine to wait to Ludovic for the API design guidelines, and fine to have for final Commonalities Release v0.4.0.

Indeed i like more your proposal Rafal for the wording:

"Date when the event subscription has started. This attribute must be present in the response when subscription creation process is completed"

Copy link
Copy Markdown
Contributor

@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

@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Aug 7, 2024

Line 453: Spectral warning, "Event-typeNotification should be pascal case (UpperCamelCase)"

Corrected by rartych@52d754e

Copy link
Copy Markdown
Contributor

@shilpa-padgaonkar shilpa-padgaonkar left a comment

Choose a reason for hiding this comment

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

/LGTM

@PedroDiez
Copy link
Copy Markdown
Contributor

Merging this PR as having 2 approvals an Patrice is on holidays.

Let's followUp this comment by Kevin within #243

@PedroDiez PedroDiez merged commit 60e5694 into camaraproject:main Aug 7, 2024
rartych added a commit to rartych/Commonalities that referenced this pull request Aug 7, 2024
@rartych rartych deleted the startsat-optional branch March 12, 2025 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attribute startsAt should be non-mandatory

4 participants