-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix #6490 add mybatis extension #10671
Conversation
extensions/mybatis/runtime/src/main/resources/META-INF/quarkus-extension.yaml
Show resolved
Hide resolved
extensions/mybatis/runtime/src/main/java/io/quarkus/mybatis/runtime/MyBatisRuntimeConfig.java
Outdated
Show resolved
Hide resolved
extensions/mybatis/runtime/src/main/java/io/quarkus/mybatis/runtime/MyBatisRuntimeConfig.java
Outdated
Show resolved
Hide resolved
for (String mapper : mappers) { | ||
try { | ||
configuration.addMapper(Resources.classForName(mapper)); | ||
} catch (ClassNotFoundException e) { |
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.
Maybe log.debug
here?
...s/mybatis/deployment/src/main/java/io/quarkus/mybatis/deployment/MyBatisMapperBuildItem.java
Outdated
Show resolved
Hide resolved
extensions/mybatis/runtime/src/main/java/io/quarkus/mybatis/runtime/MyBatisRuntimeConfig.java
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.
It would be nice to have integration tests (including native generation) too
e1613c6
to
b57fb1d
Compare
Thanks @gastaldi - I just made the changes as your suggestions and add the integration tests for the mybatis. The JVM test works fine but the native test looks weird. The native image build is failing with the following messges
I tried to add |
b57fb1d
to
6f4034a
Compare
Hi @gastaldi , Finally I got the native integration tests working but it has to add the |
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.
Found some minor typos in the doc
@gastaldi can you take a review of these changes ? |
I think this is cool. Can you start a conversation on https://groups.google.com/forum/#!forum/quarkus-dev about this extension before we merge this PR? Maybe someone could help on figuring out how to make the native build smoother for this extension |
Thanks @gastaldi - I will start a thread on the quarkus forum. |
Even if it happens to be true, btw, the decision should not depend on popularity in any specific country. As to the home for the extension, in my view, it's a question of maintainability. @zhfeng could you clarify your intentions wrt this work? Are you planning to be actively maintaining it? |
I wrote a article to introduce Quarkus in Chinese two months ago and got some feedback from the developers that they want to see the MyBatis support in Quarkus which could be very helpful and attract more people to have a try. @aloubyansky yeah, I'm planning to maintain this extension. The next work could be
|
629b84a
to
2656ef1
Compare
@gastaldi I just update to fix the conflict and test with Also I found the mybatis/mybatis-3#1552 which are looking for the Graal support in MyBatis. So there will be chances we can work together with the Mybatis community to support the GraalVM. In the perspective of the Quarkus, is there anything I can do before merging this PR ? |
@zhfeng thanks for the update, you've done an excellent job in this. Meanwhile if you are interested, it would be really cool if this extension ended up in https://github.com/quarkiverse instead. WDYT @aloubyansky ? |
yeah, I'm working to get rid of |
Hi @gastaldi , I still got some analyist parse erros during the native image builidng. And sometimes, these errors were very different in every building. Most of these are related to the |
@zhfeng hey! My knowledge on writing substitution classes is based on trial and error, so I am not sure I can give some hints in that regard :) |
Any update on this? |
Hi @dhaalves! We strongly recommend our community extensions to reside in the https://github.com/quarkiverse organization. This means free repository hosting (including build, CI and release publishing setup) for Quarkus extension projects contributed by the community. Plus it should integrate with our Quarkus Registry - making it visible in code.quarkus.io and related tooling (we're still working to make this happen automatically). @zhfeng are you interested in moving this code to a More details here: https://github.com/quarkiverse/quarkiverse/wiki |
Thanks @gastaldi - I'm happy to move to quarkiverse. |
@zhfeng fantastic! I'l create the repository tomorrow morning and give you the necessary push permissions. Ping me in Zulip if you have any questions. |
@zhfeng Repository created in https://github.com/quarkiverse/quarkiverse-mybatis. You should have received an invitation to join the organization (You should have the Maintainer role there). Closing this PR now |
No description provided.