-
Notifications
You must be signed in to change notification settings - Fork 37
Set explicitly min database version for older db images to fix OCP tests #2506
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
Conversation
run tests |
run arm tests |
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.
I left comments, I am not certain I am right, think about it and say....
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl); | ||
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl) | ||
// set DB version as we use older version than default version configured at the build time | ||
.withProperty("quarkus.datasource.db-version", "10.3"); |
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.
I don't see a reason why we shouldn't use either RHEL9-based 10.5 ( https://catalog.redhat.com/software/containers/rhel9/mariadb-105/61a6084dbfd4a5234d596220 ) or 10.11 version here ( https://catalog.redhat.com/software/containers/rhel9/mariadb-1011/657b0463efad7c69e591c29b ). We have a Hibernate compatibility module for testing quarkus.datasource.db-version
and I don't see any requirement for sticking to 10.3 in this specific 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.
I can't see OCP tests in compatibility module, let's keep the tests how they are written now. I agree that we should move to newer still supported database versions, RHEL9-based 10.5 is an option here
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.
I don't think that something can differ for OCP, at best it differs in a config source (using env maybe). I'd not care, but sure, let's keep what you prefer.
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl); | ||
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl) | ||
// set DB version as we use older version than default version configured at the build time | ||
.withProperty("quarkus.datasource.db-version", "10.3"); |
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.
Same comment as for the spring/spring-web-reactive/src/test/java/io/quarkus/ts/spring/web/reactive/bootstrap/OpenShiftSpringWebQuteReactiveIT.java
test.
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl); | ||
.withProperty("quarkus.datasource.reactive.url", database::getReactiveUrl) | ||
// set DB version as we use older version than default version configured at the build time | ||
.withProperty("quarkus.datasource.db-version", "10.3"); |
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.
Same comment as for the spring/spring-web-reactive/src/test/java/io/quarkus/ts/spring/web/reactive/bootstrap/OpenShiftSpringWebQuteReactiveIT.java
.
@@ -22,5 +22,5 @@ public class OpenShiftPostgresql12DatabaseIT extends AbstractSqlDatabaseIT { | |||
.withProperty("quarkus.datasource.password", database.getPassword()) | |||
.withProperty("quarkus.datasource.jdbc.url", database::getJdbcUrl) | |||
// set DB version as we use older version than default version configured at the build time | |||
.withProperty("quarkus.datasource.db-version", "10"); | |||
.withProperty("quarkus.datasource.db-version", "12"); |
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.
Same sort of comments for all changes of db-version
for PG - I think it makes total sense to test PG 12 in the compatibility module with .withProperty("quarkus.datasource.db-version", "12");
and we should definitely do that, but I don't think we need to stick to some old PG version elsewhere unless we have some reason. So https://docs.jboss.org/hibernate/orm/7.0/dialect/dialect.html says 13 is minimum, nothing stops us form using PostgreSQL 13 https://docs.jboss.org/hibernate/orm/7.0/dialect/dialect.html. As for whether we should use 15 or 16 here, I don't know. I'd probably keep it at minimal like 13 so that we test also older versions.
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.
PG has older supported version 13, so agree
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.
Too late, already merged it 😁
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.
You were too fast, we can fix it in the next round of mage versions alignment with upstream
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.
oki
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.
OK, I still think that we should move on and this is not a good idea, but have it your way.
Summary
OpenShift tests are failing as Red Hat MariaDB and PostgreSQL images are using older versions than defaults configured at the build time. This is expected failure as minimum database versions has changed after upgrade to Hibernate ORM 7.0
Also in Quarkus 3.25 migration guide: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.25#hibernate-reactive
Our MariaDB
10.3
vs default10.5
Our PostgreSQL
12
vs default13
Please select the relevant options.
run tests
phrase in comment)run arm tests
phrase in comment)Checklist: