Update Gradle cache with the configurable plugin support#114
Conversation
andrew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, the marker detection logic is correct and the config wiring matches the existing upstream pattern. A few things before merging:
Checksums and metadata don't fall back. .sha1/.sha256/.md5 and maven-metadata.xml are routed through isMetadataFile to h.upstreamURL only, with no Plugin Portal fallback. I checked com.diffplug.spotless.gradle.plugin-8.4.0.pom.sha1: 404 on Central, 200 on the Plugin Portal. So the POM resolves but its checksum doesn't, which breaks builds with verification-metadata.xml enabled. The metadata path needs the same fallback treatment for marker coordinates.
Duplicate tests. TestMavenHandler_GradlePluginMarkerFallbackAndCache and the _BenManes variant are identical apart from the path string. Please collapse them into one table-driven test.
Minor: the h.pluginPortalUpstreamURL == "" check in shouldFallbackToPluginPortal is unreachable since the constructor always sets a default. Fine to drop it.
One thing to flag for a follow-up rather than this PR: the fallback only covers *.gradle.plugin marker artifacts. Once Gradle reads the marker it fetches the implementation jar (e.g. com.diffplug.spotless:spotless-plugin-gradle), which won't match the suffix and goes to Central only. Spotless publishes to both so it works, but Portal-only plugins will still fail at that step. The README now suggests pluginManagement { maven(...) } is fully handled, so worth either a caveat in the docs or a broader fallback later.
andrew
left a comment
There was a problem hiding this comment.
Overall the config wiring (config.go, env vars, docs, server.go) is clean and matches the existing npm/cargo upstream pattern, .module support is correct, and the fallback test coverage is thorough including the negative case and checksum sidecars.
A few things below. The main ones are scope (this PR bundles three unrelated changes) and ~30 lines of duplicated conditional-request handling.
Scope
This PR is doing three independent things:
- Configurable Maven upstream + Gradle Plugin Portal fallback (the stated purpose)
- Prometheus metrics for the Gradle build cache handler (
gradle.go+gradle_test.go) - README drive-by edits (Kotlin DSL fixes in the build cache snippet, blank-line whitespace)
The metrics work is reasonable on its own but unrelated to plugin portal fallback. Could it be split into its own PR?
Design
The README is upfront that fallback is marker-only and implementation jars still come from the primary upstream. That means a plugin published only to the Gradle Plugin Portal will resolve its marker through this proxy and then 404 on the actual jar. Is shipping the half-feature useful as-is, or should fallback cover implementation coordinates before this lands?
69bd53e to
d4fc3a8
Compare
|
Okay generally should be fine, acted with the changes as requested and squashed all commits since for feature clarity. Also separated the metrics to a separate PR #124 Additionally found potential performance bottleneck which was also addresses in a separate PR #125 |
andrew
left a comment
There was a problem hiding this comment.
Thanks, this round addresses everything substantive from before: metadata/checksum fallback is in, the 404 check uses errors.Is(err, ErrUpstreamNotFound), the conditional-request handling is shared via writeMetadataCachedResponse, tests are table-driven, and the metrics are split out. handleDownload now retries the Plugin Portal on 404 too, which closes the implementation-jar gap I'd flagged for follow-up.
One regression to fix before merge. The Gradle build-cache snippet in README.md got mangled, presumably when the Kotlin DSL change was backed out:
}
remote<HttpBuildCache> {
url = uri("http://localhost:8080/gradle/")
push = true
}Indentation is mixed and push = true no longer lines up. Please restore the original 2-space block.
A few non-blocking notes:
handleDownloadreturns 502 when both upstreams 404. Pre-existing, but with two upstreams it's more likely to be hit. Anerrors.Is(err, ErrUpstreamNotFound)→ 404 branch before the generic error would be tidier.- In
isPluginPortalFallbackFilethe bare.pom/.modulechecks are unreachable since the function is only entered viaisMetadataFile, which only matchesmaven-metadata.xmland checksum/signature suffixes. Fine to drop them. - The
release.ymlcosign bump in the diff is already onmainvia #122; a rebase will make it disappear. - Fair point on the markdownlint blank lines, happy to leave those.
cfe932f to
70c767b
Compare
Adds configurable Maven upstream support and Gradle Plugin Portal fallback for Gradle plugin marker artifacts on the Maven endpoint.
Introduces new upstream config/env options, wires them through server startup, and extends Maven artifact handling to include .module files.
Tests were added/updated to validate fallback and caching behavior for plugin markers, including real-world plugin coordinates.