jamesfredley commented on code in PR #16025:
URL: https://github.com/apache/grails-core/pull/16025#discussion_r3677411983
##########
.github/workflows/gradle.yml:
##########
@@ -143,7 +143,7 @@ jobs:
distribution: liberica
java-version: ${{ matrix.java }}
- name: "🗄️ Restore dependency jar cache"
- uses: actions/cache@v4
+ uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
Review Comment:
Agreed, reverted. Every `actions/*` reference this PR had pinned is back to
its original version reference. `gradle.yml`, `vulnerability-scan.yml`,
`release-close.yml`, and `release-publish-docs.yml` now show no diff at all
against `8.0.x`.
The validator was changed to match the policy rather than fight it:
`RepositoryConventionsTask` now carries `EXEMPT_ACTION_OWNERS = ['actions',
'apache']`, which skips both the 40-hex SHA requirement and the cross-file
SHA-consistency check for those namespaces while still requiring a non-empty
`@ref`. Third-party actions and Docker digests stay enforced. New specs cover
the accept, reject, and trailing-`@` cases.
##########
.github/workflows/gradle.yml:
##########
@@ -81,7 +81,7 @@ jobs:
distribution: liberica
java-version: ${{ matrix.java }}
- name: "🗄️ Restore dependency jar cache"
- uses: actions/cache@v4
+ uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
Review Comment:
Agreed, reverted. Every `actions/*` reference this PR had pinned is back to
its original version reference. `gradle.yml`, `vulnerability-scan.yml`,
`release-close.yml`, and `release-publish-docs.yml` now show no diff at all
against `8.0.x`.
The validator was changed to match the policy rather than fight it:
`RepositoryConventionsTask` now carries `EXEMPT_ACTION_OWNERS = ['actions',
'apache']`, which skips both the 40-hex SHA requirement and the cross-file
SHA-consistency check for those namespaces while still requiring a non-empty
`@ref`. Third-party actions and Docker digests stay enforced. New specs cover
the accept, reject, and trailing-`@` cases.
##########
.github/workflows/gradle.yml:
##########
@@ -37,7 +37,7 @@ jobs:
distribution: liberica
java-version: 21
- name: "🗄️ Restore dependency jar cache"
- uses: actions/cache@v4
+ uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3
Review Comment:
Agreed, reverted. Every `actions/*` reference this PR had pinned is back to
its original version reference. `gradle.yml`, `vulnerability-scan.yml`,
`release-close.yml`, and `release-publish-docs.yml` now show no diff at all
against `8.0.x`.
The validator was changed to match the policy rather than fight it:
`RepositoryConventionsTask` now carries `EXEMPT_ACTION_OWNERS = ['actions',
'apache']`, which skips both the 40-hex SHA requirement and the cross-file
SHA-consistency check for those namespaces while still requiring a non-empty
`@ref`. Third-party actions and Docker digests stay enforced. New specs cover
the accept, reject, and trailing-`@` cases.
##########
.github/workflows/codeanalysis.yml:
##########
@@ -60,6 +60,24 @@ jobs:
file="build/reports/violations/$report"
[ -f "$file" ] && cat "$file" >> $GITHUB_STEP_SUMMARY || true
done
+ - name: "🔎 Run Core SpotBugs Advisory"
+ if: always()
+ continue-on-error: true
+ run: |
+ rm -f build/reports/violations/SPOTBUGS_VIOLATIONS.md
+ ./gradlew aggregateAnalysisViolations --continue
"-Pgrails.code-analysis.enabled.pmd.projects="
-Pgrails.code-analysis.enabled.spotbugs=true
-Pgrails.code-analysis.ignoreFailures=true
+ - name: "📤 Upload SpotBugs Advisory Reports"
+ if: always()
+ uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a
# v7.0.1
Review Comment:
This one is worth separating out, because it is not a pin this PR
introduced. Every `actions/*` reference in `codeanalysis.yml` is already
SHA-pinned on `8.0.x` today, including this `upload-artifact` line at the
enforced-report step. The advisory steps this PR adds simply copy the
convention already in force in the same file.
I deliberately did not convert them. Converting only the new lines would
leave two styles for the same action three lines apart, and converting the
whole file means also deciding whether `setup-java` here lands on `@v4` (as
`release.yml` and `vulnerability-scan.yml` use) or `@v5` (what the current pin
resolves to), which is exactly the kind of silent major-version decision you
flagged elsewhere in this review.
The gate no longer enforces SHAs for `actions/*`, so normalizing this file
is now unblocked and safe to do as a standalone change. Say the word and I will
either do it here or open an issue.
##########
.github/workflows/codeanalysis.yml:
##########
@@ -95,3 +113,22 @@ jobs:
file="grails-gradle/build/reports/violations/$report"
[ -f "$file" ] && cat "$file" >> $GITHUB_STEP_SUMMARY || true
done
+ - name: "🔎 Run Gradle Plugin SpotBugs Advisory"
+ if: always()
+ continue-on-error: true
+ working-directory: grails-gradle
+ run: |
+ rm -f build/reports/violations/SPOTBUGS_VIOLATIONS.md
+ ./gradlew aggregateAnalysisViolations --continue
-Pgrails.code-analysis.enabled.spotbugs=true
-Pgrails.code-analysis.ignoreFailures=true
+ - name: "📤 Upload Gradle Plugin SpotBugs Advisory Reports"
+ if: always()
+ uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a
# v7.0.1
Review Comment:
Same as the core-job thread above: this `upload-artifact` pin already exists
on `8.0.x` in this file, and the new advisory step matches it rather than
introducing a third style. The validator no longer enforces SHAs for
`actions/*`, so converting this file wholesale is unblocked and I would rather
do it as its own change than fold a major-version decision for `setup-java`
into this PR. Happy to do it either way.
##########
.github/workflows/vulnerability-scan.yml:
##########
@@ -36,9 +36,9 @@ jobs:
contents: read
steps:
- name: "📥 Checkout repository"
- uses: actions/checkout@v6
+ uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd #
v6.0.2
- name: "☕️ Setup JDK"
- uses: actions/setup-java@v4
+ uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 #
v5.2.0
Review Comment:
Agreed, reverted. Every `actions/*` reference this PR had pinned is back to
its original version reference. `gradle.yml`, `vulnerability-scan.yml`,
`release-close.yml`, and `release-publish-docs.yml` now show no diff at all
against `8.0.x`.
The validator was changed to match the policy rather than fight it:
`RepositoryConventionsTask` now carries `EXEMPT_ACTION_OWNERS = ['actions',
'apache']`, which skips both the 40-hex SHA requirement and the cross-file
SHA-consistency check for those namespaces while still requiring a non-empty
`@ref`. Third-party actions and Docker digests stay enforced. New specs cover
the accept, reject, and trailing-`@` cases.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisPlugin.groovy:
##########
@@ -170,16 +178,25 @@ class GrailsCodeAnalysisPlugin implements Plugin<Project>
{
def xmlReport = spotBugsReports.maybeCreate('xml')
xmlReport.required.set(true)
xmlReport.outputLocation.set(
- project.extensions.getByType(GrailsCodeAnalysisExtension)
- .reportsDirectory.get()
- .dir('spotbugs')
- .file("${project.name}-${it.name}.xml")
+
project.extensions.getByType(GrailsCodeAnalysisExtension)
+ .reportsDirectory
+ .file("spotbugs/${GradleUtils.reportFileName(project,
it.name)}")
Review Comment:
Restored. It now reads:
```groovy
xmlReport.outputLocation.set(
project.extensions.getByType(GrailsCodeAnalysisExtension)
.reportsDirectory.get()
.dir('spotbugs')
.file(GradleUtils.reportFileName(project, it.name))
)
```
The only thing kept from my version is `GradleUtils.reportFileName(project,
taskName)` in place of the old `${project.name}-${task.name}.xml`, which is the
collision fix for nested projects sharing a leaf name. Same shape applied to
the PMD, Checkstyle, and CodeNarc blocks.
--
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]
