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

Add server extension #482

Merged
merged 11 commits into from
Feb 20, 2024
Merged

Conversation

carlesarnal
Copy link
Contributor

Many thanks for submitting your Pull Request ❤️!

Generating server stubs from OpenAPI definitions. This PR adds support for generating server stubs from the OpenAPI definition.

@carlesarnal
Copy link
Contributor Author

@ricardozanini this is the extension wrapping the Apicurio codegen feature for generating server stubs. I'm opening this in draft so we can discuss what's the best structure since I believe the submitted one is not the best possible (maybe having two folders server/client like the server one I created?). Let me know what you think!

@ricardozanini
Copy link
Member

@carlesarnal I think we can have a client and server top-level modules. Will work better. My only concern is about the maven artifact ID. We should append -server to the new one and keep the client as it is to avoid compatibility issues. As we go, we can deprecate one in favor of a -client one.

@carlesarnal
Copy link
Contributor Author

@carlesarnal I think we can have a client and server top-level modules. Will work better. My only concern is about the maven artifact ID. We should append -server to the new one and keep the client as it is to avoid compatibility issues. As we go, we can deprecate one in favor of a -client one.

Sorry for the late reply, I've been off for a while. Ok, do you want me to go ahead and do the refactor (essentially moving everything into a client module and adding the trailing -server) or you prefer to do it?

Thanks!

@carlesarnal
Copy link
Contributor Author

@carlesarnal I think we can have a client and server top-level modules. Will work better. My only concern is about the maven artifact ID. We should append -server to the new one and keep the client as it is to avoid compatibility issues. As we go, we can deprecate one in favor of a -client one.

Sorry for the late reply, I've been off for a while. Ok, do you want me to go ahead and do the refactor (essentially moving everything into a client module and adding the trailing -server) or you prefer to do it?

Thanks!

@ricardozanini I have rebased main on this PR and created a separate client directory for the existing extension. Let me know if this is what you had in mind. Thanks!

@carlesarnal carlesarnal force-pushed the add-server-extension branch 3 times, most recently from 1b35f5f to 8353724 Compare October 10, 2023 08:56
Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Thanks @carlesarnal! Yeah, I believe we are on the right path.

pom.xml Outdated Show resolved Hide resolved
@carlesarnal carlesarnal marked this pull request as ready for review October 31, 2023 09:11
@carlesarnal carlesarnal force-pushed the add-server-extension branch 3 times, most recently from f7b5410 to 1a0cf2e Compare October 31, 2023 09:33
@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023

😭 Deploy PR Preview failed.

@carlesarnal
Copy link
Contributor Author

@ricardozanini CI is green and I have added the client pom. I think this is ready to go, let me know what you think. Thanks!

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Looks great! I'm just missing the docs, otherwise I think we are good to go.

client/docs/pom.xml Show resolved Hide resolved
Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

I wonder if the client directory will get the changes from the main branch. If that's not the case, we will lose the most recent changes when we merge this PR.

Edit: Yes, it seems it gets the most recent commits when we pull main into this one.
3513a7d#diff-d54746b4518200dc549c201682ea7f5441abfe3aa9f26cb6768995f6dd2d83ab

Also, we need to backport this to the quarkus2 branch, otherwise, it will become a nightmare backporting other changes to quarkus2. I believe this backport will not be as smooth as usual, so I think rather than backporting it after the merge, as we usually do, we should have two simultaneous PRs and merge both only when all tests are passing.

@ricardozanini
Copy link
Member

@hbelmiro or we can wait to merge this one until we drop quarkus 2, which will be soon, likely in 3 weeks maybe. wdyt? @carlesarnal cc

@carlesarnal
Copy link
Contributor Author

@hbelmiro or we can wait to merge this one until we drop quarkus 2, which will be soon, likely in 3 weeks maybe. wdyt? @carlesarnal cc

I think that probably makes more sense than having to backport this to quarkus2.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fjtirado fjtirado left a comment

Choose a reason for hiding this comment

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

Lets keep the existing client pom artifacts ids existing names to avoid backward compatibiity issues.
Everything else looks superb. Fantastic work, congrats.

@carlesarnal
Copy link
Contributor Author

I have updated this PR from the changes from main so being outdated does not prevent it from being merged. If anything else is needed just let me know.

Co-authored-by: Matheus Cruz <56329339+mcruzdev@users.noreply.github.com>
@carlesarnal
Copy link
Contributor Author

carlesarnal commented Feb 20, 2024

Sorry, I was on PTO. @ricardozanini rebase done and issue created. Let me know if anything else is needed.

@ricardozanini ricardozanini merged commit f61a588 into quarkiverse:main Feb 20, 2024
11 checks passed
@carlesarnal carlesarnal deleted the add-server-extension branch February 20, 2024 16:32
@ricardozanini
Copy link
Member

@allcontributors please add @carlesarnal for maintenance

Copy link
Contributor

@ricardozanini

I've put up a pull request to add @carlesarnal! 🎉

@markuszoeller
Copy link

markuszoeller commented Feb 21, 2024

@hbelmiro @mcruzdev I'd like to give this a try as a user. What's the next planned release? Am I right in the assumption that I would see an entry called quarkus-openapi-generator-server at https://mvnrepository.com/artifact/io.quarkiverse.openapi.generator when this is released?

I'm building with gradle and tried this a few minutes ago:

// file: build.gradle

dependencies {
  // [...]
  implementation "io.quarkiverse.openapi.generator:quarkus-openapi-generator-server:3.0.0-SNAPSHOT"
}

But the build failed with this:

$ ./gradlew clean build

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':compileJava'.
> Could not resolve all dependencies for configuration ':quarkusDependency'.
   > Could not find io.quarkiverse.openapi.generator:quarkus-openapi-generator-server:3.0.0-SNAPSHOT.
     Searched in the following locations:
       - https://repo.maven.apache.org/maven2/io/quarkiverse/openapi/generator/quarkus-openapi-generator-server/3.0.0-SNAPSHOT/maven-metadata.xml
       - https://repo.maven.apache.org/maven2/io/quarkiverse/openapi/generator/quarkus-openapi-generator-server/3.0.0-SNAPSHOT/quarkus-openapi-generator-server-3.0.0-SNAPSHOT.pom
       - file:/Users/markus/.m2/repository/io/quarkiverse/openapi/generator/quarkus-openapi-generator-server/3.0.0-SNAPSHOT/maven-metadata.xml
       - file:/Users/markus/.m2/repository/io/quarkiverse/openapi/generator/quarkus-openapi-generator-server/3.0.0-SNAPSHOT/quarkus-openapi-generator-server-3.0.0-SNAPSHOT.pom
[...]

@ricardozanini
Copy link
Member

@carlesarnal can you give us a hand?

@ricardozanini
Copy link
Member

@markuszoeller many thanks for being the guinea pig. :)

So if we have a trusted stable codebase for the server plugin, I can release a major version.

@carlesarnal
Copy link
Contributor Author

@carlesarnal can you give us a hand?

I'll look at this ASAP.

@carlesarnal
Copy link
Contributor Author

@carlesarnal can you give us a hand?

I'll look at this ASAP.

@markuszoeller Until there is a new release making the artifact available in maven central as you described, you need the artifact to be present in your local repository. If you add the dependency straight to a build.gradle it will fail but then if you build the project locally (just a mvn clean install) it adds the artifact to your local repository. After this step, it builds the project with no issues (just tried it with an example gradle project).

@markuszoeller
Copy link

markuszoeller commented Feb 26, 2024

@carlesarnal

Until there is a new release making the artifact available in maven central as you described, you need the artifact to be present in your local repository. If you add the dependency straight to a build.gradle it will fail but then if you build the project locally (just a mvn clean install) it adds the artifact to your local repository. After this step, it builds the project with no issues (just tried it with an example gradle project).

This worked for me, thank you! The generated code looks very good so far. We probably test this over the next weeks. If I notice something odd then I'm going to open a GH Issue.

@ygyg70
Copy link

ygyg70 commented Feb 28, 2024

The generated code included lines such as the one below, where "Schema-" is an extra, and therefore doesn't compile:
private List<Schemas-Tag> tag = new ArrayList<Schemas-Tag>();
Any ideas?

@carlesarnal
Copy link
Contributor Author

Hello @ygyg70 if you have a problem, please, open a new issue (this is the PR for introducing the support). Also, be sure to include some information to reproduce the problem.

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.

8 participants