jamesfredley commented on PR #15809: URL: https://github.com/apache/grails-core/pull/15809#issuecomment-4992629709
## Review feedback addressed Merged the latest `8.0.x` (large base gap - clean merge) and pushed follow-up commits. **@jdaugherty CHANGES_REQUESTED (all four addressed; threads resolved):** 1. *`GRAILS_REPO_URL` consistency across profiles & forge* - traced every `GRAILS_REPO_URL` / `grails.repo.url` consumer. HTTPS enforcement is now applied in **both** the Wrapper and Forge: `GradleRepository` rejects non-HTTPS remote overrides while accepting local filesystem repos (incl. Windows `C:\`, `C:/`, drive-relative `C:` paths). `INSTALL` reworded to scope the requirement to the Wrapper and Forge, noting the legacy Shell CLI preserves existing behavior. 2. *Redirect loop should use `MAX_REDIRECTS` as a hard cap* - implemented your suggested `HttpURLConnectionFactory` seam + hard-cap loop (`redirectCount >= MAX_REDIRECTS`), implicit redirect following disabled, HTTPS re-validated on every hop, missing `Location` fails, and the connection is closed on every exit path (including exceptional exits, so no leak). 3. + 4. *Don't use external URLs / `repo.example.test` in tests* - the HTTPS/redirect tests now run entirely against a local **Ersatz** mock server + an injected `HttpURLConnectionFactory`. No test performs real network I/O or depends on DNS resolving an external name. **Additional fixes surfaced by a dual GPT review pass:** - Never leak an `HttpURLConnection` when request setup / `getResponseCode()` / the redirect cap throws mid-iteration. - **Preserve repository fallback:** the eager metadata connection is opened inside `retrieveMavenMetadata`'s try, so an unavailable repo is reported as a missing release and `update()` falls back to the next repository / Maven Central instead of aborting. - Reject malformed URL-shaped remote overrides (leading-scheme detection) instead of silently treating them as local paths. - Align Forge's `GradleRepository` Windows-path handling with the Wrapper (drive-letter check before URI parsing) so `C:\`, `C:/`, `C:repo`, `C:repo\sub` and spaced paths are all accepted as local. Local verification: `:grails-wrapper:test` and `:grails-forge-core:test --tests GradleRepositorySpec` pass (Ersatz-driven, no external network). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
