graalvm*: add CE JDK 17 and fix CE JDK 21 checkver#592
graalvm*: add CE JDK 17 and fix CE JDK 21 checkver#592B67687 wants to merge 2 commits intoScoopInstaller:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new GraalVM Community Edition JDK 17 Windows x64 package manifest and updates the GraalVM 21 JDK 21 manifest's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Scoop
participant GitHub as GitHub Releases API
participant Installer as PowerShell Installer
participant FS as Filesystem/Env
User->>Scoop: request install graalvm-ce-17jdk
Scoop->>GitHub: fetch release list (checkver / autoupdate)
GitHub-->>Scoop: return JSON with tag_name and asset URLs
Scoop->>Scoop: resolve version & download URL
Scoop->>Installer: run installer script with downloaded archive
Installer->>FS: extract to temp, move files to install dir
Installer->>FS: set `JAVA_HOME`/`GRAALVM_HOME`, add `bin` to PATH
Installer->>FS: remove temp files
Installer-->>Scoop: install complete
Scoop-->>User: installation finished
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedFull review triggered. |
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bucket/graalvm-ce-17jdk.json (1)
20-24: checkver config LGTM; optional regex tightening.Mirrors the pattern now used in
graalvm21-jdk21.json;jsonpath: $[*].tag_nameagainstreleases?per_page=100correctly yields tag strings for the regex filter.Optional nit (same as the sibling manifest):
jdk-(17[\d.]+)would match a tag likejdk-170.xif one ever appeared. Usingjdk-(17\.[\d.]+)is a touch stricter. Purely defensive — safe to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/graalvm-ce-17jdk.json` around lines 20 - 24, The checkver block's regex "jdk-(17[\\d.]+)" is permissive and could match unintended tags; update the regex in the checkver section to the stricter pattern "jdk-(17\\.[\\d.]+)" (i.e., modify the regex value in the checkver object) so it requires a dot after "17" and avoids matching things like "jdk-170.x".bucket/graalvm21-jdk21.json (1)
21-23: checkver switch to Releases API LGTM.Querying
/releases?per_page=100and extracting tag names viajsonpathcorrectly avoids the/releases/latestHTML endpoint that was pinned to the 25.x train, so the existingjdk-(21[\d.]+)regex can now pick up the latestjdk-21.xtag among the paged results. Current release cadence ongraalvm/graalvm-ce-buildskeeps the relevant 21.x tags well within the first 100 entries.Optional nit: the regex could be tightened to
jdk-(21\.[\d.]+)so it doesn't match a hypotheticaljdk-21X...tag (e.g.jdk-210.0.0). Not a real-world risk today, purely defensive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bucket/graalvm21-jdk21.json` around lines 21 - 23, Tighten the release-tag regex used for checkver: replace the current "regex" value jdk-(21[\d.]+) with a more defensive pattern that requires a dot after 21 (e.g. jdk-(21\.[\d.]+)); update the "regex" field in the JSON (the entry with keys "url", "jsonpath", "regex") so it only matches tags like jdk-21.x and not hypothetical jdk-210... tags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bucket/graalvm-ce-17jdk.json`:
- Around line 20-24: The checkver block's regex "jdk-(17[\\d.]+)" is permissive
and could match unintended tags; update the regex in the checkver section to the
stricter pattern "jdk-(17\\.[\\d.]+)" (i.e., modify the regex value in the
checkver object) so it requires a dot after "17" and avoids matching things like
"jdk-170.x".
In `@bucket/graalvm21-jdk21.json`:
- Around line 21-23: Tighten the release-tag regex used for checkver: replace
the current "regex" value jdk-(21[\d.]+) with a more defensive pattern that
requires a dot after 21 (e.g. jdk-(21\.[\d.]+)); update the "regex" field in the
JSON (the entry with keys "url", "jsonpath", "regex") so it only matches tags
like jdk-21.x and not hypothetical jdk-210... tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28b087af-1347-4a0b-ae88-be1e829ff9c8
📒 Files selected for processing (2)
bucket/graalvm-ce-17jdk.jsonbucket/graalvm21-jdk21.json
✅ Actions performedReview triggered.
|
|
This is the CE side of the long-running GraalVM request in #472. It adds graalvm-ce-17jdk using the naming discussed there, and fixes graalvm21-jdk21 checkver so it can still resolve the 21 line from the official GraalVM CE releases source. CI and CodeRabbit are clean now, so I thought I’d flag it in case it helps with review. |
1 participant
Relates to #472
Relates to #508
This handles the still-useful Community Edition part of #508 without reusing its stale manifest shape directly.
Changes:
graalvm-ce-17jdkfor GraalVM Community JDK 17.0.9, using the naming discussed in [Request] New Oracle GraalVM and GraalVM Community for JDK 17.0.7 and JDK 20.0.1 #472.graalvm21-jdk21manifest for Community JDK 21, but change itscheckversource away fromreleases/latestso it can still see the latest 21.x release instead of only the latest 25.x release.Design decisions:
graalvm-ce-17jdkuses the sameextract_toplus installer move pattern as the existing newer GraalVM manifests, so it does not depend on the internal extracted directory name.graalvm21-jdk21already covers the Community JDK 21 package requested in graalvm-ce-jdk: add version 17.0.9 and 21.0.2 #508, so this PR fixes its version detection rather than adding a duplicate manifest under another name.graalvm/graalvm-ce-buildsGitHub releases API, not a third-party source.Validation:
checkver.ps1 graalvm-ce-17jdk->17.0.9checkver.ps1 graalvm21-jdk21->21.0.2checkurls.ps1 graalvm-ce-17jdk->[1][1][0]checkurls.ps1 graalvm21-jdk21->[1][1][0]I have read the Contributing Guide.
Summary by CodeRabbit
New Features
Improvements