Conversation
|
Code Review This is a clean, well-scoped upgrade. The consolidation of repository declarations into Potential Issue: Previously This is probably intentional (the PR description says 'Add There is also a mild security note: Missing: gradle-wrapper.jar update When upgrading the Gradle wrapper, the binary Minor: distributionSha256Sum For reproducible/verified builds, The core change is correct The removal of |
7fd0578 to
e392e1a
Compare
Code ReviewThis is a clean, minimal PR. The approach is correct and the changes are focused. What looks good
Minor observationsImport still uses old package name ( import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJarShadow plugin 9.x ( No explicit dependencyResolutionManagement {
repositoriesMode.set(RepositoriesMode.FAIL_ON_PROJECT_REPOS)
repositories {
mavenLocal()
mavenCentral()
}
}Again, not blocking — just a note. SummaryThe change is correct and safe. No bugs, no security concerns, and no test changes are needed for a build tooling upgrade. The consolidation of repository declarations is the right approach for Gradle 9. |
Code ReviewOverall this is a clean, well-scoped upgrade. A few observations: Good
Minor Issues
dependencyResolutionManagement {
repositoriesMode.set(RepositoriesMode.FAIL_ON_PROJECT_REPOS)
repositories {
mavenLocal()
mavenCentral()
}
}Without this, any future
Stale buildscript {
repositories {
mavenCentral()
}
}This is separate from Leftover blank lines in SummaryThe core changes are correct and necessary. The |
7bddcea to
bb19227
Compare
|
test comment |
|
PR Review: Gradle 9.4.0 Upgrade Overall this is a clean, well-scoped upgrade focused on what Gradle 9 requires without unnecessary scope creep. Positive changes worth calling out:
Minor issues:
No concerns on:
|
|
@claude diagnose the error, what is wrong here? |
|
Claude finished @rustyrazorblade's task in 2m 36s —— View job Diagnosis
Root Cause: Kover now triggers
|
bb19227 to
1a98a58
Compare
PR Review: Gradle 9.4.0 UpgradeOverall this is a clean, well-scoped upgrade. The consolidation of repositories into 1.
|
|
@claude diagnose the failures |
|
Claude finished @rustyrazorblade's task in 2m 7s —— View job Diagnosis & Fix
Root Cause:
|
Code ReviewThe PR is well-scoped and the Gradle 9 migration approach is correct. Previous comments have covered most concerns. A few remaining observations: Unaddressed: bin/easy-db-lab auto-build removal The auto-build block was removed without explanation. If intentional, the script should fail fast when the JAR is missing rather than producing a cryptic error. The PR description does not mention this change, so it may have been unintentional. Minor: Hardcoded dependency in spark/common/build.gradle.kts The newly added junit-platform-launcher dependency has no version pin. It should use libs.junit.platform.launcher from the version catalog for consistency. Core changes are correct: Gradle 9.4.0 wrapper, dependencyResolutionManagement consolidation, and junit-platform-launcher in the testing bundle are all correct for the Gradle 9 upgrade. |
Code Coverage
|
|
What's failing? |
|
@claude Diagnose the failure |
|
Claude finished @rustyrazorblade's task in 49s —— View job Diagnosis & Fix
Root Cause: ktlint violation in
|
- Upgrade wrapper from 8.12.1 to 9.4.0 - Consolidate repository declarations into settings.gradle - Add useJUnitPlatform() to integrationTest task (Gradle 9 requires it) - Add junit-platform-launcher to version catalog and testing bundle - Add junit-platform-launcher to spark:common test runtime - Build by default in dev script (EASY_DB_LAB_SKIP_BUILD=1 to opt out) - Add EASY_DB_LAB_SKIP_DOCKER=1 to skip docker compose startup
koverXmlReport was automatically picking up integrationTest after useJUnitPlatform() was added to it, causing CI to try to run end-to-end tests that require real AWS infrastructure. Co-authored-by: Jon Haddad <rustyrazorblade@users.noreply.github.com>
The integration-test source set contained a single obsolete test using commands that no longer exist. The end-to-end-test script is the real integration test now.
Co-authored-by: Jon Haddad <rustyrazorblade@users.noreply.github.com>
c9b0169 to
e3ba08b
Compare
PR Review: Gradle 9.4.0 UpgradeThe core Gradle upgrade is solid — wrapper update, shadow plugin migration to 🔴 Breaking:
|
|
Address this: Shadow plugin import still uses old package name build.gradle.kts:1 still imports from the old package: import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar import com.gradleup.shadow.tasks.ShadowJar 🟡 junit-platform-launcher hardcoded version in spark/common In spark/common/build.gradle.kts, the launcher is added with a hardcoded version: testRuntimeOnly("org.junit.platform:junit-platform-launcher") testRuntimeOnly(libs.junit.platform.launcher) |
|
Claude finished @rustyrazorblade's task in 37s —— View job Fixing Shadow Plugin Import and Version Catalog Usage
Both changes applied in commit
|
…ion catalog for junit-platform-launcher Co-authored-by: Jon Haddad <rustyrazorblade@users.noreply.github.com>
PR Review: Gradle 9.4.0 UpgradeOverall this is a clean, well-scoped upgrade. The changes are minimal for the version jump and the consolidation of repository declarations is the right approach for Gradle 9. A few things worth discussing: Issues
The cleanest fix is to remove
The PR removes the Concerns Worth Flagging
The PR removes the entire
Minor
SummaryThe core upgrade is sound. The main actionable item is the |
|
@claude still has build failures |
|
Claude finished @rustyrazorblade's task in 1m 23s —— View job Diagnosis & Fix
Root Cause: Wrong shadow plugin import packageThe previous fix changed the import to CI error: Fix AppliedReverted import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJarShadow 9.x renamed the Gradle plugin ID ( Committed as |
…up package does not exist in shadow 9.x) Co-authored-by: Jon Haddad <rustyrazorblade@users.noreply.github.com>
PR Review: Upgrade Gradle to 9.4.0Overall this is a clean, well-structured upgrade. The changes follow Gradle 9's recommended patterns and the housekeeping (removing stale integration tests) is welcome. A few things worth discussing:
|
Upgrades Gradle from 8.12.1 to 9.4.0 to support the shadow plugin 9.x (
com.gradleup.shadow).dependencyResolutionManagementin settings.gradlemavenLocal()to settings for cassandra-analytics local buildsCloses #558
Generated with Claude Code