jamesfredley commented on code in PR #16025:
URL: https://github.com/apache/grails-core/pull/16025#discussion_r3677421153
##########
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsViolationAggregationPluginSpec.groovy:
##########
@@ -141,15 +163,496 @@ class GrailsViolationAggregationPluginSpec extends
Specification {
!testProjectDir.resolve('CHECKSTYLE_VIOLATIONS.md').toFile().exists()
!testProjectDir.resolve('CODENARC_VIOLATIONS.md').toFile().exists()
- and: "checkstyle report contains the violation"
+ and: "legacy Checkstyle XML is ignored"
def checkstyleMd = new File(violationsDir,
'CHECKSTYLE_VIOLATIONS.md').text
- checkstyleMd.contains('## Module: app-module')
- checkstyleMd.contains('JavadocPackageCheck')
+ !checkstyleMd.contains('Legacy report.')
- and: "codenarc report contains the violation"
+ and: "legacy CodeNarc XML is ignored"
def codenarcMd = new File(violationsDir, 'CODENARC_VIOLATIONS.md').text
- codenarcMd.contains('## Module: app-module')
- codenarcMd.contains('EmptyClass')
+ !codenarcMd.contains('Legacy report')
+ }
+
+ def "aggregateAnalysisViolations recognizes the PMD project allowlist and
reports disabled SpotBugs"() {
+ given:
+ testProjectDir.resolve('gradle.properties').toFile().text =
'''grails.code-analysis.enabled.pmd.projects=:app-module
+grails.code-analysis.ignoreFailures=true
+pmdVersion=7.25.0
Review Comment:
Fixed. `build-logic/plugins/build.gradle` already loads the root
`gradle.properties`, so the `test` task now forwards the values as system
properties and the specs read them:
```groovy
tasks.named('test') {
systemProperty 'grails.test.pmdVersion', gradleProperties.pmdVersion
systemProperty 'grails.test.codenarcVersion',
gradleProperties.codenarcVersion
}
```
No fixture pins a tool version literal any more, so the next bump cannot
leave the suite silently exercising an old toolchain. `mavenCentral()` stays in
the fixtures because they do have to resolve the analyzers to run them, which
is inherent to a TestKit test that actually executes PMD or CodeNarc.
##########
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisPluginSpec.groovy:
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.buildsrc
+
+import java.nio.file.Path
+
+import org.gradle.testkit.runner.GradleRunner
+import spock.lang.Specification
+import spock.lang.TempDir
+
+class GrailsCodeAnalysisPluginSpec extends Specification {
+
+ @TempDir
+ Path testProjectDir
+
+ def "PMD project allowlist enables only listed projects"() {
+ given:
+
writeMultiProjectBuild('grails.code-analysis.enabled.pmd.projects=:selected')
+
+ when:
+ def result = run('tasks', '--all', '--configuration-cache')
+
+ then:
+ result.output.contains('selected:pmdMain')
+ !result.output.contains('excluded:pmdMain')
+ }
+
+ def "global PMD opt-in remains compatible with project selection"() {
Review Comment:
Fixed by making the fixture match the name rather than by renaming it. The
feature now sets the global flag and a project-scoped opt-in at the same time
and asserts both projects still get `pmdMain`, so the OR contract of
`isToolEnabled` is genuinely verified. It also covers the third arm added by
the per-project extension change in the other thread, since `isToolEnabled` is
now global property OR project-paths property OR extension flag.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/RepositoryConventionsTask.groovy:
##########
@@ -0,0 +1,474 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.buildsrc
+
+import groovy.transform.CompileStatic
+
+import org.gradle.api.DefaultTask
+import org.gradle.api.GradleException
+import org.gradle.api.file.ConfigurableFileCollection
+import org.gradle.api.file.DirectoryProperty
+import org.gradle.api.file.RegularFileProperty
+import org.gradle.api.tasks.InputFiles
+import org.gradle.api.tasks.Internal
+import org.gradle.api.tasks.OutputFile
+import org.gradle.api.tasks.PathSensitive
+import org.gradle.api.tasks.PathSensitivity
+import org.gradle.api.tasks.TaskAction
+import org.yaml.snakeyaml.LoaderOptions
+import org.yaml.snakeyaml.Yaml
+import org.yaml.snakeyaml.constructor.SafeConstructor
+import org.yaml.snakeyaml.error.YAMLException
+
+import java.nio.charset.StandardCharsets
+import java.util.Set
+import java.util.regex.Matcher
+import java.util.regex.Pattern
+
+@CompileStatic
+abstract class RepositoryConventionsTask extends DefaultTask {
+
+ private static final Pattern AGENT_SKILL_PATH =
Pattern.compile(/\.agents\/skills\/[A-Za-z0-9_-]+\/SKILL\.md/)
+ private static final Pattern COMMIT_SHA = Pattern.compile(/^[0-9a-f]{40}$/)
+ private static final Pattern DOCKER_IMAGE_DIGEST =
Pattern.compile(/^docker:\/\/[^@\s]+@sha256:[0-9a-f]{64}$/)
+ private static final Pattern CONTAINER_IMAGE_DIGEST =
Pattern.compile(/^[^@\s]+@sha256:[0-9a-f]{64}$/)
+
+ @Internal
+ abstract DirectoryProperty getRepositoryDirectory()
+
+ @InputFiles
+ @PathSensitive(PathSensitivity.RELATIVE)
+ abstract ConfigurableFileCollection getConventionSources()
+
+ @OutputFile
+ abstract RegularFileProperty getReportFile()
+
+ @TaskAction
+ void validateRepositoryConventions() {
+ File root = repositoryDirectory.get().asFile
+ List<File> files = conventionSources.files.toList()
+ List<String> violations = []
+ validateSkills(root, files, violations)
+ validateActions(root, files, violations)
+ validateProperties(root, files, violations)
+ writeReport(violations)
+ if (!violations.isEmpty()) {
+ List<String> safeViolations = violations.collect { String
violation -> sanitizeViolation(violation) }
+ throw new GradleException("Repository convention violations:\n -
${safeViolations.join('\n - ')}\nSee ${reportFile.get().asFile}")
+ }
+ }
+
+ private static void validateSkills(File root, List<File> files,
List<String> violations) {
+ List<File> skills = files.findAll { relativePath(root, it) ==~
/^\.agents\/skills\/[^\/]+\/SKILL\.md$/ }.sort()
+ Map<String, File> names = [:]
+ Set<String> canonicalPaths = []
+ skills.each { File skill ->
+ String path = relativePath(root, skill)
+ String directoryName = skill.parentFile.name
+ Map<String, String> metadata = frontMatter(skill, path, violations)
+ ['name', 'description', 'license'].each { String key ->
+ if (!metadata[key]) {
+ violations.add("${path}: skill front matter is missing
'${key}'".toString())
+ }
+ }
+ String name = metadata['name']
+ if (name && name != directoryName) {
+ violations.add("${path}: skill name '${name}' does not match
directory '${directoryName}'".toString())
+ }
+ if (name && names.containsKey(name)) {
+ violations.add("${path}: skill name '${name}' duplicates
${relativePath(root, names[name])}".toString())
+ } else if (name) {
+ names[name] = skill
+ }
+ canonicalPaths << path
+ }
+
+ File agents = new File(root, 'AGENTS.md')
+ if (!agents.isFile()) {
+ violations << 'AGENTS.md: file is missing'
+ return
+ }
+ Set<String> documentedPaths = []
+ Matcher matcher = AGENT_SKILL_PATH.matcher(agents.text)
+ while (matcher.find()) {
+ documentedPaths << matcher.group()
+ }
+ canonicalPaths.each { String path ->
+ if (!documentedPaths.contains(path)) {
+ violations.add("AGENTS.md: missing canonical skill path
'${path}'".toString())
+ }
+ }
+ documentedPaths.each { String path ->
+ if (!new File(root, path).isFile()) {
+ violations.add("AGENTS.md: skill path '${path}' does not
exist".toString())
+ }
+ }
+ }
+
+ private static Map<String, String> frontMatter(File skill, String path,
List<String> violations) {
+ List<String> lines = skill.readLines()
+ if (lines.isEmpty() || lines[0] != '---') {
+ return [:]
+ }
+ int end = -1
+ for (int index = 1; index < lines.size(); index++) {
+ if (lines[index] == '---') {
+ end = index
+ break
+ }
+ }
+ if (end < 0) {
+ return [:]
+ }
+ Object document
+ try {
+ LoaderOptions options = new LoaderOptions()
+ options.setAllowDuplicateKeys(false)
+ document = new Yaml(new
SafeConstructor(options)).load(lines.subList(1, end).join('\n'))
+ } catch (YAMLException exception) {
+ violations.add("${path}: malformed skill front matter:
${exception.message}".toString())
+ return [:]
+ }
+ if (!(document instanceof Map)) {
+ violations.add("${path}: skill front matter must be a YAML
mapping".toString())
+ return [:]
+ }
+ Map<String, String> values = [:]
+ ['name', 'description', 'license'].each { String key ->
+ Object value = ((Map<?, ?>) document).get(key)
+ if (value instanceof String) {
+ values[key] = (String) value
+ } else if (value != null) {
+ violations.add("${path}: skill front matter field '${key}'
must be a string".toString())
+ }
+ }
+ values
+ }
+
+ private static void validateActions(File root, List<File> files,
List<String> violations) {
+ Map<String, String> actionShas = [:]
+ Map<String, String> actionFiles = [:]
+ Set<String> validatedManifests = []
+ files.findAll { File file -> isActionManifest(root, file)
}.sort().each { File manifest ->
+ validateActionManifest(root, manifest, actionShas, actionFiles,
violations, validatedManifests)
+ }
+ }
+
+ private static void validateActionManifest(File root, File manifest,
Map<String, String> actionShas,
+ Map<String, String> actionFiles, List<String> violations,
Set<String> validatedManifests) {
+ String canonicalPath = manifest.canonicalPath
+ if (!validatedManifests.add(canonicalPath)) {
+ return
+ }
+ String path = relativePath(root, manifest)
+ Object document = parseYaml(manifest, path, violations)
+ if (document != null) {
+ validateDockerActionImage(document, path, violations)
+ if (isWorkflowManifest(root, manifest)) {
+ validateWorkflowContainerImages(document, path, violations)
+ validateWorkflowUses(root, document, path, actionShas,
actionFiles, violations, validatedManifests)
+ } else {
+ validateCompositeActionUses(root, document, path, actionShas,
actionFiles, violations, validatedManifests)
+ }
+ }
+ }
+
+ private static boolean isActionManifest(File root, File file) {
+ String path = relativePath(root, file)
+ isWorkflowManifest(root, file) ||
+ path ==~ /(?:^|.*\/)action\.ya?ml$/
+ }
+
+ private static boolean isWorkflowManifest(File root, File file) {
+ relativePath(root, file) ==~ /^\.github\/workflows\/[^\/]+\.ya?ml$/
+ }
+
+ private static Object parseYaml(File manifest, String path, List<String>
violations) {
+ try {
+ LoaderOptions options = new LoaderOptions()
+ options.setAllowDuplicateKeys(false)
+ new Yaml(new SafeConstructor(options)).load(manifest.text)
+ } catch (YAMLException exception) {
+ violations.add("${path}: malformed YAML:
${exception.message}".toString())
+ null
+ }
+ }
+
+ private static void validateDockerActionImage(Object document, String
path, List<String> violations) {
+ if (!(document instanceof Map)) {
+ return
+ }
+ Object runs = ((Map<?, ?>) document).get('runs')
+ Object using = runs instanceof Map ? ((Map<?, ?>) runs).get('using') :
null
+ if (!(using instanceof String) || !((String)
using).equalsIgnoreCase('docker')) {
+ return
+ }
+ Object image = ((Map<?, ?>) runs).get('image')
+ String location = '$.runs.image'
+ if (!(image instanceof String)) {
+ violations.add("${path}:${location}: Docker action image must be a
string".toString())
+ } else if (((String) image).regionMatches(true, 0, 'docker://', 0,
'docker://'.length()) && !DOCKER_IMAGE_DIGEST.matcher((String)
image).matches()) {
+ violations.add("${path}:${location}: Docker action image
'${image}' must use an immutable sha256 digest".toString())
+ }
+ }
+
+ private static void validateWorkflowContainerImages(Object document,
String path, List<String> violations) {
+ if (!(document instanceof Map)) {
+ return
+ }
+ Object jobs = ((Map<?, ?>) document).get('jobs')
+ if (!(jobs instanceof Map)) {
+ return
+ }
+ ((Map<?, ?>) jobs).each { Object jobName, Object job ->
+ if (!(job instanceof Map)) {
+ return
+ }
+ String jobLocation = "\$.jobs.${jobName}"
+ Map<?, ?> jobDefinition = (Map<?, ?>) job
+ if (jobDefinition.containsKey('container')) {
+ Object container = jobDefinition.get('container')
+ if (container instanceof Map) {
+ validateContainerImage(((Map<?, ?>)
container).get('image'), "${jobLocation}.container.image", path, violations)
+ } else {
+ validateContainerImage(container,
"${jobLocation}.container", path, violations)
+ }
+ }
+ Object services = jobDefinition.get('services')
+ if (services instanceof Map) {
+ ((Map<?, ?>) services).each { Object serviceName, Object
service ->
+ if (service instanceof Map && ((Map<?, ?>)
service).containsKey('image')) {
+ validateContainerImage(((Map<?, ?>)
service).get('image'), "${jobLocation}.services.${serviceName}.image", path,
+ violations)
+ }
+ }
+ }
+ }
+ }
+
+ private static void validateContainerImage(Object image, String location,
String path, List<String> violations) {
+ if (!(image instanceof String)) {
+ violations.add("${path}:${location}: container image must be a
string".toString())
+ } else if (!CONTAINER_IMAGE_DIGEST.matcher((String) image).matches()) {
+ violations.add("${path}:${location}: container image '${image}'
must use an immutable sha256 digest".toString())
+ }
+ }
+
+ private static void validateWorkflowUses(File root, Object document,
String path, Map<String, String> actionShas,
+ Map<String, String> actionFiles, List<String> violations,
Set<String> validatedManifests) {
+ if (!(document instanceof Map)) {
+ return
+ }
+ Map<?, ?> workflow = (Map<?, ?>) document
+ validateStepUses(root, workflow.get('steps'), '$.steps', path,
actionShas, actionFiles, violations, validatedManifests)
+ Object jobs = workflow.get('jobs')
+ if (!(jobs instanceof Map)) {
+ return
+ }
+ ((Map<?, ?>) jobs).each { Object jobName, Object job ->
+ if (!(job instanceof Map)) {
+ return
+ }
+ Map<?, ?> jobDefinition = (Map<?, ?>) job
+ String jobLocation = "\$.jobs.${jobName}"
+ if (jobDefinition.containsKey('uses')) {
+ validateActionUse(root, jobDefinition.get('uses'),
"${jobLocation}.uses", path, actionShas, actionFiles, violations,
+ validatedManifests)
+ }
+ validateStepUses(root, jobDefinition.get('steps'),
"${jobLocation}.steps", path, actionShas, actionFiles, violations,
+ validatedManifests)
+ }
+ }
+
+ private static void validateCompositeActionUses(File root, Object
document, String path, Map<String, String> actionShas,
+ Map<String, String> actionFiles, List<String> violations,
Set<String> validatedManifests) {
+ if (!(document instanceof Map)) {
+ return
+ }
+ Object runs = ((Map<?, ?>) document).get('runs')
+ if (runs instanceof Map) {
+ validateStepUses(root, ((Map<?, ?>) runs).get('steps'),
'$.runs.steps', path, actionShas, actionFiles, violations,
+ validatedManifests)
+ }
+ }
+
+ private static void validateStepUses(File root, Object steps, String
location, String path, Map<String, String> actionShas,
+ Map<String, String> actionFiles, List<String> violations,
Set<String> validatedManifests) {
+ if (!(steps instanceof Iterable)) {
+ return
+ }
+ int index = 0
+ ((Iterable<?>) steps).each { Object step ->
+ if (step instanceof Map && ((Map<?, ?>) step).containsKey('uses'))
{
+ validateActionUse(root, ((Map<?, ?>) step).get('uses'),
"${location}[${index}].uses", path, actionShas, actionFiles,
+ violations, validatedManifests)
+ }
+ index++
+ }
+ }
+
+ private static void validateActionUse(File root, Object value, String
location, String path, Map<String, String> actionShas,
+ Map<String, String> actionFiles, List<String> violations,
Set<String> validatedManifests) {
+ if (!(value instanceof String)) {
+ violations.add("${path}:${location}: 'uses' must be a
string".toString())
+ return
+ }
+ String use = (String) value
+ if (use.startsWith('./')) {
+ validateLocalAction(root, use, location, path, actionShas,
actionFiles, violations, validatedManifests)
+ return
+ }
+ if (use.startsWith('docker://')) {
+ if (!DOCKER_IMAGE_DIGEST.matcher(use).matches()) {
+ violations.add("${path}:${location}: Docker action '${use}'
must use an immutable sha256 digest".toString())
+ }
+ return
+ }
+ int separator = use.lastIndexOf('@')
+ if (separator <= 0 || separator == use.length() - 1) {
+ violations.add("${path}:${location}: action '${use}' must use a
lowercase 40-hex commit SHA".toString())
+ return
+ }
+ String action = use.substring(0, separator)
+ String sha = use.substring(separator + 1)
+ if (!COMMIT_SHA.matcher(sha).matches()) {
+ violations.add("${path}:${location}: action '${action}' uses
'${sha}', not a lowercase 40-hex commit SHA".toString())
+ } else if (actionShas.containsKey(action) && actionShas[action] !=
sha) {
+ violations.add("${path}:${location}: action '${action}' uses
${sha}, inconsistent with ${actionShas[action]} in
${actionFiles[action]}".toString())
+ } else {
+ actionShas[action] = sha
+ actionFiles[action] = path
+ }
+ }
+
+ private static void validateLocalAction(File root, String use, String
location, String path,
+ Map<String, String> actionShas, Map<String, String> actionFiles,
List<String> violations,
+ Set<String> validatedManifests) {
+ File canonicalRoot = root.canonicalFile
+ File target = new File(root, use.substring(2)).canonicalFile
+ if (!target.toPath().startsWith(canonicalRoot.toPath())) {
+ violations.add("${path}:${location}: local action '${use}'
resolves outside the repository".toString())
+ return
+ }
+ if (target.isFile() && target.name ==~ /.*\.ya?ml/) {
Review Comment:
All three now have coverage: the direct-file local-action branch (a `uses:
./.github/workflows/x.yml` reference, including a regression asserting that an
external non-SHA action inside the referenced file is still reported, so a
break in the recursion cannot go silent), the trailing-`@` guard, and an
even-backslash line in `continues()` that must not continue onto the next line.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisPlugin.groovy:
##########
@@ -128,33 +131,38 @@ class GrailsCodeAnalysisPlugin implements Plugin<Project>
{
project.tasks.withType(Pmd).configureEach {
it.group = 'verification'
- it.onlyIf { !project.hasProperty('skipCodeStyle') }
+ it.onlyIf { !skipCodeStyle.present }
it.ignoreFailures = ignoreFailures.get()
if (it.name.contains('Test') || it.name.contains('test')) {
it.enabled = testStylingEnabled.get()
}
+ it.exclude { org.gradle.api.file.FileTreeElement element ->
Review Comment:
Fixed, `FileTreeElement` is imported and used by simple name. Same cleanup
applied to the fully-qualified `Property<Boolean>` that had crept into the
`isToolEnabled` signature.
--
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]
