jamesfredley commented on PR #16025:
URL: https://github.com/apache/grails-core/pull/16025#issuecomment-5122505746
Thanks for the very thorough pass, @jdaugherty. All 69 threads are answered
individually and pushed in 7a0aceb. The description is rewritten, because
several claims in it were no longer true. Summary of what changed and the three
places I did something other than what was literally asked.
## The pinning reversal
Every `actions/*` and `apache/*` pin this PR introduced is reverted.
`gradle.yml`, `vulnerability-scan.yml`, `release-close.yml` and
`release-publish-docs.yml` now have a **zero diff** against `8.0.x`. That also
removes the `actions/setup-java` v4 to v5 major-version crossing you spotted
inside the two release reproducibility pins, which was an unintended side
effect of mechanically swapping tags for the SHAs they currently resolve to.
Rather than leave the validator encoding the opposite of the policy,
`RepositoryConventionsTask` now exempts the `actions` and `apache` owner
namespaces from the 40-hex requirement and from the cross-file SHA consistency
check, while still requiring a non-empty `@ref`. Third-party actions and Docker
digests stay enforced.
## Three places I did not do exactly what was asked
1. **`codeanalysis.yml` `upload-artifact` pins.** Every `actions/*`
reference in that file is already SHA-pinned on `8.0.x`; the new advisory steps
copy the convention already in force in the same file. Converting only the new
lines would leave two styles three lines apart, and converting the file means
deciding whether `setup-java` lands on `@v4` or `@v5` there, which is exactly
the silent major-version decision you flagged elsewhere. The gate no longer
enforces SHAs for `actions/*`, so normalizing that file is unblocked as a
standalone change. Happy to do it here if you would rather.
2. **`release-drafter`.** Documented rather than re-pinned. There is now a
comment on the `COMMIT_SHA` constant recording that a 40-hex ref may be a
commit or an annotated-tag object, that both are immutable, and that the
validator cannot distinguish them.
3. **The `.gitignore` and RAT requests.** The premise turned out to be wrong
on my side: `CLAUDE.md`, `GEMINI.md`, `.clinerules`, `.cursorrules`,
`.windsurfrules` are tracked symlinks to `AGENTS.md`, and
`.claude/skills/*/SKILL.md` are tracked symlinks into `.agents/skills/`. So
they cannot be gitignored, and per your other comment they now ship in the
source zip again, stored as links by `zip -y`. I did try dropping their RAT
exclusion on the theory that RAT would follow the link to the licensed target;
`./gradlew rat` then failed with 8 unapproved licenses, because on a platform
without symlink support git materializes each one as a plain text file
containing the target path. The exclusion is kept with that written down as the
reason. `.gitignore` gained only the genuinely local `.omo/` and `.worktrees/`.
## Two things the final review caught that your comments led to
Both came out of re-reviewing the finished diff, and both were caused by
fixes to your feedback rather than by the original code.
**The conventions gate would have run in zero CI jobs.** Removing the
redundant `aggregateStyleViolations` edge, as you asked, was correct for the
task graph but no workflow actually invokes `aggregateViolations`.
`codestyle.yml` runs `aggregateStyleViolations` and `codeanalysis.yml` runs
`aggregateAnalysisViolations`, so the entire feature would have been unenforced
on PRs. The edge stays removed; the core `codestyle.yml` job now invokes
`validateRepositoryConventions` explicitly and publishes its report. There is a
regression test asserting both directions so the edge cannot be quietly
reintroduced.
**The `Modules analyzed:` header alone did not fix the partial-report
problem.** Because the reports are assembled from marker files on disk, a
module analyzed in an earlier run still contributed its stale markers, so a
single-module run could produce a report listing modules it never ran. I ended
up taking both of the options you offered in that thread: the header stays, and
`finalizeStyleViolations` / `finalizeAnalysisViolations` and all four analyzer
`finalizedBy` registrations are gone. Only the aggregate lane writes the
Markdown now. One consequence worth knowing: `--continue` is now required to
get a report when an analyzer fails, which the documented commands and every
workflow already pass, and that is documented in `AGENTS.md` and the skill.
## Other notable changes
- PMD and SpotBugs enablement moved out of `gradle.properties` entirely to a
per-project `grailsCodeAnalysis { pmdEnabled = true }` opt-in. The root
aggregator derives the enabled set from applied plugins, so adding a module is
a one-line change in that module and nothing central is edited. The `-P`
properties remain as overrides. One caveat: analyzer tasks are now registered
during `afterEvaluate`, so per-task customization must be wrapped in
`afterEvaluate { }`. That is documented.
- Configuration avoidance restored via `configureEach` and collection-based
edges; the cleanup task no longer wipes every module's reports or defeats
up-to-date checks.
- SnakeYAML now uses a managed `snakeyamlVersion=2.6` property instead of
the hardcoded 2.4.
- The `AGENTS.md` skill-index requirement is dropped, so this no longer
collides with #15977. Either an explicit index or directory-based discovery
passes. Whichever of us rebases second: the front-matter rules still require
string `name`, `description` and `license` on every canonical `SKILL.md`.
## Verification
`../gradlew :build-logic:test` from `build-logic`: **85 tests, 0 failures, 0
errors, 0 skipped**, up from 77. That includes the SpotBugs and Checkstyle
end-to-end paths you called out as missing, the style-side missing-report case,
aggregate report immutability under a direct analyzer run, and the CI wiring in
both directions. Fixtures no longer hardcode tool versions. `./gradlew rat`
passes.
No runtime module behavior changes, so CI will re-verify the full suite on
the updated branch.
--
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]
