Fix java version matching - #1714
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Java feature’s SDKMAN version resolution logic to correctly handle SDKMAN identifiers that include + build metadata (e.g. 21.0.12+1-ms) and simplifies parsing by switching from sdk list table parsing to the /versions/all endpoint.
Changes:
- Update
find_version_listto fetch available versions fromhttps://api.sdkman.io/.../versions/alland adjust regex matching to allow+...metadata for Java distributions. - Refactor
sdk_install/find_version_listcall signatures to remove the oldprefix/suffixparsing approach. - Bump the feature version to
1.8.2.
Show a summary per file
| File | Description |
|---|---|
src/java/install.sh |
Switches version discovery to /versions/all and updates matching logic to handle + build metadata; refactors install helper signatures. |
src/java/devcontainer-feature.json |
Bumps feature version to 1.8.2. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
src/java/install.sh:286
- When selecting a partial match from
version_list, the code builds agrep -Epattern from user input but only escapes dots. With the newly-supported+versions (e.g. users specifying21.0.12+1),+(and other regex metacharacters) will be interpreted by grep and can cause wrong matches or no match.
find_version_list "$install_type" version_list "${requested_version}"
if [ "${requested_version}" = "latest" ] || [ "${requested_version}" = "current" ]; then
requested_version="$(echo "${version_list}" | head -n 1)"
else
set +e
src/java/install.sh:279
- The
ltsbranch pickshead -n 1fromversion_listbut never validates it. If the version list fetch/parsing fails and returns empty,sdk installis invoked with an empty version and silently installs SDKMAN’s default candidate (the failure mode described in #1712).
elif [ "${requested_version}" = "lts" ]; then
find_version_list "$install_type" version_list "${requested_version}"
requested_version="$(echo "${version_list}" | head -n 1)"
elif echo "${requested_version}" | grep -oE "${full_version_check}" > /dev/null 2>&1; then
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Kaniska (Kaniska244)
left a comment
There was a problem hiding this comment.
Hi David Shen (@pantherman594),
Thank you for the contribution. Would you please accept the license agreement and take a look at the review comments.
- Escape `+` in requested_version regex substitutions so SDKMAN identifiers with build metadata (e.g. `21.0.12+1-ms`) match correctly. - Validate that the LTS branch resolves to a non-empty version before invoking `sdk install`, so a failed version lookup surfaces an error instead of silently installing SDKMAN's default candidate. - Add integration test scenarios covering major-only version resolution for the `ms` and `tem` JDK distributions.
4c8938e to
745f523
Compare
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
LGTM. To be further reviewed by maintainers.
One small observation, although not an absolute necessity, it could have been great to have couple of extra test cases such as below:
"install_additional_java_versions": {
"image": "ubuntu:noble",
"features": {
"java": {
"version": "25",
"additionalVersions": "21"
}
}
},
"install_java_8_ms_distro_debian": {
"image": "mcr.microsoft.com/devcontainers/base:debian",
"features": {
"java": {
"version": "8",
"jdkDistro": "ms"
}
}
}
install_additional_java_versions.sh script:
#!/bin/bash
set -e
# Optional: Import test library
source dev-container-features-test-lib
check "java version openjdk 25 installed as default" grep "openjdk 25\." <(java --version)
check "java version 21 installed as additional version" grep "^21\." <(ls /usr/local/sdkman/candidates/java)
# Report result
reportResults
install_java_8_ms_distro_debian.sh script:
#!/bin/bash
set -e
# Optional: Import test library
source dev-container-features-test-lib
check "java version openjdk 8 installed" /bin/bash -c 'java -version 2>&1 | grep "openjdk version \"1.8\."'
# Report result
reportResults
Added configurations for installing additional Java versions and Java 8 MS distribution on Debian.
82f4079
27febe2
into
devcontainers:main
Fixes finding versions that include a
+1. Also uses the/versions/allendpoint, which returns a comma-delimited list of all available versions, to simplify the version parsing.Closes #1712