You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2020/10/17 21:30:35 UTC
[GitHub] [fineract] vidakovic opened a new pull request #1422: FINERACT-1210: Swagger TypeScript client
vidakovic opened a new pull request #1422:
URL: https://github.com/apache/fineract/pull/1422
## Description
I've chosen the Angular Typescript flavor, because I think it'll be one of the more common choices out there. But there are quite some options available for Typescript; see here (and search for "typescript"): https://openapi-generator.tech/docs/generators/
In terms of configuration I didn't do actually too much here. I suggest you build it yourself and look in fineract-client/build/generated/typescript for the result. A first look at it seems OK to me (e. g. dependencies in package.json seem reasonable and minimal for Angular).
## Checklist
- [x] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [x] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [x] Create/update unit or integration tests for verifying the changes made.
- [x] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [x] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [x] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-712473424
We could add a Jasmin test... just have to see how to put this here in a nice way (the module layout is Java... now we have to mixin NPM/Typescript)... I'll see if I can come up with a suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-713283690
@vorburger @vidakovic Interesting question re what we should do wrt tests if we provide multiple clients
I suppose there are three different reasons why we want to test:
1. Test the Fineract server side business logic. This is what our existing Integration Test base is focused on.
2. Test the API spec itself. We clearly don't have tests for this - and as @vorburger has found that some of the API specs don't currently generate valid client code, so the spec is broken.
3. Test a specific language implementation of the client generated from spec - i.e. does the TypeScript version work, does the Java version work etc
To avoid maintaining a huge amount of duplicate tests, I think it would make sense to try to cover as much of 1 through 2 as possible. So actually using the generated clients to exercise the business logic, instead of e.g. some custom code as currently in Integration Tests or directly exercising the tests against Spring Boot. That way we are both testing one of the generated clients, validity of the API spec, and the business logic all in one go. But then we still need to see how to cover 3.
My view is that to focus our efforts, majority of the tests should be written against one of the generated clients only (in my view Java as that will align with most of the contributors' skill base). For the other generated clients, we should agree a very basic test suite (e.g. login / trigger one API / check response) to confirm the client works, but not to build a full functional test suite for them.
Just my thoughts... happy to discuss.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-717809675
@ptuomola You're OK to merge this and proceed as discussed above?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-711150797
Alright then. Just one note: this creates a dependency on NodeJS and NPM for the builds. We can check if those tools are available on a dev's machine and disable NPM related builds (fineract-client Typescript and demo angular app).
Maybe it would make sense then to put the fineract-client's in separate modules they are not sharing much (other then configuration); and it would make it easier to add client specific tweaks (e. g. JDK8 support, Angular demo app) vs. stuffing everything in one module.
How about:
* fineract-client-jdk11-sync
* fineract-client-jdk8-rx
* fineract-client-typescript-angular
Just spit-balling...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-711105429
@vorburger I would say the only thing maybe missing in this context is to publish the NPM package. I am wondering if we can use Apache's Nexus for that. Nexus supports NPM packages (and Docker images for that matter); the question is only if we can use those services. If not we have to figure out how to publish to NPM. I think we can do this in a separate PR.
I agree that we should show how this code is supposed to be used. Not sure how much code needs to be shown to make the basics work; for the description in "client.md" I would limit it to the bare minimum and just put a reference to a sample implementation.
I don't think this repo here is the best place for a full sample implementation? I think this would fit better in a separate Git repository or as part of one of the more current Fineract web UI projects (not sure which one uses a more recent version of Angular).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-719237513
@vidakovic @vorburger Sounds good - let's take this one step at at time: I'll merge this one, but let's not create any more clients after this until we have a) agreed what basic tests we should have in place to test the clients actually work and b) have implemented those tests for this client. Does that sound OK?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-712468014
@vidakovic once #1445 is merged, do you want to add a similar section re. the TS client to that same doc as part of this PR here?
@ptuomola what's your view pro/con merging this as-is without tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-711105429
@vorburger I would say the only thing maybe missing in this context is to publish the NPM package. I am wondering if we can use Apache's Nexus for that. Nexus supports NPM packages (and Docker images for that matter); the question is only if we can use those services. If not we have to figure out how to publish to NPM. I think we can do this in a separate PR.
I agree that we should show how this code is supposed to be used. Not sure how much code needs to be shown to make the basics work; for the description in "client.md" I would limit it to the bare minimum and just put a reference to a sample implementation.
I don't think this repo here is the best place for a full sample implementation. I think this would fit better in a separate Git repository or as part of one of the more current Fineract web UI projects (not sure which one uses a more recent version of Angular).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-717139983
@vorburger I would merge as is, can't really hurt. And maybe it motivates someone to contribute a little sample to illustrate how this is supposed to be used... or I find some time later to do that. And when we improve the Swagger yaml file (aka duplicate function names etc.) then this client will automatically benefit of that work too.
As for Cucumber: agreed, that's a whole different beast.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-713345111
@ptuomola agree... I think @vorburger started already something using fineract-client... if I'm not mistaken it was a login test? Not sure right now where I saw this. We could start "translating" the existing ITs one by one into the new "integration-test" module (I think we wanted to move things there).
Concerning 3.: we could specify the tests with Cucumber... that way we can make sure to cover the same with both Java and Typescript. It doesn't save anything with implementing the test functions themselves, but it's a nice way to keep track... and the Cucumber specs can be easily transformed into documentation (can't hurt).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola merged pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
ptuomola merged pull request #1422:
URL: https://github.com/apache/fineract/pull/1422
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-711172639
> put the fineract-client's in separate modules they are not sharing much (other then configuration); and it would make it easier to add client specific tweaks (e. g. JDK8 support, Angular demo app) vs. stuffing everything in one module.
SGTM. If I were you, I would pause this, and first do FINERACT-1214 - for the first 2 of the 3 outlined above.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vorburger commented on pull request #1422: FINERACT-1210: Swagger TypeScript client
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #1422:
URL: https://github.com/apache/fineract/pull/1422#issuecomment-711146250
I think it's initially MORE important to illustrate how generated code can be used than publishing it.
In the _monorepo_ philosophy, a simple running example within this repo seems more useful to me. Especially if we may still _refine_ (change) annotations, and thus will still "break" the generated clients, for a while. In an ideal world, such changes would go along with fixing the example code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org