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 to jdk23 #294

Merged
merged 1 commit into from
Nov 15, 2024
Merged

update to jdk23 #294

merged 1 commit into from
Nov 15, 2024

Conversation

vilmosnagy
Copy link
Contributor

Closes #238

@Vacxe
Copy link
Member

Vacxe commented Nov 15, 2024

@vilmosnagy thanks for the contribution mate, however this update will require a bit deeper changes, including using the java version in code and build pipelines, not only docker base image

@vilmosnagy
Copy link
Contributor Author

@Vacxe yeah, let met create a local build env, and try it out for myself :)

@vilmosnagy
Copy link
Contributor Author

@Vacxe with this change my main goals seems to work perfectly (I've build & tested the newly built docker image):

  • the image builds without a problem
  • I'm able to run the following Dangerfile.df.kts:
@file:Repository("https://artifacts.company.tld/repository/company/") // internal artifact registry 
@file:DependsOn("tld.company.tools:danger-plugin:1.0-210620.g6516d70-SNAPSHOT") // built with JDK23

import tld.company.tools.dangerplugin.DangerInfraPlugin

import systems.danger.kotlin.*

register plugin DangerInfraPlugin

danger(args) {
   DangerInfraPlugin.runChecks()
}

@Vacxe
Copy link
Member

Vacxe commented Nov 15, 2024

@vilmosnagy as well I can't see any reason for that upgrade. It mostly about base compatibility. Technically you can use any java version (as I remember for the latest kotlin it should be 17+) but it also has a compatibility option for java 8, which is the base for the docker.

Here is only one sense for that upgrade - if you use your specific docker image that is based on current Danger for grouping tasks. But you also can do it by installing Danger in your image (like it implemented in the Dockerfile)

@Vacxe
Copy link
Member

Vacxe commented Nov 15, 2024

@vilmosnagy I am just trying to understand what exactly the issue you are trying to fix with that update or is it just a "version bump"?

@vilmosnagy
Copy link
Contributor Author

vilmosnagy commented Nov 15, 2024

@vilmosnagy as well I can't see any reason for that upgrade.

Java8 is unsupported for years (more than 2 years), and most of the libs (which I'd like to use in my own plugin) tends to require newer JDK versions 'cause they want to use newer JDK features as well.

On the other hand, there should be no problem with upgrading the runtime: most of the code (with a few exceptions like spring, or javaee) which was written & compiled & targeted for JDK version 8 runs fine on any newer JDKs. The other way not so much - code & libs which was targeted for jdk11 cannot run on jdk8.

@Vacxe
Copy link
Member

Vacxe commented Nov 15, 2024

@vilmosnagy I'll try to update CI as well and prepare a new release

@vilmosnagy
Copy link
Contributor Author

@Vacxe thanks :)

But: I don't think you need to upgrade anything on the CI yet. I built this docker image in our company-wide docker repo, and tried in in our existing CI pipelines -> everything works perfectly fine for me with only this one change, and I'm able to run:

  • the currently existing danger-kotlin executable & danger-kotlin.jar
  • our plugin built with & targeted for jdk23

@Vacxe Vacxe merged commit a230d57 into danger:master Nov 15, 2024
3 checks passed
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.

Upgrade to newer JDK
2 participants