fix: three silent-failure bugs in verification, cache path, and JDK selection#1696
Closed
Fikri-20 wants to merge 6 commits intojenkins-infra:mainfrom
Closed
fix: three silent-failure bugs in verification, cache path, and JDK selection#1696Fikri-20 wants to merge 6 commits intojenkins-infra:mainfrom
Fikri-20 wants to merge 6 commits intojenkins-infra:mainfrom
Conversation
In dry-run mode, the tool was still writing modernization metadata files to disk and attempting to push them to the metadata repository. This conflicts with the expected non-destructive behavior of dry-run. Changes: - Add !config.isDryRun() check in the finally block of process() - Skip collectModernizationMetadata() and related operations in dry-run - Only plugin-metadata.json (needed for processing) is now created Before fix: - modernization-metadata.json was created in dry-run - metadata-plugin-modernizer/* files were created After fix: - modernization-metadata.json is NOT created in dry-run - No metadata repository operations in dry-run mode Bug: jenkins-infra#2 - Dry-run still writes metadata
verifyPlugin() called plugin.withoutErrors() twice — once inside the failure branch and once unconditionally — so build failures after recipe application were always silently discarded. The guard in start() that was meant to abort the run and skip the PR on broken builds never triggered. Remove both withoutErrors() calls so errors from verify() propagate back to start(), which already has the correct skip logic. Upgrade the log from INFO to WARN to match the severity.
…efault getDiffStats() resolved the plugin's .git directory from Settings.DEFAULT_CACHE_PATH regardless of the --cache-path option or CACHE_DIR environment variable. Any run with a custom cache path would fail with 'Failed to get diff stats' and report zero additions/deletions in the modernization metadata. Replace the hardcoded constant with config.getCachePath() so the correct cache location is always used.
The previous implementation had a precedence bug:
if (jdks == null || jdks.isEmpty() && jenkinsVersion == null)
Because && binds tighter than ||, this evaluated as:
if (jdks == null || (jdks.isEmpty() && jenkinsVersion == null))
As a result, whenever jdks was non-null and non-empty the method fell
through to a single-line return that completely ignored the jdks
argument. A plugin declaring e.g. [JAVA_17, JAVA_21] would get back
JAVA_11 for Jenkins 2.479.3, even though 11 is not in the declared set.
The fix rewrites the method to:
1. Fast-path on null jenkinsVersion (delegate to single-arg min).
2. Compute the version-range list once.
3. Intersect the caller-supplied jdks with that list and return the
minimum; fall back to the range minimum only when the intersection
is empty.
Also replaces the bare orElseThrow() with orElse(JDK.min()) to avoid
a cryptic NoSuchElementException when a future version boundary leaves
the supported list empty.
…ails In run mode, a verification failure after applying a recipe must prevent commits and PRs so broken code is never pushed. In dry-run mode the user is only previewing changes so a verification failure should be logged as a warning but must not suppress the diff output.
Collaborator
|
If this fixes 3 bug please open 3 PR. While I agree with 1 and 2 I would like to study more the 3rd Opening a PR with different contexts reduce the change it gets merged or reviewer |
Contributor
Author
|
yeah I see, breaking them into 3 separate PRs now .. |
Contributor
Author
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
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.
Three silent bugs I found while working on the tool. Each one causes the tool to proceed as if nothing
went wrong when it should have stopped or warned.
1. Verification failures were silently swallowed (PluginModernizer.verifyPlugin)
verifyPlugin()calledplugin.withoutErrors()unconditionally at the end, regardless of whether the build actually passed. The caller instart()would always see a clean plugin and continue to fork, commit, and open a PR — even when the verification build failed. Removed bothwithoutErrors()calls and upgraded the log from INFO to WARN.In dry-run mode, the error is still cleared after logging so the diff preview is still shown to the user (that's intentional — you're previewing, not committing).
2.
getDiffStatsalways read from the default cache path (GHService)getDiffStats()was resolving the .git directory against Settings.DEFAULT_CACHE_PATH instead ofconfig.getCachePath(). So running with--cache-path /tmp/my-cachewould clone the plugin correctly therebut then fail to read it back when building the PR description. One-line fix.
3.
JDK.min(Set, String)ignored thejdksargument (JDK)Operator precedence bug in the guard:
// was parsed as:
if
(jdks == null || (jdks.isEmpty() && jenkinsVersion == null))So a non-null, non-empty jdks fell straight through to
JDK.get(jenkinsVersion)with no filtering — jdks was dead code. A plugin that declares [17, 21] in its Jenkinsfile would get compiled with Java 11 against an older Jenkins version. Rewrote the method to intersect the declared JDKs with the version-supported set and added tests for the untested paths.Testing: all three reproduced locally before the fix, gone after. mvn test -pl plugin-modernizer-core
passes.