Skip to content
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

Update MariaDB image to 10.6 for integration tests and dev services #23329

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Update MariaDB image to 10.6 for integration tests and dev services #23329

merged 1 commit into from
Feb 1, 2022

Conversation

famod
Copy link
Member

@famod famod commented Jan 31, 2022

Came up here: #23249 (comment)

From a recent poll (ongoing): https://mariadb.org/poll/server-version-with-10-6/

poll

/cc @Sanne

I also came up with a way to keep the respective dev service aligned by using what was moved to build-parent not long ago.
If you guys like it I can adjust the other dev services in a separate PR.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE labels Jan 31, 2022
@@ -23,7 +23,7 @@ public static String guessDialect(String persistenceUnitName, String resolvedDbK
return "io.quarkus.hibernate.orm.runtime.dialect.QuarkusH2Dialect";
}
if (DatabaseKind.isMariaDB(resolvedDbKind)) {
return "org.hibernate.dialect.MariaDB103Dialect";
return "org.hibernate.dialect.MariaDB106Dialect";
Copy link
Member Author

@famod famod Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely sure about this one. But to quote the comment further up:

// For now select the latest dialect from the driver

If it's get's merged like this we should probably add something to the migration guide? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the change and the note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.parse(imageName.orElse("docker.io/" + MariaDBContainer.IMAGE + ":" + MariaDBDevServicesProcessor.TAG))
.asCompatibleSubstituteFor(DockerImageName.parse(MariaDBContainer.IMAGE)));
.parse(imageName.orElseGet(() -> ConfigureUtil.getDefaultImageNameFor("mariadb")))
.asCompatibleSubstituteFor(DockerImageName.parse(MariaDBContainer.NAME)));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: IMAGE is deprecated.


static {
var tccl = Thread.currentThread().getContextClassLoader();
try (InputStream in = tccl.getResourceAsStream("devservices.properties")) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about "external" dev services. To cover those as well here we could load all properties on the CP.
But then again this can be considered more of an "internal" feature for the beginning?

@@ -0,0 +1 @@
mariadb.image=${mariadb.image}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you would have one common file for all the images, right?

I thought of doing it with a file specific to each devservice but it's certainly easier this way and good enough IMHO. This could be generalized to all devservices next.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsmet Yeah, thought about multiple files too. I can change it to a specific one per dev service if you prefer.
This would make loading that specific file a bit more complicated though, but it's definitely doable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: This would also make #23329 (comment) obsolete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW:

This could be generalized to all devservices next.

I have started working on this.

@@ -23,7 +23,7 @@ public static String guessDialect(String persistenceUnitName, String resolvedDbK
return "io.quarkus.hibernate.orm.runtime.dialect.QuarkusH2Dialect";
}
if (DatabaseKind.isMariaDB(resolvedDbKind)) {
return "org.hibernate.dialect.MariaDB103Dialect";
return "org.hibernate.dialect.MariaDB106Dialect";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the change and the note.

@gsmet gsmet merged commit a63da1f into quarkusio:main Feb 1, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Feb 1, 2022
@famod famod deleted the mariadb-10.6 branch February 1, 2022 10:00
@Sanne
Copy link
Member

Sanne commented Feb 1, 2022

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/hibernate-orm Hibernate ORM area/persistence OBSOLETE, DO NOT USE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants