-
Notifications
You must be signed in to change notification settings - Fork 5
Add Temurin JDK 24 #93
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
base: master
Are you sure you want to change the base?
Conversation
994b099
to
00ba2e2
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.
General question: should we have it named 24
or latest
? or both?
My point is we would have to update both CI (dd-trace-java and dd-trace-java-docker-build) every 6 months if we have a fixed name (ie 24) whereas we will only need to update the docker build if we name it latest.
Good point - I think it makes sense to name it |
4f2146f
to
44272b4
Compare
07a708e
to
926820f
Compare
926820f
to
cbc6f39
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.
Looking good!
My main question is about latest_stable
being compute at check time versus build time -- check my comment for context 😉
|
||
COPY --from=eclipse-temurin:8-jdk-jammy /opt/java/openjdk /usr/lib/jvm/8 | ||
COPY --from=eclipse-temurin:11-jdk-jammy /opt/java/openjdk /usr/lib/jvm/11 | ||
COPY --from=eclipse-temurin:17-jdk-jammy /opt/java/openjdk /usr/lib/jvm/17 | ||
COPY --from=eclipse-temurin:21-jdk-jammy /opt/java/openjdk /usr/lib/jvm/21 | ||
COPY --from=temurin-latest /opt/java/openjdk /usr/lib/jvm/${LATEST_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.
Dumb question: do you know why we have to set an indirection by creating a temurin-latest
stage and not use
COPY --from=eclipse-temurin:${LATEST_VERSION}-jdk-noble /opt/java/openjdk /usr/lib/jvm/${LATEST_VERSION}
instead?
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.
For some reason, --from
does not allow variable interpolation (docker docs, community forum discussion, open moby issue) :/
build
Outdated
@@ -161,6 +190,7 @@ function do_describe() { | |||
} | |||
|
|||
function do_inner_describe() { | |||
compute_latest_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.
The new LATEST_VERSION
will be computed with the actual time, and not the time used when the image was built. I think that might be an issue 🤔
What about using ENV
in Dockerfile
to capture the ARG
value?
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.
Ah I see what you mean. After the image is built, we should store the then-calculated LATEST_VERSION
into an ENV
variable in the Dockerfile. Subsequently, we should use that ENV
variable as the LATEST_VERSION
value so that we do not risk compute_latest_version
here returning a mis-matching value.
Done in 6078764!
build
Outdated
@@ -3,7 +3,7 @@ set -eu | |||
|
|||
readonly IMAGE_NAME="ghcr.io/datadog/dd-trace-java-docker-build" | |||
|
|||
readonly BASE_VARIANTS=(8 11 17 21) | |||
readonly BASE_VARIANTS=(8 11 17 21 latest_base) |
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.
Nitpick: it feels like latest_base
will be confusing and I guess you did not use latest
as it would conflict with the docker latest tag.
What about "stable" and "ea" (that we will introduce later for "early access" OpenJDK builds)?
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.
Good point - I changed the name to stable
. I couldn't find a clean way to copy in an image only when it exists, so I'm thinking ea
can be added after the early access build becomes available.
cee1539
to
6078764
Compare
Add Temurin JDK 24 to help with testing the latest version.