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

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

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

rartych
Copy link
Collaborator

@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
Collaborator

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

Kevsy commented Aug 6, 2024

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

@rartych
Copy link
Collaborator 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
Collaborator

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

@rartych
Copy link
Collaborator Author

rartych commented Aug 7, 2024

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

Corrected by rartych@52d754e

Copy link
Collaborator

@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
Collaborator

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