Migrate code base to Java 21#26075
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
I really like this, but the amount of changes is quite high. Maybe split the PR into smaller pieces? You already have quite nice commits. If you would bundle them into smaller packages, it would be a lot easier to review. |
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the Jenkins codebase by applying Java 21 language features through automated IntelliJ migration recipes. The changes include adopting SequencedCollection methods, pattern matching for instanceof, switch expressions, lambda expressions, text blocks, and other Java 8-21 improvements.
Changes:
- Replaced
.get(0)and.get(size()-1)with.getFirst()and.getLast()from SequencedCollection API - Converted if-else chains to switch expressions with pattern matching
- Replaced anonymous classes with lambda expressions and method references
- Adopted pattern matching for instanceof to eliminate explicit casts
- Used text blocks for multi-line strings
- Applied
Objects.requireNonNull*methods for null handling
Reviewed changes
Copilot reviewed 194 out of 194 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| war/src/main/java/executable/Main.java | Applied Objects.requireNonNullElseGet for cookie name handling |
| Multiple test files | Converted .get(0) to .getFirst() for SequencedCollection |
| core/src/main/java/jenkins/security/ApiTokenProperty.java | Converted if-else to switch expression (with duplicate comment issue) |
| core/src/main/java/jenkins/model/Jenkins.java | Lambda conversion and Objects.requireNonNullElseGet usage (with unreachable code bug) |
| core/src/main/java/hudson/Functions.java | Objects.requireNonNullElseGet usage (with unreachable code bug) |
| Multiple CLI files | Text blocks and switch expression conversions |
| Multiple core files | Pattern matching, lambda conversions, and switch expressions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Resolve GitHub Copilot review comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 194 out of 194 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 194 out of 194 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MarkEWaite
left a comment
There was a problem hiding this comment.
I've reviewed it in detail and submitted several small changes that came from the review. GitHub Copilot has reviewed the changes as well and changes have been implemented based on its feedback.
This has passed BOM testing and ATH testing.
This PR is now ready for merge. We will merge it after approximately 72 hours if there is no negative feedback.
/label ready-for-merge
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 194 out of 194 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The most recent test failure was due to a Jenkins update center outage. I've scheduled a new build to start in about 6 hours. I assume that the update center outage will be resolved by then. |
* Replace if-cascades with switch * Reference to empty collection field replaced with method call (Java 5) * Anonymous type replaced with lambda (Java 8) * Replaced lambda with method reference (Java 8) * Statement lambda replaced with expression lambda (Java 8) * Loop replaced with 'Collection.removeIf()' (Java 8) * Null check replaced with method call (Java 9) * Statement replaced with enhanced 'switch' (Java 14) * Use text blocks (Java 15) * Use pattern variable (Java 16) * Replace if-cascades with switch * Use 'Comparator' combinator * Null check replaced with method call (Java 9) * Use pattern variable (Java 16) * Use record pattern (Java 21) * Use SequencedCollection method (Java 21) * Replaced lambda with method reference (Java 8) * Cleanup unused imports * Cleanup migrations (move comments, formatting, spotbugs...) * Remove duplicate comment Resolve GitHub Copilot review comment * Remove blank line that may confuse Copilot * Inject comments into source line Attempt to satisfy GitHub Copilot Complains about unreachable code * Simplify with return value of switch statement * Use pattern matching instanceof in one more case * Don't lose the NetBeans comment that was there before * Remove TODO for Java 21 pattern use, implemented * Align comments with Unicode character they describe --------- Co-authored-by: Mark Waite <[email protected]>
Now that core is moving towards Java 21 there are quite a few improvements to the code base that can be made thanks to that.
These changes were produced by "Java Level Migration" recipes provided by IntelliJ IDE, including:
Java 5
Java 8
if-cascades withswitchComparatorcombinatorCollection.removeIf()Java 9
Java 14
switchJava 15
Java 16
Java 21
SequencedCollectionmethodIn case there are any questions to the particular recipe or change don't hesitate to ping me.
There sure are some more improvements to be made, such as usage of
records. However I do not know how well these play with (de-)serialization and some of the mechanisms used in Jenkins / Stapler. As I do not want to risk breaking anything, this might be something to investigate in the future.So from what I can see all changes here are internal, so no API changes or change in behavior in general - just (subjectively) cleaner code through simplifications.
I know that "cleanup" or "refactoring" PRs are generally frowned upon due to the large change set.
Yet I hope that others also see the benefit of making use of the language features that are available to us and keep the code base up to date with them.
Testing done
Tested changes locally.
Proposed changelog entries
Proposed changelog category
/label internal, skip-changelog
Proposed upgrade guidelines
N/A
Submitter checklist
The issue, if it exists, is well-described.New public classes, fields, and methods are annotated with@Restrictedor have@since TODOJavadocs, as appropriate.New deprecations are annotated with@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not callevalto ease future introduction of Content Security Policy (CSP) directives (see documentation).For dependency updates, there are links to external changelogs and, if possible, full differentials.For new APIs and extension points, there is a link to at least one consumer.Desired reviewers
@jenkinsci/core-pr-reviewers
Before the changes are marked as
ready-for-merge:Maintainer checklist
If the change needs additional upgrade steps from users, theupgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).If it would make sense to backport the change to LTS, be a Bug or Improvement, and either the issue or pull request must be labeled aslts-candidateto be considered.