search

cloudfoundry / cf-java-client

Java Client Library for Cloud Foundry
Apache License 2.0
327 stars 316 forks source link

Upgrade cf-java-client to support Spring Boot 3.1.x and 3.2.x with Java 17 #1198

Closed pacphi closed 11 months ago

pacphi commented 1 year ago

It's been 7 months since the 5.10.0.RELEASE. The current release documentation states that that release is compatible with Spring Boot release versions up to 2.6.x. Meanwhile, Spring Boot 3.1.x is the current release, with Spring Boot 3.2.x nearing GA.

There's not much that needed to change to prepare a version of the cf-java-client to be compatible with Spring Boot 3.1.x. I did, however, set the baseline for Java 17. (A decision that could of course be revisited). That required a change to the maven-compiler-plugin compileArgs configuration to facilitate source code generation and data-binding when using Immutables. I also took the test dependencies forward, so that in the future, the tests could be refactored to leverage JUnit 5 APIs. The rest of the changes are pretty straight-forward, just upgrades to dependencies and plugins to the latest available as of the date of this pull request.

linux-foundation-easycla[bot] commented 1 year ago

CLA Not Signed

pacphi commented 1 year ago

Built with

❯ mvn clean install -DskipTests -Dgpg.skip

Then 

❯ mvn test

...
[ERROR] Tests run: 636, Failures: 36, Errors: 0, Skipped: 2
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Cloud Foundry Java Client Parent 6.0.0.BUILD-SNAPSHOT:
[INFO] 
[INFO] Cloud Foundry Java Client Parent ................... SUCCESS [  0.309 s]
[INFO] Cloud Foundry Java Client .......................... SUCCESS [ 23.485 s]
[INFO] Cloud Foundry Java Utilities ....................... SUCCESS [ 15.961 s]
[INFO] Cloud Foundry Java Client - Reactor Implementation . SUCCESS [ 19.845 s]
[INFO] Cloud Foundry Java Operations ...................... FAILURE [ 16.091 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:15 min
[INFO] Finished at: 2023-10-03T19:04:35-07:00
[INFO] ------------------------------------------------------------------------

Then 

❯ cd cloudfoundry-operations
❯ mvn test

See output, here:

test-build.log

Need to figure out why ~5% of the total number of tests are failing.

anthonydahanne commented 12 months ago

Hello @pacphi ! Thanks for your PR! There's something that I wonder though: why do we need the upgrade to Spring Boot 3 / Java 17? java-cf-client does not depend on spring libraries... I mean sure cloudfoundry-client-reactor depends on some utilities from spring, but it does not depend on Spring APIs. Which leads me to think that: sure we can upgrade the maven wrapper, the unit tests to use Junit5 - but mandating users to run on Java 17 when they could simply run on Java 8; for no tangibles advantages... I'd prefer this PR to just upgrade all the dependencies (including the Spring ones that will be supported until 2027, see https://spring.io/projects/spring-framework#support)

Update 1hour later Do you have an example of a Spring Boot 3 application for example that cannot include java-cf-client ? If so could you just upgrade some dependencies to make them compatible with Java 17? Thanks!

pacphi commented 12 months ago

@anthonydahanne First, thanks for considering inclusion of this PR. To answer your questions and concerns.

If you continue to employ Spring Framework dependencies in the cf-java-client libraries, then when you upgrade dependencies to 6.0, that version's source code baselines support on Java 17. See What's New in Spring Framework 6.0.

And Spring Boot 3.1.x depends on Spring Framework 6, see https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.1-Release-Notes#dependency-upgrades.

So, yeah. We could strip away Spring Boot 3.1, and then you'd want to strip the convenient dependencies management which manages version for all transitives.

I went diving on dependencies, and I see spring-core and spring-web as dependencies in client, client-reactor, and util libs. And deeper I saw that the only two classes employed from Spring are:

Why not remove Spring dependencies entirely? You're just using it for convenience.

Update: I realize my last statement is easier said than done.

Update 2: To your point on no tangible advantages. We're addressing CVEs in transitives.

pacphi commented 12 months ago

Is it of utmost importance to maintain backward target compatibility with Java 8?

I realize extended support is available for that release until 2030. (Java 8 was first released in March 2014). But before that future, on current LTS release cadence, Java 25 will be out in September 2025.

anthonydahanne commented 12 months ago

@pacphi

I went diving on dependencies, and I see spring-core and spring-web as dependencies in client, client-reactor, and util libs. And deeper I saw that the only two classes employed from Spring are:

org.springframework.web.util.UriComponents
org.springframework.web.util.UriComponentsBuilder

Ah, I had a very quick look and came to the same conclusion!so weird to import 2 big libraries just for 2 classes; but I guess there was history.

Why not remove Spring dependencies entirely? You're just using it for convenience.

Indeed!

Update 2: To your point on no tangible advantages. We're addressing CVEs in transitives.

That's true; maybe it's possible to keep Spring Framework 5 BOM without importing any Spring Libs?

Is it of utmost importance to maintain backward target compatibility with Java 8? I realize extended support is available for that release until 2030. (Java 8 was first released in March 2014). But before that future, on current LTS release cadence, Java 25 will be out in September 2025.

Personally, I always prefer to use Java latest; but being new to this codebase (and to its users) I don't feel comfortable to impose an upgrade from Java 8 to Java 17 (9 versions); I imagine there are still some people on Java 8 out there.

Then we could branch, like your major upgrade suggested; but maintaining 2 branches is something we'd prefer to avoid for now.

Thanks for your understanding; let me know what you think!

pacphi commented 12 months ago

I don't feel comfortable to impose an upgrade from Java 8 to Java 17 (9 versions)

It's really only 2 LTS jumps. But I get it... to allow for the widest possible adoption and consumption we could stay pinned to Java 8.

Then we could branch, like your major upgrade suggested; but maintaining 2 branches is something we'd prefer to avoid for now.

If you don't like maintaining branches, perhaps prep building and packaging thru Maven profiles (and properties) to support Java 8 and Java17+ variants of the cf-java-client libraries? For the latter, just activate a Maven profile during build, that overrides properties, and produces artifact ids appended with -jdk17 or -jdk21? When publishing and releasing, you could then decide what versions of the JDK/JRE to support.

I have completed a proof-of-concept of this recommendation. See later commits. And you can take a surface-level look at the recently added Github Action workflow runs for each version of Java here.

My fork still suffers from some test failures in versions built with Java 17+. But for Java 8 and Java 11, tests do pass. (I could adapt workflow to run unit tests for each variant).

pacphi commented 12 months ago

Adapted workflow run (that executes tests for each child module) results here.

pacphi commented 12 months ago

I believe we are facing this issue when employing Mockito with JDK 17+. Not sure how to proceed.

christopherclark commented 11 months ago

@pacphi If you're having issues with EasyCLA, you can open a ticket with LF IT at the link on the check above, and someone should get you sorted out within a day or so. You could also try other link, select VMware, Inc. from the list of companies, and then retrigger the CLA check by commenting "/easycla" in this PR.

However, It looks like you're already approved under the VMware CLA, so I'm guessing those commits might be associated with an alternative email address? That happens sometimes. GH docs on that here: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

Regardless, opening a ticket should get this fixed quick.

DerrickLFX commented 11 months ago

/easycla

pacphi commented 11 months ago

My Git-fu is not strong enough.

What I'm going to do...

I'm going to close this PR.

anthonydahanne commented 11 months ago

replaced with: https://github.com/cloudfoundry/cf-java-client/pull/1201