Update API definition and Test definition#10
Conversation
|
@lihaojie-CMCC Please reserve the PR title "Release r1.1 with 0.1.0-alpha.1" for the actual release PR and not for the initial seed upload to the repository as in this PR. Only the release PR should set an actual version, the current "work in progress" should use "wip" as version. See https://github.com/camaraproject/ReleaseManagement/blob/main/documentation/API_Release_Guidelines.md On first glance there are several points which need to be adapted for CAMARA in the current files in the PR ... let me know if you want to get some hints / a review of it. |
hdamker
left a comment
There was a problem hiding this comment.
Hi @lihaojie-CMCC,
I’ve made some comments on the current upload that need some extra work to meet the CAMARA API Design Guidelines. There are more, so please check out the links I’ve shared.
On top of that, I suggest deploying the linting_rules within the repository, as described in https://github.com/camaraproject/Commonalities/blob/main/documentation/API-linting-Implementation-Guideline.md. This will help ensure that the API definition complies with the API Design Guidelines.
| type: string | ||
| description: Optional reference to the knowledge base source (e.g., "kb123_section5"). | ||
| error: | ||
| $ref: '#/components/schemas/Error' |
There was a problem hiding this comment.
The error schema cant't be used within a 200 response (it has it's own HTTP code different from 200)
| @@ -0,0 +1,272 @@ | |||
| openapi: 3.0.4 | |||
There was a problem hiding this comment.
we are currently using 3.0.3
| version: 0.0.1 | ||
|
|
||
| servers: | ||
| - url: '{apiRoot}/qa/v1' |
There was a problem hiding this comment.
must contain the API name
version part should be "vwip" for now, within release if would be v0.1alpha1
| license: | ||
| name: Apache 2.0 | ||
| url: https://www.apache.org/licenses/LICENSE-2.0.html | ||
| version: 0.0.1 |
There was a problem hiding this comment.
Should be wip for now
In release PR it will be 0.1.0-alpha.1 according to the PR title
| '400': | ||
| $ref: '#/components/responses/BadRequest' | ||
| '500': | ||
| $ref: '#/components/responses/ServerError' |
There was a problem hiding this comment.
| Error: | ||
| type: object | ||
| properties: | ||
| code: | ||
| type: integer | ||
| description: HTTP status code | ||
| message: | ||
| type: string | ||
| description: Error message | ||
| example: | ||
| code: 400 | ||
| message: "Invalid document format. Supported formats: doc/pdf/txt/csv" |
There was a problem hiding this comment.
Please follow the schema defined in CAMARA API Design guide and in https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml
There was a problem hiding this comment.
Please use definitions from https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml where appropriate.
Add x-correlator to all operations (see https://github.com/camaraproject/Commonalities/blob/main/documentation/CAMARA-API-Design-Guide.md#x-correlator-header)
There was a problem hiding this comment.
Security schemata and scopes are missing, see https://github.com/camaraproject/Commonalities/blob/main/documentation/CAMARA-API-Design-Guide.md#x-correlator-header
|
Hi @hdamker, Thank you so much for your comments - we'll carefully review and improve these items. |
|
Hi @hdamker, thanks for your valuable suggestions! We've confirmed several issues in our files and have now completed updates based on your comments. |
@lihaojie-CMCC great progress! But it's difficult to review for me, as you haven't replied to my comments, so I don't know which topics you are considering as solved and which you don't plan to change for some reasons. And as the PR is already merged, I don't know if it is still worth the effort now (unaddressed points will be show up again in the mandatory release review). Two points which I have seen on first glance:
|
|
Hi @hdamker, thank you for your thorough review! We truly appreciate your help, and I apologize for having rushed the merge earlier. To address your feedback:
Additionally, could you kindly confirm if we've met the alpha release requirements? If any gaps remain, we'd appreciate guidance on further improvements. Truly appreciate your expertise and support! |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Prepare the alpha release r1.1:
Which issue(s) this PR fixes:
Special notes for reviewers: