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 API-design-guidelines for subscription #198

Merged
merged 32 commits into from
Jun 4, 2024

Conversation

bigludo7
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

Reformulate the subscription part to align with our decision to move to CloudsEvent subscription model. This PR is aligned with the PR #189 which provide an examples.

Which issue(s) this PR fixes:

Fixes #185 #153 #149 #140

Special notes for reviewers:

Changelog input

 release-note
- Move to cloudsEvents subscription model
- Add `initialevent` management
- Add draft subscription status management

Additional documentation

This section can be blank.

docs

documentation/API-design-guidelines.md Show resolved Hide resolved
documentation/API-design-guidelines.md Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md 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
Copy link
Contributor

@AxelNennker AxelNennker left a comment

Choose a reason for hiding this comment

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

sinkCredential is not explained enough.
Is it a shared secret provided by the API consumer?
Do we enforce that the shared secret is different and random enough for each subscription?

Is the sinkCredential a signed and encrypted JWT? With an aud of sink?
What is the value of source? Is it the value of issuerthe the AZ metadata? Or is the source maybe API specific (and the event still have different types for the same API?

documentation/API-design-guidelines.md Show resolved Hide resolved
documentation/API-design-guidelines.md Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
@AxelNennker
Copy link
Contributor

I changed my mind about ACCESSTOKEN. There is not much Camara can do on enforcing security at the API consumer.
Somebody can please resolve the conversations. @shilpa-padgaonkar

bigludo7 and others added 6 commits May 26, 2024 21:35
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Improved sinkCredential description.
Rename webhook to sink for implicit subscription
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Rename webhook to sink for implicit subscription + accepted @PedroDiez proposals + proposal for token expiration

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

I changed my mind about ACCESSTOKEN. There is not much Camara can do on enforcing security at the API consumer. Somebody can please resolve the conversations. @shilpa-padgaonkar

Thanks @AxelNennker
@PedroDiez I can close this point?

Copy link
Contributor

@AxelNennker AxelNennker left a comment

Choose a reason for hiding this comment

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

  • phoneNumber format should be "corrrect" (if needed at all, better use sub)
  • add Authorization Header

documentation/API-design-guidelines.md 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 and others added 2 commits May 27, 2024 14:56
Co-authored-by: Axel Nennker <axel.nennker@telekom.de>
Co-authored-by: Axel Nennker <axel.nennker@telekom.de>
Co-authored-by: Shilpa Padgaonkar <77152136+shilpa-padgaonkar@users.noreply.github.com>
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.

Need to think about Access Token Expiration. Get your view @bigludo7

Several editorial comments. The whole view looks fine to me

documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
documentation/API-design-guidelines.md 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
documentation/API-design-guidelines.md Outdated Show resolved Hide resolved
bigludo7 and others added 2 commits May 29, 2024 08:34
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Co-authored-by: Pedro Díez García <pedro.diezgarcia@telefonica.com>
Copy link
Collaborator Author

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

@shilpa-padgaonkar @AxelNennker I've accepted your proposal + changed the sentence line 1404.

@shilpa-padgaonkar
Copy link
Collaborator

@bigludo7 Thanks. There is another very small fix already proposed as suggestion in this comment by Axel #198 (comment) which will fix the missing authZ header in one of the examples.
After that its /LGTM :)

Co-authored-by: Axel Nennker <axel.nennker@telekom.de>
@bigludo7
Copy link
Collaborator Author

bigludo7 commented Jun 3, 2024

Thanks @shilpa-padgaonkar for the pointer on @AxelNennker comment.
Commited!

@rartych rartych self-requested a review June 3, 2024 13:36
rartych
rartych previously approved these changes Jun 3, 2024
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
PedroDiez previously approved these changes Jun 4, 2024
@shilpa-padgaonkar
Copy link
Collaborator

@bigludo7 I noticed that there is a minor conflict you would need to resolve in the file which is blocking the merge. If you could kindly do this, we would go ahead and merge this PR as we have enough approvals now :)

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Jun 4, 2024

@bigludo7 I noticed that there is a minor conflict you would need to resolve in the file which is blocking the merge. If you could kindly do this, we would go ahead and merge this PR as we have enough approvals now :)

Thanks @shilpa-padgaonkar. Hope to have fixed it.

Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
@rartych rartych self-requested a review June 4, 2024 08:46
rartych
rartych previously approved these changes Jun 4, 2024
Co-authored-by: Axel Nennker <axel.nennker@telekom.de>
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

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.

Common proposal to tackle subscription-based open issues.
5 participants