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

New openapi-cli subgenerator for OpenApi client generation #9623

Merged
merged 22 commits into from Sep 5, 2019
Merged

New openapi-cli subgenerator for OpenApi client generation #9623

merged 22 commits into from Sep 5, 2019

Conversation

ecostanzi
Copy link
Contributor

@ecostanzi ecostanzi commented Apr 26, 2019

Create a sub-generator for generating clients from OpenAPI specs. Can be invoked using jhipster openapi-client. Open discussion in issue #9548.

TODO:

Jhipster checks:

  • Travis tests are green
  • Tests are added where necessary
  • Documentation is added/updated where necessary
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

.vscode/launch.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@murdos murdos left a comment

Choose a reason for hiding this comment

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

I made a few comments here and there, I hope they're not irrelevant since I'm a bit lot on what we want to achieve exactly here.
I'm also worried to have yet another subgenerator in the main generator-jhipster to maintain. I think we should really foster the module/blueprint ecosystem, and have interesting but not essential feature available as optional module.

cli/commands.js Outdated Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
generators/openapi-cli/index.js Outdated Show resolved Hide resolved
@vwiencek
Copy link
Contributor

Hello, just 1 comment/question.

Implementation of REST based clients to allow inter-microservices communication (based on @FeignClient is very useful. I see to options to share DTOObjects between services:

  • option 1: have a shared library with all DTOObjects to make sure we keep them in sync ;
  • option 2: reimplement (or regenerate with openapi-cli) and have duplicated objects in different services.

I guess the good pattern is to keep microservices as isolated as possible, which means that we should prefer option 1 over option 2, right ?

@murdos
Copy link
Contributor

murdos commented May 18, 2019

I guess the good pattern is to keep microservices as isolated as possible, which means that we should prefer option 1 over option 2, right ?

FYI on my current project we're using a 3rd option: each microservice publish its swagger contract in Nexus, and other microservices that need to communicate with this microservice generate DTO objects at compile time from the published swagger contract.

@ecostanzi
Copy link
Contributor Author

FYI on my current project we're using a 3rd option: each microservice publish its swagger contract in Nexus

@murdos that's interesting. How do you generate the spec file? I have read somewhere that it can be done with an integration test invoking the springfox endpoints.

@murdos
Copy link
Contributor

murdos commented May 20, 2019

@murdos that's interesting. How do you generate the spec file? I have read somewhere that it can be done with an integration test invoking the springfox endpoints.

That's exactly how we generate it indeed. I tried with https://github.com/kongchen/swagger-maven-plugin at build time, but the generated swagger was not reliable.

@PierreBesson
Copy link
Contributor

@ecostanzi Do you need this merged and released before your talk at Jhipster conf ?

@ecostanzi
Copy link
Contributor Author

@PierreBesson if the code looks good to you it would be nice! Let's see if the test pass. If so I'll add some more tests and mark the PR as ready for review.

generators/client/templates/angular/package.json.ejs Outdated Show resolved Hide resolved
generators/client/templates/react/package.json.ejs Outdated Show resolved Hide resolved
generators/server/templates/package.json.ejs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@ecostanzi ecostanzi marked this pull request as ready for review June 21, 2019 08:36
@ecostanzi
Copy link
Contributor Author

If it's ok for you members of the core team I'd like to do a demo preview of this feature during the talk at the jhipster conf. Otherwise if you prefer to take your time and review it before advertising it no problem, just tell me, i have something else to show.

@PierreBesson
Copy link
Contributor

Or you can just show it from your branch

@ecostanzi
Copy link
Contributor Author

@PierreBesson yes that's what I meant no need to have it in the main branch

@DanielFran
Copy link
Member

@ecostanzi Can you fix conflicts and confirm when this PR can be reviewed? Thanks

@PierreBesson
Copy link
Contributor

Hi everyone, let's move this forward, update the dependencies it introduces and merge this if this OK for you. For me it's important as a convergence of the OpenApiGenerator and JHipster projects.

@ecostanzi
Copy link
Contributor Author

Hi, I've just rebased the branch and removed a couple of dependencies. It should be ok for now.

@DanielFran
Copy link
Member

Restarted failing test

@DanielFran
Copy link
Member

Many thanks @ecostanzi for the awesome work

@DanielFran DanielFran merged commit 1bc7b60 into jhipster:master Sep 5, 2019
@ecostanzi ecostanzi deleted the openapi-cli branch September 6, 2019 07:54
@pascalgrimaud pascalgrimaud added this to the 6.3.0 milestone Sep 10, 2019
@@ -164,6 +164,7 @@ limitations under the License.
<%_ if (protractorTests) { _%>
"webdriver-manager": "12.1.5",
<%_ } _%>
"@openapitools/openapi-generator-cli": "0.0.14-4.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

@ecostanzi,
It doesn't look appropriate to include generator sub-module dependency in the generated client application. Can't it be encapsulated in the module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Vishal, what do you mean by saying "encapsulated in the module"?

The documentations suggests:

It is recommended to install the package as development dependency, because normally you only need this dependency during the development process

https://www.npmjs.com/package/@openapitools/openapi-generator-cli#package-mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes for me there is no problem to add this as dev dependency. Yes it does add significantly to what you have to download. But as it is useful, I don't see the problem of it being downloaded from npm rather than maven...

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it should be added as generator-jhipster dependency (not under dev) and not in the client applications dev dependency. Can you check if that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll let you know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vishal423 I opened issue #10510 to discuss some changes. In that case I think that the dependency should remain in the package.json of the generated project.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ecostanzi, I will leave it to other members to decide. For me, I am planning to create a separate module that wouldn't contain it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants