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

Align API spec format with the latest Commonalities agreements #34

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

jpengar
Copy link
Collaborator

@jpengar jpengar commented Mar 21, 2023

Address issue #33

Align API spec format with the latest Commonalities agreements:

  • Commonalities - adjust the error status field, which will eventually be an integer (not a patterned string): PR #145.
  • Commonalities - adjust the schema references in the errors responses and apply the agreed solution: Issue #151 & PR #162.
  • Commonalities - change the format of the API fields to lowerCamelCase: Issue #157, Discussion #169 & PR #171.

In addition, this PR includes:

  • Some minor tweaks to non-functional definitions in the API spec (e.g. reusing the x-correlator definition).
  • Removal of max_allowed missed as part of RFC4594 related changes made as a result of issue #23.

bigludo7
bigludo7 previously approved these changes Mar 22, 2023
Copy link
Collaborator

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

Look good for me. Thanks

Copy link
Collaborator

@rartych rartych left a comment

Choose a reason for hiding this comment

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

I have doubts about adding the definition of X-Correlator header. See https://github.com/camaraproject/WorkingGroups/blob/main/Commonalities/documentation/API-design-guidelines.md#9-architecture-headers and the Eric's comment in
camaraproject/WorkingGroups#124
The reason I originally raised this issue was to get an agreement on which headers should explicitly be included in OAS API definition files. And the answer is "None of the headers listed in the API Design Guidelines".

@monamok
Copy link
Collaborator

monamok commented Mar 23, 2023

I have doubts about adding the definition of X-Correlator header.

Hello @rartych, the headers that should not be included in the defenitions are standard headers specified in section 3.5 where it says: "To avoid cluttering the CAMARA OAS (Swagger) definition files, the above headers must not be included explicitly in the API definition files even when supported, as it can be assumed that developers will already be familiar with their use."
x-correlator comes in section 9 and can be included.

@monamok monamok requested a review from rartych March 28, 2023 07:29
@monamok
Copy link
Collaborator

monamok commented Mar 30, 2023

@bigludo7 could you please review the changes and share your feedback? Thank you.

Copy link
Collaborator

@rartych rartych left a comment

Choose a reason for hiding this comment

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

LGTM.

@monamok monamok merged commit f5a0c45 into main Mar 31, 2023
@jpengar jpengar deleted the jpengar/api-spec-adaptation-to-commonalities branch April 10, 2023 08:18
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.

4 participants