-
Notifications
You must be signed in to change notification settings - Fork 74
MLE-23032 Bumping to Java 17 and OkHttp 5 #1818
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the Java Client API from Java 8/11 to Java 17 and updates OkHttp from version 4.x to 5.x, along with related dependency updates. The version is also bumped from 7.3-SNAPSHOT to 8.0-SNAPSHOT, indicating this is a major version upgrade.
- Updates Java target from 1.8 to 17 in Kotlin compilation settings
- Upgrades OkHttp dependencies from 4.12.0 to 5.1.0 and updates mockwebserver to mockwebserver3
- Updates supporting dependencies including okhttp-digest and logback-classic
- Modifies Jenkins build configuration to default to Java 17 instead of Java 8
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
gradle.properties | Version bump from 7.3-SNAPSHOT to 8.0-SNAPSHOT |
ml-development-tools/build.gradle | Updates Kotlin JVM target from 1.8 to 17 |
marklogic-client-api/build.gradle | Upgrades OkHttp from 4.12.0 to 5.1.0 and related dependencies |
marklogic-client-api-functionaltests/build.gradle | Updates test dependencies to use OkHttp 5.1.0 and logback 1.5.18 |
TokenAuthenticationInterceptorTest.java | Updates imports and API usage for mockwebserver3 compatibility |
OAuthAuthenticationConfigurerTest.java | Updates imports and test method to use mockwebserver3 API |
Jenkinsfile | Changes default Java version from 8 to 17 and simplifies version logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
try { | ||
mockWebServer.start(); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting the MockWebServer inside the enqueueResponseCodes
method is problematic as this method is called multiple times throughout the test lifecycle. The server should be started once in the test setup method, not every time responses are enqueued.
Copilot uses AI. Check for mistakes.
void test() throws Exception { | ||
MockWebServer mockWebServer = new MockWebServer(); | ||
mockWebServer.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MockWebServer is created and started but never properly shut down, which can lead to resource leaks. Consider adding a try-finally block or using @AfterEach to ensure the server is stopped after the test.
Copilot uses AI. Check for mistakes.
76d8dd0
to
7c377fd
Compare
Can revert this if we end up needing a 7.x release.