-
Notifications
You must be signed in to change notification settings - Fork 76
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
THREESCALE-3927 Update Swagger to Open API 3.0 #3103
Conversation
Yes I will remove rswag completely now |
@jlledom Rswag is removed completely |
Didn't review/try properly, as I see there are quite a few change requests from @jlledom already.
I think the Also, I don't know what @3scale/system-ruby think, but I personally don't like spaces if filenames (even less as these files will eventually get loaded by the browser via URL), so maybe we could use this opportunity to rename the specification files and use hyphens instead (I'd prefer hyphens to underscores to be more URL-friendly). |
Yes, there's some code to show some specs in OpenAPI v2 format. That code should be cleaned up. There also a javascript library
Agree |
179ac0a
to
1135ede
Compare
sure |
@jlledom |
b10c046
to
02235af
Compare
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.
This is better 😎 👍
Restore custom.d.ts Fixes Fix tests
Serialize Service Mgmt API's report endpoint body correctly
@@ -100,7 +114,7 @@ export interface Response extends SwaggerUIResponse { | |||
text: string; | |||
} | |||
|
|||
const autocompleteOAS3 = async (response: SwaggerUIResponse, accountDataUrl: string, serviceEndpoint: string): Promise<Response> => { | |||
export const autocompleteOAS3 = async (response: SwaggerUIResponse, accountDataUrl: string, serviceEndpoint: string): Promise<Response> => { |
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.
It seems this is being exported only for tests. IMO this has became a private function now, so it shouldn't be exported. Tests shouldn't condition the code, they should adapt to the code. In this case, the tests to assert autocompleteOAS3
logic should be included as tests for the 'fetching OpenAPI spec' context of autocompleteRequestInterceptor
test, since that's the 'unit' being exported. What do you think?
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.
It seems this is being exported only for tests. IMO this has became a private function now, so it shouldn't be exported. Tests shouldn't condition the code, they should adapt to the code.
That is right, and I agree with you that ideally the tests should adapt to the code, and not the other way around. On the other hand, when we code, we generally think about making the code testable (splitting codes in smaller units, adding dependency injection mechanism to allow for easier mocking of the dependencies, etc.).
In this case, I would really prefer to test the function autocompleteOAS3
itself, and not the only "public" function autocompleteRequestInterceptor
.
I am not sure if there is another easy way to do this, without exporting the function (there are ways, but not too straightforward).
In this case, the tests to assert
autocompleteOAS3
logic should be included as tests for the 'fetching OpenAPI spec' context ofautocompleteRequestInterceptor
test, since that's the 'unit' being exported.
I am not sure I understand what you mean 🙃
@josemigallas what do you think?
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.
I meant basically move 'should inject servers to the spec'
and 'should autocomplete fields of OpenAPI spec with x-data-threescale-name property'
to line 100 after 'should update the response interceptor'
. But OK.
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.
After all the changes you made on |
I don't see it as a trivial task, but I do agree that this might be actually a good time to fix it, as we are changing the interface for all three related packs anyway. I'll give it a spin tomorrow. |
I'm wondering if @josemigallas will approve 😬 |
98ff758
to
8200ed9
Compare
What this PR does / why we need it:
Update our own docs(sourcetoswagger) to OAS 3.0(Open API 3.0)
Which issue(s) this PR fixes
Main Task: https://issues.redhat.com/browse/THREESCALE-3927
There is a subtask: https://issues.redhat.com/browse/THREESCALE-8904 which is NOT part of this PR.
Special notes for your reviewer:
Outstanding issues:
Serialization does not work for
POST /transactions.xml
in Service Management API, see details in THREESCALE-3927 Update Swagger to Open API 3.0 #3103 (comment)UPDATE: this is fixed in Serialize Service Mgmt API's report endpoint body correctly #3294
Auto-complete does not work for all cases:
I (@mayorova) think this is OK - we may outline this as a limitation in the release notes.
/p/admin/api_docs/account_data
endpoint is called multiple times - for every API, because it is part of the existing auto-complete logic, which is called for every SwaggerUI invocation. We may want to refactor this part, so that the account data is pulled just one time, and then reused by all of the APIs.I (@mayorova) think that we need to somehow fix issue 1, and leave 2 and 3 for later.
Also, cleaning up the code and JSON specs that become "dead" after this upgrade will be done in another PR.
TODO:
ThreeScaleApiDocs
JS moduledoc/active_docs
to follow snake case (e.g.service_management_api.json
?