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 Docker image to Java 17 #1689

Merged
merged 1 commit into from Jan 17, 2024
Merged

Conversation

heyLu
Copy link
Contributor

@heyLu heyLu commented Jan 10, 2024

This updates the Java version Kroki uses to Java 17.

For now this is a draft as I found it a bit difficult to test Kroki locally. (I have IPv6 disabled which some tests seem to require and at least one of the Vega tests fails locally because of some Node.js stuff not being in place as expected. And even with a hack to get Node.js working it seems to generate a different SVG on my machine.)

Fixes #1688 when merged.

@heyLu heyLu force-pushed the update-to-java17 branch 2 times, most recently from 24d4f89 to e8d78e6 Compare January 10, 2024 11:11
@heyLu heyLu marked this pull request as ready for review January 11, 2024 11:04
@ggrossetie
Copy link
Member

Maybe we should do an intermediate step where the minimum version is Java 11 but we use Java 17 as base image? That way we can give a heads-up in the next release for users running Kroki from the jar file. What do you think?

For now only the Docker image is using Java 17, so that people running
Java 11 have some time to update before everything gets updated to Java 17.
@heyLu heyLu changed the title Update to Java 17 Update Docker image to Java 17 Jan 15, 2024
@heyLu
Copy link
Contributor Author

heyLu commented Jan 15, 2024

Maybe we should do an intermediate step where the minimum version is Java 11 but we use Java 17 as base image? That way we can give a heads-up in the next release for users running Kroki from the jar file. What do you think?

@ggrossetie, sounds good, done! I changed the PR title to reflect this as well.

From my side this is good to merge now. I can't run the tests locally though¹ (neither on this branch nor on main), can you trigger the CI tests to run on this branch? On the previous runs it looked like only the "test-container" checks ran, not the other CI pipelines. (Or just merge if things look fine to you.)

¹ A few tests generate differing SVG output with slightly different floating point results. I suspect somehow my versions are not matching up, but I couldn't figure out what's different. I am running node 18.18.2, which seems to be what generates the differences in some tests. But as this happens on main as well I suspect it's just my setup that is different.

@ggrossetie
Copy link
Member

¹ A few tests generate differing SVG output with slightly different floating point results. I suspect somehow my versions are not matching up, but I couldn't figure out what's different. I am running node 18.18.2, which seems to be what generates the differences in some tests. But as this happens on main as well I suspect it's just my setup that is different.

It can be related to your operating system and/or fonts. I will run the tests on the CI.

@ggrossetie ggrossetie merged commit 81e47a7 into yuzutech:main Jan 17, 2024
1 check passed
@ggrossetie
Copy link
Member

Thanks!

@heyLu heyLu deleted the update-to-java17 branch January 18, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Java version to 17 or 21
2 participants