-
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
Add an Hibernate ORM codestart #20879
Conversation
...quarkus/extension-codestarts/hibernate-orm-codestart/base/src.main.resources/application.yml
Outdated
Show resolved
Hide resolved
a811d16
to
d353d27
Compare
@loicmathieu will the import.sql work with any type of db? |
I think so as it's standard SQL with no reserved word in it, I think. But maybe @Sanne knows better. |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building d353d27
Full information is available in the Build summary check run. Failures⚙️ Devtools Tests - JDK 11 #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-c104dc43-0be3-4b5c-981c-7b5ffaa9edde✖
⚙️ Devtools Tests - JDK 11 Windows #- Failing: integration-tests/devtools
📦 integration-tests/devtools✖
✖
✖
✖
✖
✖
📦 integration-tests/devtools/target/quarkus-codestart-build-test/project-maven-java-78cb4975-169b-417a-abb5-b36c36e4d717✖
⚙️ Gradle Tests - JDK 11 #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
|
@ia3andy the build fails because I didn't add any datasource to the quickstart test. Should I disable the |
I am not sure :-/ |
Indeed it looks like standard SQL but it won't work on DBs which don't support sequences. |
Well I didn't think about databases that didn't support sequences. |
MySql didn't support sequences so I'll remove the import.sql file and I create the items manually. |
@loicmathieu you don't have to delete it, just detect which db is selected using the list of extension, if you don't know, you keep it commented. wdyt? |
@ia3andy it may be difficult to list the db that support/didn't support sequences, and how would we treat 'other' ? And if we comment it the proposed unit test will not work. |
So keep the |
@ia3andy the test is the way to show how to use Hibernate ORM (inject and EM and work with it) so it should stay. |
@loicmathieu how will the test work without any data? |
@ia3andy the test is really to show how to use a JPA entity for someone how don't know, most of the time it will be deleted and replaced by a REST endpoint, a reactive messaging method or a main. In the test I will create three entity, persist them and retrieve them. Not a good unit test but a way to show how to use Hibernate ORM. |
TBH I would not keep the test if it's meant to be deleted.. the initial code is not here to teach or show anything (the guide and quickstart are here for this). It's meant for people who already know how to use it and like having a head start. |
d353d27
to
a4e0a29
Compare
So there is no point of having anything for Hibernate ORM as it will always be deleted. I updated my proposal with an import.sql with everything commented and a test that show the usage. |
@loicmathieu failure seems unrelated.. |
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.
Thanks @loicmathieu.. I am not sure if we should iterate on this PR with new commits for panache et reactive or create new PRs. I believe we should be fine with multiple PRs.
I can provide a Kotlin version tomorrow so we can merge this one with it. |
@loicmathieu maybe it would be easier to:
WDYT? |
I don't think it's feasible to make it a generic one as the Panache version will be different, in fact all versions will be different as the javadoc contains the way to use it. |
by generic I meant that it works with the different cases by altering the content depending on the selection. I didn't mean we we should have a "generic" content which fits all cases. |
@ia3andy I still thinks this will be too much work for almost no benefits, each extension will share very little code so almost all lines will be templated. This would just add complexity |
@loicmathieu I think we can separate reactive from the others, but hibernate and hibernate with panache may be selected together in the list, right? The generic approach let us make sure we don't have both in the same project while in the end only panache is needed.. |
ex: |
@ia3andy well, yes, this was my plan, conditionaly add different body to the MyEntity file when Panache is selected. I tought you mean having a base file that is modified via template. |
ok cool :) |
9e88c4f
to
630bba8
Compare
@ia3andy I added the Kotlin version. Please make a final review before merging this one; I'll then create the others for Panache/reactive/mongo. |
630bba8
to
45ee91d
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 45ee91d
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #- Failing: integration-tests/gradle
📦 integration-tests/gradle✖
⚙️ Native Tests - Windows - hibernate-validator #- Failing: integration-tests/hibernate-validator
📦 integration-tests/hibernate-validator✖ |
* } | ||
*/ | ||
@Entity | ||
class MyKotlinEntity { |
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.
would it make sense to use a kotlin data
object?
@loicmathieu could you investigate the gradle dev mode test to find out it it's related to the codestart? Maybe you should rebase from main? |
@ia3andy as it's only on Windows, only on Gradle, not related to codestart I think this is safe to merge. |
@loicmathieu I would feel better if the CI is green, could you rebase from main (this error happened already before on this PR, so it should have been fixed on main in the meantime) |
45ee91d
to
4ead0fe
Compare
Failing Jobs - Building 4ead0fe
Failures⚙️ Native Tests - Windows - hibernate-validator #- Failing: integration-tests/hibernate-validator
📦 integration-tests/hibernate-validator✖ |
@ia3andy the last CI runs failed on native image test for Hiberate validator, I can see how it can be related to my PR. |
I agree, just we should find out why this test is failing (so it doesn't bother every PRs).. @gsmet any thoughts |
Ok it seems those tests are flaky: #20495 |
No description provided.