-
Notifications
You must be signed in to change notification settings - Fork 35
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 support for Hibernate 6 #348
Conversation
@javax.persistence.GeneratedValue(generator = "increment") | ||
@jakarta.persistence.GeneratedValue(generator = "increment") |
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.
This is the way... to avoid duplication of the code for tests against Hibernate 5 and 6
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.
Shouldn't this PR update the following:
pom.xml
Outdated
<groupId>jakarta.persistence</groupId> | ||
<artifactId>jakarta.persistence-api</artifactId> | ||
<version>3.1.0</version> | ||
<scope>provided</scope> |
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.
If this is required to run the tests then the scope should be test
?
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.
Not sure why, but it's needed for compilation against Hibernate 6, otherwise we got:
hazelcast-hibernate/hazelcast-hibernate53/src/main/java/com/hazelcast/hibernate/AbstractHazelcastCacheRegionFactory.java:[47,17] cannot access jakarta.persistence.PersistenceException
[ERROR] class file for jakarta.persistence.PersistenceException not found
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.
With the latest changes this seems to work fine with test
scope.
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 fails with test
scope: mvn clean verify -Dhazelcast.version=5.1.3 -Dhibernate.core.version=6.1.3.Final
Yes :) , forgot to push it: a36cd04 |
6887ffd
to
a36cd04
Compare
- name: Publish Test Results | ||
uses: EnricoMi/[email protected] | ||
if: ${{ !cancelled() }} | ||
with: | ||
check_name: Test results | ||
files: '**/*-reports/TEST-*.xml' |
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.
Where is this visible?
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 will be visible as a PR comment and as a check: https://github.com/EnricoMi/publish-unit-test-result-action#publishing-test-results (after the merge of this PR)
hazelcast-hibernate53/pom.xml
Outdated
@@ -27,7 +27,6 @@ | |||
|
|||
<properties> | |||
<hibernate.core.version>5.3.7.Final</hibernate.core.version> |
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.
Do we need this property here? should we have just one in the root pom, and up-to-date?
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.
removed: 2f5599b
pom.xml
Outdated
<groupId>jakarta.persistence</groupId> | ||
<artifactId>jakarta.persistence-api</artifactId> | ||
<version>3.1.0</version> | ||
<scope>provided</scope> |
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.
With the latest changes this seems to work fine with test
scope.
817746f
to
f5be7b6
Compare
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.
Great work. Thanks!
Fixes: #194
Fixes: #239