-
Notifications
You must be signed in to change notification settings - Fork 9
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
Enhancements for Carrier Billing v0.2 - Part I #119
Conversation
Thanks @PedroDiez |
Hi @bigludo7, Many thanks for the review!. I wanted to add these ones in advance as were easy to manage. |
Ready for review: @bigludo7, @rartych, @alabajnaid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current API Design Guidelines states that X-Correlator header is not Required in OAS Definition.
Some clarifications are currently proposed in camaraproject/Commonalities#88
BTW, X-Correlator
or x-correlator
or more popular header: X-Correlation-ID
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good. A small fix for the application/cloudevents+json that we have in the notification. I will take a second look on the state engine later today.
requestBody: | ||
description: Creates a new carrier billing payment notification | ||
content: | ||
application/json: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be: application/cloudevents+json:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and here also: camaraproject/Commonalities#86
We have discussed this point in Commonalities call 2 weeks ago and the team was agreed to align with Cloud Events application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarification!
Hi Rafał, Regarding the fact of not indicating the header in the yaml is not a good approach, because there is no contingency to inform consumers that they can use the header. Consumers do NOT have to know the CAMARA API Design guidelines. Then, for the time being I prefer to keep the "x-correlator" header (HTTP headers are case insensitive). I have already commented in commonalities for alignment |
Co-authored-by: Rafal Artych <[email protected]>
Co-authored-by: Rafal Artych <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look fine for me . Thanks !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
This PR deals with some Issues whose output is already agreed:
/documentation/API_documentation/Carrier_Billing_API.md
and aligned with current model approach for QoD and Home Devices QoD APIs. Providing details about state transitions -in the current context, without considerations of Issue#116]Which issue(s) this PR fixes:
Fixes #97, #105, #106, #107, #108, #112
Special notes for reviewers:
COMPLETED. Ready for review