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

Added 4 new Java 11 recipes #300

Merged
merged 8 commits into from
Sep 26, 2023
Merged

Conversation

cjobinabo
Copy link
Contributor

@cjobinabo cjobinabo commented Sep 19, 2023

What's changed?

Screenshot 2023-09-19 at 5 06 49 PM Screenshot 2023-09-19 at 5 06 27 PM Screenshot 2023-09-19 at 5 06 10 PM Screenshot 2023-09-19 at 5 05 53 PM

What's your motivation?

Anything in particular you'd like reviewers to focus on?

There seems to still be issues with the testJREDoNotUseSunNetSslInternalSslProvider and testJREDoNotUseSunNetSslInternalWwwProtocolHttpsHandler tests. It seems like the dependencies provided in sun.internal.jar aren't resolving properly despite the code compiling properly in a live environment using that jar. Feel free to look into that issue with me.

Anyone you would like to review specifically?

@timtebeek @joanvr

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@cjobinabo cjobinabo self-assigned this Sep 19, 2023
@cjobinabo cjobinabo added the recipe Recipe requested label Sep 20, 2023
Comment on lines 216 to 226
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.migrate.JREDoNotUseSunNetSslInternalWwwProtocolHttpsHandler
displayName: Use `com.ibm.net.ssl.www2.protocol.https.Handler` instead of `com.sun.net.ssl.internal.www.protocol.https.Handler`
description: Do not use the com.sun.net.ssl.internal.www.protocol.https.Handler class.
tags:
- java11
recipeList:
- org.openrewrite.java.ChangeType:
oldFullyQualifiedTypeName: com.sun.net.ssl.internal.www.protocol.https.Handler
newFullyQualifiedTypeName: com.ibm.net.ssl.www2.protocol.https.Handler
ignoreDefinition: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm doubting if we should migrate folks over to the ibm classes; would those not be constrained to a particular JVM vendor or dependency?

Curious to hear your thoughts as I might lack some background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of the recipes I mentioned would be IBM-specific. It's possible other solutions exist, but this is one we provided an alternative for. This recipe would really be aimed at our customers. If you want I can remove the recipe from the general Java 11 list.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think removing it from the java-version-11.yml would be best indeed; would it fit in better with rewrite-liberty perhaps? Or would you think a separate IBM, OpenJ9 or Semuru yml file would fit in best here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A separate ibm-java yml would be a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll push that update soon. I hadn't gotten a chance to revisit this PR until today. I just noticed that the build is passing now. Thanks for helping me work out those issues.

@cjobinabo
Copy link
Contributor Author

For whatever reason, the 2 tests I mentioned in the description are still failing locally. It seems like there is something about my environment I'll need to fix. That is something I'll need to investigate further later.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Nearly there I suppose.

src/main/resources/META-INF/rewrite/java-version-11.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/java-version-11.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/ibm-java.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/ibm-java.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/ibm-java.yml Outdated Show resolved Hide resolved
@timtebeek
Copy link
Contributor

Thanks again! :)

Copy link
Contributor

@kunli2 kunli2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contribution!

@cjobinabo
Copy link
Contributor Author

Thanks Kun! How can I kick off a new build when your fix goes live?

@kunli2
Copy link
Contributor

kunli2 commented Sep 26, 2023

The fix will need a while to go live, I think we can merge this directly, and I will re-run the build ~1 hour later to make sure it's green.

@kunli2 kunli2 merged commit 493eadc into openrewrite:main Sep 26, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants