-
Notifications
You must be signed in to change notification settings - Fork 29
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.md #88
Conversation
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
Need to check existing API because I think some of them reference "x-correlator" in lowercase.
@patrice-conil On header naming, I'm happy for ALL headers to be written in lower case only, as it makes no difference to their use or purpose, but that is for a separate issue. The current definition of "X-Correlator" was proposed by Telefonica, and I don't want to make unilateral proposals to change that without understanding why they originally chose Pascal case. |
Just to comment here and aligned with @RubenBG7 that, understanding the proposal, this does not mean to remove the header "x-correlator" from yaml. There is no contingency to inform consumers that they can use the header if not indicated in the YAML. Consumers do NOT have to know the CAMARA API Design guidelines. Point that was commeted by TEF in latest commonalities meeting (as per alignment done with Rubén) |
@PedroDiez |
Get your point Eric, what i want to highlight is the fact that it is not required in OAS definition does not enter in contradiction with its indication in YAML (for sure as optional field) and that also help API consumers |
@PedroDiez @eric-murray that will not work for CAMARA - we need to be consistent across the APIs in CAMARA and should not keep this point open for decision in every sub project. My understanding is that it is agreed that the support for X-correlator is mandatory for every API provider (which means if the APi consumer provides the header, it has to be in the response). That is valid for every CAMARA API. But then we need to be consistent regarding including it into the YAML file of CAMARA APIs. It can't be that some APIs include it and others not. So the column "Required in OAS Definition" should rather be "Included in OAS Definition" and than binding for all CAMARA APIs. The next question is than if the value should be "No" or "Yes". I tend to "No" (not to clutter the YAML files). But we might need anyway general documentation
|
One other point which is related to the chapter Architecture Header but may require a new issue (or is already addressed in another issue?): What is this "X-Version" about ... it's expected from the API consumer ... what is the meaning, what can the API provider expect here, what should the API provider do with it? Currently this is completely undefined ... therefore I would rather delete it from the document (which would allow to rename the chapter to "Correlation ID" or similar. |
@hdamker Regarding |
As we comment in the last commonalities, it is still pending how we inform to the consumers that the x-correlator is available to be used in all CAMARA APIs. "Consumers shall known the API design guidelines" IT IS NOT A SOLUTION. Because consumers shall not Know anything about design guidelines, there exist for designers and not for consumers. When a contingency to that issue will be agreed, as @hdamker says we shall apply it to all APIs to be compliant with ourselfs |
I agree with @eric-murray, |
I propose to close the discussion here and merge the PR. |
What type of PR is this?
What this PR does / why we need it:
The current API guidelines do not specify the conditions under which the API provider includes the
X-Correlator
header in their response to an API request. This PR clarifies that the API provider:X-Correlator
header in the response if it is included in the requestX-Correlator
header in their responseWhich issue(s) this PR fixes:
N/A
Special notes for reviewers:
None
Changelog input
Additional documentation
None