-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update Java settings and JDK requirement in Java containers #1448
Conversation
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.
LGTM! Also adding Brigit and Josh to the review since they're starting to drive dev containers (partly tied to plans related to containers.dev).
I guess one question is whether we should fully drop java-8 and just add that to the java
definition. Are there any differences anymore?
@bamurtaugh @joshspicer Removing JDK 11 from the JDK8 image makes sense and is great, but this would be a breaking change for existing java-8
devcontainer.json files. So I think we should probably do a major version bump in definition-manifest.json
for the release and tweak any related Dockerfiles to be java:1-<JDK version>
instead of java:0-<JDK version>
@joshspicer Note that the related dev container future here could be tweaked to no longer install JDK11 automatically if JDK8 is selected.
We can also tweak https://github.com/microsoft/vscode-remote-try-java
This is a good question. It makes sense to merge java-8 to the general java definition. Currently one problem is we're using Microsoft Build of OpenJDK for Java 11 and Java 17, but use Eclipse Tem for Java 8 because Microsoft doesn't provide Java 8 build. To install JDKs from different sources in one Dockerfile, we need to tweak the installation script a bit more. I'd take this as a follow-up of this PR. |
380491e
to
f0255d1
Compare
Thanks for the tag, @Chuxel, and for making this change, @testforstephen!
This makes sense - @testforstephen would you be able to make this change in this PR too?
I can open a PR there to update that one. |
@bamurtaugh done, I have bumped java-8 image to 1.0.0. |
Thanks @testforstephen! If there are no other thoughts or concerns @joshspicer @Chuxel, I'll merge this update. I also opened microsoft/vscode-remote-try-java#39 to update the Java try sample and tagged @testforstephen for review. |
LGTM too! |
Fixes #1444