refactor: extract BaseJRE to eliminate duplication across standard JRE providers#1288
Open
stokpop wants to merge 2 commits into
Open
refactor: extract BaseJRE to eliminate duplication across standard JRE providers#1288stokpop wants to merge 2 commits into
stokpop wants to merge 2 commits into
Conversation
…E providers Reduces ~1300 lines of near-identical code in 6 JRE files to a single BaseJRE struct (243 lines) plus thin per-JRE wrappers (~11 lines each). BaseJRE uses a template method pattern with injected config/function fields so implementers never need to override Supply/Finalize, eliminating the 'forgot to call base' risk from method-override approaches. Variation points: - dirPrefixes/dirExacts: drive findJavaHome directory matching per JRE - extraFinalizeOpts: hook for JRE-specific JAVA_OPTS (used by IBM JRE) - installErrNote: extra context in install error messages (GraalVM) Zing JRE is intentionally excluded — it has no memory calculator or jvmkill and has different Finalize behaviour. Fixes cloudfoundry#1264
DetermineJavaVersion previously returned 17 silently when the JRE release file was missing. This made it impossible to diagnose broken or incomplete JRE installations. Changes: - DetermineJavaVersion now returns an error for missing/unreadable release files - BaseJRE.Finalize() logs a WARNING when falling back to Java 17 - Tomcat.Supply() logs a WARNING when falling back to Java 17 - Updated tests to reflect the new error behaviour
Contributor
Author
|
Added a follow-up commit that improves observability when the JRE
This means a broken or incomplete JRE installation will now produce a visible warning in buildpack output rather than silently proceeding with Java 17 assumptions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1264
Summary
Reduces ~1300 lines of near-identical code in 6 JRE files to a single
BaseJREstruct (243 lines) plus thin per-JRE wrappers (~11 lines each).Design
BaseJREuses a template method pattern with injected config/function fields so implementers never need to overrideSupply/Finalize, eliminating the 'forgot to call base' risk from method-override approaches.Variation points:
dirPrefixes/dirExacts: drivefindJavaHomedirectory matching per JREextraFinalizeOpts: hook for JRE-specific JAVA_OPTS (used by IBM JRE:-Xtune:virtualized -Xshareclasses:none)installErrNote: extra context in install error messages (GraalVM:ensure repository_root is configured)ZingJREis intentionally excluded — it has no memory calculator or jvmkill and has genuinely differentFinalizebehaviour.Before / After
Tests
Added
standard_jres_test.gocovering all 5 non-OpenJDK standard JREs (OpenJDK already hadopenjdk_test.go). For each JRE:Name()returns the correct display stringDetect()triggers on the correct env var and not on othersVersion()/JavaHome()are empty before installationfindJavaHome(viaFinalize()) locates the JRE-specific subdirectory prefix correctlyAll existing and new tests pass.