Isolate mavenBom import requirements of Spring DM test project - #16077
Conversation
…ode being built is the code being tested
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16077 +/- ##
================================================
+ Coverage 0 51.8893% +51.8893%
- Complexity 0 18128 +18128
================================================
Files 0 2046 +2046
Lines 0 96280 +96280
Branches 0 16730 +16730
================================================
+ Hits 0 49959 +49959
- Misses 0 38949 +38949
- Partials 0 7372 +7372
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
jamesfredley
left a comment
jamesfredley
left a comment
There was a problem hiding this comment.
Review (replacement for #16073)
Direction: approve. Moving the Spring DM fixture into end-to-end/ is the correct replacement for #16073. It keeps the real mavenBom import (the point of the example) and sits behind the existing publish-then-consume boundary, so both original failures are fixed:
- Wrong BOM -
exclusiveContentonorg.apache.grails.*forcesbuild/local-maven; a remote snapshot cannot quietly win. - Unpublished-version bootstrap - removing it from root
settings.gradleunblocks core/release builds; the E2E job publishes first, then runscheck.
Prefer this over #16073, which stopped exercising the actual Maven BOM import path. CI is green including End to End.
Inline notes below are optional polish unless marked otherwise. None of them need to block merge if you want this in sooner.
Please track (can be this PR or immediate follow-ups)
- Follow-up PR (do not mix into this one unless tiny): carry the
BomPropertyOverridesPluginProjectDependencyskip + unit test from #16073 on its own. That defect (grails.core.ROOT:grails-hibernate*-bom:unspecified) is real and independent of this move; #16077 does not replace it. - Merge-up when this lands on
8.1.x/9.0.x: drop deadbomSnapshotNotPublishedfrom those branches'gradle.properties(and any remaining comments). It is still needed on those lines until the example is gone from core there; after this change forwards it becomes a no-op / dead property.
Optional only
| Item | Notes |
|---|---|
| Thin resolved-version assertion on the fixture | IT proves boot; not managed-version drift |
DO_NOT_CACHE_TESTS=1 on the E2E check step |
Defense in depth after republish |
| Confirm E2E is a required status check | Only if branch protection currently allows red E2E |
Revert group functionaltests → springdm |
Cosmetic |
| Matrix loss (Win / J25 / SiteMesh 2 paths) | Acceptable; one-line PR-body note is enough |
DirectoryScanner copies outside end-to-end |
Legitimate per-build; optional split for thinner review |
Verdict: Fine to merge as-is. Optional nits and the two tracked follow-ups above.
| apply from: rootProject.layout.projectDirectory.file('../dependencies.gradle') | ||
| ext['logback.version'] = bomDependencyVersions['logback.version'] | ||
| // Same situation for the Jackson 3 security override (CVE-2026-59889) - see jackson3.version in dependencies.gradle. | ||
| ext['jackson-bom.version'] = bomDependencyVersions['jackson3.version'] |
There was a problem hiding this comment.
Optional: Logback/jackson property overrides correctly document that Spring Boot version properties can win over the grails-bom import.
With the real mavenBom still here (unlike #16073's final approach), coordinate-level management from grails-bom should pin most other coords. A green HelloControllerSpec still will not notice silent property wins on e.g. groovy/log4j2/commons-codec.
If you want belt-and-suspenders without rewriting the fixture: a small verify… task on check asserting a few resolved versions (at least org.apache.grails:* → projectVersion, plus these logback/jackson pins). Not a merge blocker.
There was a problem hiding this comment.
This is the downside of the spring dm plugin. The purpose of this is to document compatibility and not try to revisit all of the problems with spring dm so I'm going to forego this.
| tasks.withType(Test).configureEach { | ||
| // Allow extra headroom for slow start-up under CI load (the client default is 60s), matching | ||
| // what gradle/functional-test-config.gradle gave this application in the core build. | ||
| systemProperty('grails.http.client.timeout', '120') |
There was a problem hiding this comment.
Optional follow-up: Timeout carry-over looks good.
Related optional idea from #16073: a tiny verification task that fails on version drift. The IT proves the app boots under Spring DM against the published BOM; it does not prove managed versions match the tree under test. Easy to add later if you prefer not to grow this PR.
| // inputs in any way Gradle can see, so re-running against freshly published artifacts is | ||
| // exactly the case that otherwise comes back UP-TO-DATE or FROM-CACHE. | ||
| outputs.cacheIf { !doNotCacheTests } | ||
| outputs.upToDateWhen { !doNotCacheTests } |
There was a problem hiding this comment.
Optional: Matching core's DO_NOT_CACHE_TESTS pattern is the right call - republished build/local-maven is invisible to Gradle inputs, so FROM-CACHE is a real footgun locally.
For CI, consider also setting DO_NOT_CACHE_TESTS=1 on the workflow check step. Not required on a clean runner that always publishes first; cheap insurance now that this workflow is the sole home of the Spring DM BOM-import fixture.
Also worth confirming the E2E job is a required status on 8.0.x - if optional, this path can go red without blocking merge.
| // would fix it. Set -PbomSnapshotNotPublished (or add it to gradle.properties) on such | ||
| // a branch to exclude the example until its first snapshot publish succeeds, then drop | ||
| // the flag again. The flag is presence-based; its value is ignored. | ||
| def bomSnapshotNotPublished = providers.gradleProperty('bomSnapshotNotPublished').isPresent() |
There was a problem hiding this comment.
Removing this from core is the load-bearing part of the bootstrap fix - core no longer needs a published BOM (or bomSnapshotNotPublished) to configure.
Merge-up (track explicitly): when this change lands on 8.1.x / 9.0.x, delete bomSnapshotNotPublished from those branches' gradle.properties and any remaining comments. Until the example is gone from core on those lines the flag is still needed; after the forward-port it is dead config.
ProjectDependency extends ModuleDependency, so the instanceof guard in
BomPropertyOverridesPlugin.detectDeclaredBoms did not filter out
platform(project(':some-bom')) - the idiom used throughout a multi-project
build. Such a dependency passed the guard, passed isPlatformDependency
(the platform category attribute is set either way) and had its coordinates
collected. An unversioned project reports its version as "unspecified",
producing grails.core.ROOT:grails-hibernate*-bom:unspecified.
BomManagedVersions.resolvePomFile catches the resulting failure and logs at
INFO, so this cost a wasted detached-configuration pom lookup per project
platform rather than breaking the build. The sharper case is when the
coordinates do match something published: :grails-bom resolves to
org.apache.grails:grails-bom:<projectVersion>, which exists on Apache
snapshots, so the lookup succeeds against the published pom and property
overrides get derived from it instead of from the BOM in the working tree.
Skip project dependencies outright. An in-build BOM has no published pom
whose <properties> could be read, so nothing could have depended on them
being detected. The same skip already exists for the same reason in
build-logic ExtractDependenciesTask.
Add a unit test covering the case; it fails against the unfixed plugin.
|
I've implemented all of @jamesfredley feedback. Going to merge since this blocks releasing correctly. |
2 participants
An alternative to #16073 since we now have the end to end tests - they also really only need to run in CI instead of locally.