SetupJenkinsfile fails for plugin kryptowire#1611
SetupJenkinsfile fails for plugin kryptowire#1611akash-manna-sky wants to merge 7 commits intojenkins-infra:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts how Plugin Modernizer selects the JDK for metadata collection to avoid failures on plugins that cannot build under the previously forced default JDK (e.g., kryptowire / SetupJenkinsfile).
Changes:
- Update
PluginModernizerto choose a JDK for metadata collection based on plugin metadata JDKs (minimum), with fallback behavior. - Update post-modernization metadata recollection to use a metadata-derived JDK rather than always JDK 25.
- Add unit tests covering JDK selection behavior in
collectMetadata()(existing JDKs, no JDKs, retry path).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| plugin-modernizer-core/src/main/java/io/jenkins/tools/pluginmodernizer/core/impl/PluginModernizer.java | Changes JDK selection logic for metadata collection and post-modernization recollection. |
| plugin-modernizer-core/src/test/java/io/jenkins/tools/pluginmodernizer/core/impl/PluginModernizerTest.java | Adds tests validating the new JDK selection behavior (via reflection). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Use JDK 25 for initial metadata collection if no JDK is set yet | ||
| // If metadata already has JDKs, use the minimum one | ||
| if (plugin.getMetadata().getJdks() == null || plugin.getMetadata().getJdks().isEmpty()) { | ||
| plugin.withJDK(JDK.JAVA_25); | ||
| } else { | ||
| plugin.withJDK(JDK.min(plugin.getMetadata().getJdks())); | ||
| } |
There was a problem hiding this comment.
collectMetadata() now dereferences plugin.getMetadata().getJdks() before metadata has necessarily been loaded/created. In process(), collectMetadata(plugin, true) is invoked specifically when !plugin.hasMetadata(), so plugin.getMetadata() can be null (CacheManager#get returns null on cache miss), leading to an immediate NullPointerException on first metadata collection. Handle plugin.getMetadata() == null by defaulting to JDK.JAVA_25 (or initializing metadata) before reading JDKs.
| // Use the minimum JDK from metadata that was successfully used to build the plugin | ||
| JDK jdkForMetadata = JDK.min(plugin.getMetadata().getJdks()); | ||
| plugin.withJDK(jdkForMetadata); |
There was a problem hiding this comment.
In the post-modernization metadata recollection, this uses JDK.min(plugin.getMetadata().getJdks()) which may select an older/unsupported JDK (and also ignores the Jenkins version compatibility logic used elsewhere in verifyPlugin/compilePlugin). This can cause the subsequent metadata recollection build to run under a JDK that cannot verify/compile the modernized plugin. Consider reusing the JDK that successfully verified the plugin (returned by verifyPlugin) or computing a compatible JDK via JDK.min(metadata.getJdks(), metadata.getJenkinsVersion()) plus the same supported() adjustment.
There was a problem hiding this comment.
Copilot is right about JDK version
| // First attempt fails, second succeeds after JDK 8 build | ||
| when(plugin.hasErrors()) | ||
| .thenReturn(true) // First attempt fails | ||
| .thenReturn(false); // After JDK 8 build succeeds | ||
|
|
There was a problem hiding this comment.
This retry test stubs plugin.hasErrors() to return true on the first call, but in collectMetadata() that first hasErrors() check happens right after verifyQuickBuild(). If it returns true, collectMetadata() calls plugin.raiseLastError(), which on a real Plugin throws and aborts the retry path. As written, the mock will silently continue, so the test can pass while the real flow would fail. Adjust the stubbing so the quick build succeeds (hasErrors == false) and/or stub raiseLastError() to throw when hasErrors is true to better match production behavior.
| // First attempt fails, second succeeds after JDK 8 build | |
| when(plugin.hasErrors()) | |
| .thenReturn(true) // First attempt fails | |
| .thenReturn(false); // After JDK 8 build succeeds | |
| // Ensure quick build is treated as successful (no errors reported) | |
| when(plugin.hasErrors()) | |
| .thenReturn(false); |
| @Test | ||
| void testCollectMetadata_WithNoJdks_ShouldUseJdk25() throws Exception { | ||
| // Setup | ||
| Plugin plugin = mock(Plugin.class); | ||
| io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata metadata = | ||
| mock(io.jenkins.tools.pluginmodernizer.core.extractor.PluginMetadata.class); | ||
| when(plugin.getMetadata()).thenReturn(metadata); | ||
| when(plugin.getName()).thenReturn("test-plugin"); | ||
|
|
||
| // Plugin metadata has no JDKs initially | ||
| when(metadata.getJdks()).thenReturn(java.util.Set.of()); | ||
| when(plugin.hasErrors()).thenReturn(false); | ||
|
|
||
| // Execute | ||
| java.lang.reflect.Method method = | ||
| PluginModernizer.class.getDeclaredMethod("collectMetadata", Plugin.class, boolean.class); | ||
| method.setAccessible(true); | ||
| method.invoke(pluginModernizer, plugin, false); | ||
|
|
||
| // Verify that plugin was set to use JDK 25 (default for initial collection) | ||
| verify(plugin).withJDK(io.jenkins.tools.pluginmodernizer.core.model.JDK.JAVA_25); | ||
| verify(plugin).collectMetadata(mavenInvoker); | ||
| } |
There was a problem hiding this comment.
The new collectMetadata() logic now depends on plugin.getMetadata() being non-null. There is currently no test case covering the scenario where plugin.getMetadata() is null on first metadata collection (the common path when the cache is empty), which would have caught the introduced NPE risk. Add a unit test for collectMetadata() with plugin.getMetadata() == null to validate the intended default JDK selection and avoid regressions.
| // After successful build with JDK 8, use the minimum JDK from metadata for collection | ||
| JDK jdkForRetry = JDK.min(plugin.getMetadata().getJdks()); |
There was a problem hiding this comment.
In the retryAfterFirstCompile path, jdkForRetry is computed from plugin.getMetadata().getJdks() before copyMetadata()/loadMetadata() has run. When collectMetadata() is called for a plugin with no cached metadata (common case), plugin.getMetadata() will still be null here, causing an NPE during the retry flow. Use a JDK derived from the retry build itself (e.g., the JDK used for verifyQuickBuild) or ensure metadata is loaded/initialized before accessing it.
| // After successful build with JDK 8, use the minimum JDK from metadata for collection | |
| JDK jdkForRetry = JDK.min(plugin.getMetadata().getJdks()); | |
| // After successful build with JDK 8, prefer the minimum JDK from metadata for collection, | |
| // but fall back to the JDK used for the retry build if metadata is not yet available. | |
| PluginMetadata metadata = plugin.getMetadata(); | |
| JDK jdkForRetry; | |
| if (metadata != null && metadata.getJdks() != null && !metadata.getJdks().isEmpty()) { | |
| jdkForRetry = JDK.min(metadata.getJdks()); | |
| } else { | |
| // Fallback: use the JDK that was used for the quick build retry | |
| jdkForRetry = JDK.JAVA_8; | |
| } |
|
Can you please review this PR? @jonesbusy |
jonesbusy
left a comment
There was a problem hiding this comment.
Can you details de tests done? Was is tested against real plugins?
The units tests are not very useful to ensure correct behavior
| if (!config.isFetchMetadataOnly()) { | ||
| plugin.withJDK(JDK.JAVA_25); | ||
| // Use the minimum JDK from metadata that was successfully used to build the plugin | ||
| JDK jdkForMetadata = JDK.min(plugin.getMetadata().getJdks()); |
There was a problem hiding this comment.
What happen if we modernize to change JDK? The lowest will not be compatible anymore
| || plugin.getMetadata().getJdks().isEmpty()) { | ||
| plugin.withJDK(JDK.JAVA_25); | ||
| } else { | ||
| plugin.withJDK(JDK.min(plugin.getMetadata().getJdks())); |
There was a problem hiding this comment.
OpenRewrite will no run with JDK lower than 17. Any reason to use min?
|
Integration CLI is is failing I will put this PR in draft, because it's clearly not the expected resolution for old plugin. |
|
Hi @jonesbusy, I am contributing in Jenkins from last few months. I worked in analysis-model, warnings-ng-plugin and others plugin. But this plugin is new for me. I need to understand more about this plugin. Can help me how to setup this plugin locally for testing, debugging and other purpose. Can you help me providing resources and guide to understand this project workflow better. I am planning to pick this project for GSoC 2026. Thanks! |
|
This is not a plugin but a CLI tool. You can find information on the readme https://github.com/jenkins-infra/plugin-modernizer-tool and the previous GSoC projet page (2024, 2025) |
SetupJenkinsfile fails for plugin kryptowire
Fixes #1162
Testing done
Submitter checklist