Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Java 11 methods where possible #6700

Merged
merged 14 commits into from Jun 30, 2022
Merged

Conversation

basil
Copy link
Member

@basil basil commented Jun 25, 2022

Applies the OpenRewrite Migrate to Java 11 from Java 8 migration against this code base, along with any suggestions from IntelliJ's Java language migration aids and some manual touch-up.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added the skip-changelog Should not be shown in the changelog label Jun 25, 2022
@basil basil marked this pull request as draft June 25, 2022 19:11
bom/pom.xml Outdated
Comment on lines 43 to 44
<!-- TODO https://github.com/jenkinsci/stapler/pull/377 -->
<stapler.version>1701.va_9519c27c7c2</stapler.version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pom.xml Outdated
Comment on lines 91 to 92
<!-- TODO https://github.com/jenkinsci/remoting/pull/551 -->
<remoting.version>3032.v630e4c777a_75</remoting.version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to pretest these PRs, not actually required, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, have reverted now that BOM tests passed

@basil basil added the work-in-progress The PR is under active development, not ready to the final review label Jun 25, 2022
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the POM bumps, all looks fine to merge right away, from a random sampling of diffs (did not attempt to read every line). Any reason to hold back on this?

pom.xml Outdated
Comment on lines 91 to 92
<!-- TODO https://github.com/jenkinsci/remoting/pull/551 -->
<remoting.version>3032.v630e4c777a_75</remoting.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to pretest these PRs, not actually required, right?

@basil
Copy link
Member Author

basil commented Jun 28, 2022

Any reason to hold back on this?

Testing is still underway in jenkinsci/bom#1234

@basil
Copy link
Member Author

basil commented Jun 28, 2022

@basil basil marked this pull request as ready for review June 28, 2022 20:42
@basil basil removed the work-in-progress The PR is under active development, not ready to the final review label Jun 28, 2022
@basil basil requested a review from timja June 28, 2022 20:43
@basil
Copy link
Member Author

basil commented Jun 28, 2022

did not attempt to read every line)

I suggest at least reading the changes to core/src/main/resources/jenkins/security/whitelisted-classes.txt which were needed to prevent numerous failures in the core test suite

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through most of it, nice job :)

@timja timja requested a review from jglick June 28, 2022 21:28
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hot stuff, nice one 🔥

@basil
Copy link
Member Author

basil commented Jun 29, 2022

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

@basil basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 29, 2022
@basil basil merged commit 6d17999 into jenkinsci:master Jun 30, 2022
@basil basil deleted the java11-migration branch June 30, 2022 16:31
@@ -58,6 +58,7 @@ java.time.Ser
java.util.ArrayDeque
java.util.ArrayList
java.util.Arrays$ArrayList
java.util.CollSer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(private top-level class in ImmutableCollections.java)

@@ -103,6 +104,7 @@ java.util.GregorianCalendar
java.util.HashMap
java.util.HashSet
java.util.Hashtable
java.util.ImmutableCollections$List12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw #6700 (comment). For reference, I think this is so that tests like

p.addProperty(new ParametersDefinitionProperty(List.of(
could still work. We would not expect such entries to be used in production code, any more than we would older entries like

if (url.endsWith("/")) url = url.substring(0, url.length() - 1);
if (href.endsWith("/")) href = href.substring(0, href.length() - 1);

return url.endsWith(href);
}

public <T> List<T> singletonList(T t) {
return Collections.singletonList(t);
return List.of(t);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
5 participants