-
Notifications
You must be signed in to change notification settings - Fork 174
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
Convert Load to Quarkus App #494
Conversation
@agoncal @holly-cummins questions here |
64c53a7
to
f499a0f
Compare
Oh noo, this got opened, and then ignored! I'm totally unable to answer these questions, but @geoand may be able to. |
You mean you can't stop the running application?
correct |
@holly-cummins I'm goign to retake this :) |
46bf7b7
to
4e74422
Compare
@geoand @holly-cummins I rebased the code and pushed some changes |
004f491
to
c5e65ef
Compare
@geoand I removed the native build for the cli. After playing around and testing many things, I think https://github.com/datafaker-net/datafaker is not graalvm compatible now, and to be used inside a native quarkus app, I feel it needs it's own extension |
😭 Deploy PR Preview failed. |
Interesting! |
LGTM, @holly-cummins WDYT? |
quarkus-workshop-super-heroes/docs/src/docs/asciidoc/optional-cli/cli.adoc
Outdated
Show resolved
Hide resolved
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.
LGTM, except for my comment about the project name. If we stick with the original name the diff might be smaller/more readable, as a bonus.
Do we want to add an example of how to build and invoke the new cli? (Or is that the same as before and it's already in the docs?)
quarkus-workshop-super-heroes/super-heroes/cli-super-heroes/pom.xml
Outdated
Show resolved
Hide resolved
I added it in the docs |
@holly-cummins @geoand I reported the changes |
+1 from me |
Looks good to me. |
Thanks, Katia! |
I have done in this PR:
Things to discuss here:
Closes #100