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

Replaced swagger2markup maven plugin with openapi-generator #928

Merged
merged 9 commits into from
Sep 28, 2024

Conversation

ppatierno
Copy link
Member

The current build of the bridge is broken, more specifically the part related to the documentation generation.
This is because the Maven plugin we use, swagger2markup, has been abandoned (since years) and lately some of its dependencies were removed from JCenter (which seems to be also going to be dismissed by JFrog).
Several times we tried to replace it and now it's the right time.
This PR is an attempt to use the openapi-generator project to do so. This project provides several generators for clients and documentation. It's able to generate code and content starting from the OpenAPI specification.
The generator used in this PR is the asciidoc one in order to generate the documentation for our OpenAPI specification in asciidoc format without making any change in the documentation flow generation we have today (through the Makefile).

The output I have got for the official website is pretty similar to the current one with some caviets:

  • it seems openapi-generator doesn't have support for generating the examples in asciidoc starting from the examples we have in the OpenAPI specification. The only workaround they provide is by using "snippets". With snippets we can put adoc examples in specific folders and they are included in the final asciidoc file. The drawback of this approach is duplication: every time we change or add a new example to the OpenAPI spec, we need to copy the corresponding snippet into the adoc file.
  • today our OpenAPI spec v3 defines our "key" and "value" for some components (i.e. ConsumerRecord, ProducerRecord) by using "oneOf" because they can be array, JSON (object) or string. From a generator perspective this is something that can't be handled by the generator itself because on code and documentation side, the generator can't create meaningful objects for a language. The "oneOf" approach is instead used by the vertx-web-openapi to validate the content of the body from the HTTP request. The end result of this is that right now the field defined as "oneOf" are rendered with a custom "empty" type by the openapi-generator and they are not rendered at all by the current swagger2markup (yes! we didn't notice that but for example, if you look for ProducerRecord in the current doc, the fields key and value are completely missing).
  • the generated adoc file has some duplicated sections (I opened an issue on the openapi generator project to get insights The asciidoc generator generates duplicated documentation for the same operation id OpenAPITools/openapi-generator#19659) but I also noticed that the final rendering in HTML (through the asciidoctor) contains just one, so it's not a big issue.

In general, we should continue to investigate how to improve and fix the above "issues" but right now this is the only way I see to unlock the bridge build.

@ppatierno ppatierno added this to the 0.31.0 milestone Sep 24, 2024
@ppatierno
Copy link
Member Author

I also noticed that this happened to Keycloack project in the past and they fixed by pushing the missing artifact in a personal namespace dropping JCenter. To be honest I am not so keed to this approach keycloak/keycloak#5070

Fixed OpenAPI v3 spec for missing items def under array types

Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno ppatierno marked this pull request as ready for review September 24, 2024 12:00
@ppatierno ppatierno requested a review from a team September 24, 2024 12:01
documentation/book/api/snippet/GET/http-response.adoc Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
documentation/book/api/index.adoc Outdated Show resolved Hide resolved
documentation/book/api/index.adoc Show resolved Hide resolved
Signed-off-by: Paolo Patierno <[email protected]>
@scholzj scholzj requested review from scholzj and PaulRMellor and removed request for scholzj September 24, 2024 13:21
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Ok, I guess I do not have a strong opinion on this.

Signed-off-by: Paolo Patierno <[email protected]>
producer records on OpenAPI v2

Signed-off-by: Paolo Patierno <[email protected]>
producer record definitions

Signed-off-by: Paolo Patierno <[email protected]>
Signed-off-by: Paolo Patierno <[email protected]>
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Good to find a solution to this ongoing issue. The content maps to what we have currently. If we want to maintain a single bridge doc including the API content, considering the API is stable so we don't need to change it much, this works fine.

Otherwise, we could consider spinning off the API content and hosting HTML output that we link to from the main bridge docs. Or it is possible to include HTML content in an asciidoc book (but that might introduce other issues).

@ppatierno ppatierno merged commit d999a35 into strimzi:main Sep 28, 2024
13 checks passed
@ppatierno ppatierno deleted the fix-markup-doc-generation branch September 28, 2024 07:12
ilkerkocatepe pushed a commit to ilkerkocatepe/strimzi-kafka-bridge that referenced this pull request Sep 28, 2024
)

* Replaced swagger2markup maven plugin with openapi-generator

Signed-off-by: Paolo Patierno <[email protected]>

* Fixed local path issue to the snippet dir

Signed-off-by: Paolo Patierno <[email protected]>

* Added OpenAPI validation during documentation build
Fixed OpenAPI v3 spec for missing items def under array types

Signed-off-by: Paolo Patierno <[email protected]>

* Fixed scholzj comments

Signed-off-by: Paolo Patierno <[email protected]>

* Removed JCenter repository

Signed-off-by: Paolo Patierno <[email protected]>

* Fixed key and value schemas to be consistent across consumer and producer
records

Signed-off-by: Paolo Patierno <[email protected]>

* Fixed key and value schemas to be consistent across consumer and
producer records on OpenAPI v2

Signed-off-by: Paolo Patierno <[email protected]>

* Extracted a common RecordKey and RecordValue components for consumer and
producer record definitions

Signed-off-by: Paolo Patierno <[email protected]>

* Fixed openapi related test

Signed-off-by: Paolo Patierno <[email protected]>

---------

Signed-off-by: Paolo Patierno <[email protected]>
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.

3 participants