-
Notifications
You must be signed in to change notification settings - Fork 132
Upgrade Gradle to 9 along with JDK and Spotless #880
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
WalkthroughBuild and CI configuration updated: Gradle wrapper → 9.0.0, project Java toolchain → Java 17, Spotless and google-java-format upgraded, deprecated archivesBaseName replaced, Docker Compose and GitHub Actions switched to JDK 17, and one test expectation changed to InaccessibleObjectException. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Repo as Git Repository
participant CI as GitHub Actions (Temurin/17)
participant Gradle as Gradle Wrapper (9.0.0)
participant Docker as Docker Compose (OpenJDK 17)
Dev->>Repo: push changes (build/CI/tooling updates)
Repo->>CI: trigger workflows
CI->>CI: Set up Java 17 (temurin)
CI->>Gradle: run build/test with Gradle 9
Note right of Gradle: Spotless 6.25.0\ngoogle-java-format 1.10.0 applied
Dev->>Docker: docker-compose up package
Docker->>Docker: start container using OpenJDK 17 image
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docker-compose.yml (1)
3-3: Pin the JDK image to a fixed patch tag for reproducible builds
azul/zulu-openjdk:17-latestfloats to whatever Azul ships next. This can break CI the day a new 17.x patch appears (e.g. 17.0.20 → 17.0.22). Prefer a deterministic tag such as:- image: azul/zulu-openjdk:17-latest + image: azul/zulu-openjdk:17.0.22You still get security updates by bumping the tag deliberately and reviewing the diff.
build.gradle (1)
22-24: Avoid duplicated archive naming – keep it in one placeYou now set the base archive name twice:
base { archivesName.set('meilisearch-java') }(new)buildJar { archiveBaseName = 'meilisearch-java' }(existing)If one of them drifts, you’ll get two differently-named JARs in
build/libs. Drop one (prefer thebaseblock) or referenceproject.archiveBaseNameinside the custom task to stay DRY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.gradle(3 hunks)docker-compose.yml(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)
🔇 Additional comments (4)
gradle/wrapper/gradle-wrapper.properties (1)
3-3: Confirm wrapper JAR + plugin stack are really Gradle 9-ready
distributionUrlnow targets Gradle 9 GA. Make sure:
gradle/wrapper/gradle-wrapper.jarwas regenerated (./gradlew wrapper --gradle-version 9.0.0) and committed; otherwise CI will still download the old bootstrapper.- All applied plugins (Spotless 6.25.0, nexus-publish, etc.) were tested against Gradle 9 to catch any runtime incompatibilities early.
A quick smoke-run locally or in CI should surface problems, but please double-check before merging.
build.gradle (3)
16-17: Spotless upgrade looks goodBumping to 6.25.0 is required for Gradle 9 and has no breaking changes for Java formatters.
123-124: Java 17 target – validate dependency compatibilityRaising
sourceCompatibility/targetCompatibilityto 17 is aligned with the new Docker image, but a few dependencies (e.g.json:20250517,jackson-databind:2.19.0) historically lag behind latest JDKs. Please run the full test matrix on JDK 17 before merging to ensure no reflective-access or module-system issues surface.
199-199: Formatter bump acknowledgedUpdating Google Java Format to 1.10.0 (AOSP) syncs with Spotless 6.x; no concerns.
|
@joonseolee thank you for your PR Can you fix the tests? |
7f9bf6c to
e1a11fc
Compare
|
Do you mind if I ask you check it again? |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/tests.yml (1)
49-53: Linter job aligned to JDK 17.Spotless/format tasks will now run on the same toolchain as the build. Consider adding Gradle’s official setup step for better caching and wrapper verification (optional).
Add after “Set up Java”:
- name: Setup Gradle uses: gradle/actions/setup-gradle@v3 with: cache-read-only: ${{ github.ref != 'refs/heads/main' }}src/test/java/com/meilisearch/integration/ClientTest.java (2)
322-327: Make the failing-serialization assertion resilient and fix misleading var name.The specific exception can vary across JDK/Gson versions. Also, the variable name suggests “transient excluded” while it excludes only STATIC.
Apply:
- Gson gsonWithTransient = - new GsonBuilder().excludeFieldsWithModifiers(Modifier.STATIC).create(); + Gson gsonStaticOnly = + new GsonBuilder().excludeFieldsWithModifiers(Modifier.STATIC).create(); - Assertions.assertThrows(InaccessibleObjectException.class, () -> gsonWithTransient.toJson(test)); + Throwable thrown = Assertions.assertThrows(Throwable.class, () -> gsonStaticOnly.toJson(test)); + assertThat( + thrown, + anyOf( + instanceOf(InaccessibleObjectException.class), + instanceOf(JsonIOException.class), + instanceOf(StackOverflowError.class)));Alternatively, keep the strict assertion but add a brief comment explaining why STATIC (not TRANSIENT) is used to demonstrate the failure case.
3-6: Minor consistency: avoid mixing static and class-qualified Assertions.Either use static imports consistently (assertThrows/assertDoesNotThrow) or the Assertions class everywhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/pre-release-tests.yml(1 hunks).github/workflows/tests.yml(2 hunks)README.md(1 hunks)build.gradle(3 hunks)docker-compose.yml(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)src/test/java/com/meilisearch/integration/ClientTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- gradle/wrapper/gradle-wrapper.properties
- docker-compose.yml
- build.gradle
🔇 Additional comments (3)
.github/workflows/pre-release-tests.yml (1)
20-21: All JDK 8 references removed—workflows now fully on JDK 17
- Switched
.github/workflows/pre-release-tests.ymltojava-version: 17withdistribution: 'temurin', which unblocks Gradle 9 on CI and resolves the “Gradle requires JVM 17” error.- Ran a global search across all workflows for any remaining
java-version: 8,1.8ordistribution: 'zulu'patterns—no matches found.- No other CI workflows are pinning JDK 8.
.github/workflows/tests.yml (1)
24-25: Good: tests job now uses JDK 17 (Temurin).This aligns CI with Gradle 9’s minimum and should resolve the earlier failure.
src/test/java/com/meilisearch/integration/ClientTest.java (1)
16-16: Import of InaccessibleObjectException is correct for JDK 17+.
# Conflicts: # .github/workflows/tests.yml
|
Could you check it again? :) |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/tests.yml (2)
24-26: Good fix: JDK 17 aligns with Gradle 9 and should resolve the JVM 8 CI failure.Optional CI hardening suggestions:
- Add Gradle wrapper validation and the Gradle Build Action for better caching and safety.
- Add a short wait-for-health after starting Meilisearch to reduce flakes.
Apply incrementally as needed:
- uses: actions/checkout@v5 + - name: Validate Gradle Wrapper + uses: gradle/actions/wrapper-validation@v4 - name: Set up Java uses: actions/setup-java@v5 with: java-version: 17 distribution: 'temurin' cache: gradle + - name: Setup Gradle + uses: gradle/actions/setup-gradle@v4 + with: + cache-read-only: ${{ github.event_name == 'pull_request' }} - name: Grant execute permission for gradlew run: chmod +x gradlew - name: Meilisearch (latest) setup with Docker run: docker run -d -p 7700:7700 getmeili/meilisearch:latest meilisearch --master-key=masterKey --no-analytics + - name: Wait for Meilisearch health + run: | + for i in {1..30}; do + if curl -sf http://127.0.0.1:7700/health >/dev/null; then exit 0; fi + sleep 1 + done + echo "Meilisearch not healthy" >&2; exit 1Also consider pinning the Meilisearch image to a specific version instead of
latestto avoid surprise breakages.
49-49: LGTM on switching the linter job to JDK 17.If
scripts/lint.shinvokes Gradle/Spotless, enable Gradle cache here too.- name: Set up JDK 17 uses: actions/setup-java@v5 with: java-version: 17 distribution: 'temurin' + cache: gradleAlso applies to: 52-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pre-release-tests.yml(1 hunks).github/workflows/tests.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pre-release-tests.yml
curquiza
left a comment
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.
bors merge
Pull Request
Related issue
Fixes #869
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Chores
Documentation
Tests