Skip to content

Commit da00653

Browse files
sarahchen6sarahchen6
authored
Forbid new Groovy files (DataDog#11216)
Forbid new Groovy files Merge branch 'master' into sarahchen6/hard-junit-test-enforcement Add doc link to PR comment Merge branch 'master' into sarahchen6/hard-junit-test-enforcement Apply workflow to draft PRs as well Co-authored-by: sarah.chen <sarah.chen@datadoghq.com>
1 parent 39aaeae commit da00653

3 files changed

Lines changed: 52 additions & 113 deletions

File tree

‎.github/g2j-migrated-modules.txt‎

Lines changed: 0 additions & 12 deletions
This file was deleted.

‎.github/workflows/README.md‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,12 +122,11 @@ _Trigger:_ When creating or updating a pull request targeting `master`, or when
122122

123123
_Actions:_
124124

125-
* Fail the PR if a new Groovy test file is added to a module listed in [`.github/g2j-migrated-modules.txt`](../g2j-migrated-modules.txt) (hard enforcement),
126-
* Post a warning comment on the PR if a new Groovy test file is added to any other non-exempt module (soft warning). Instrumentation (`dd-java-agent/instrumentation/`) and smoke-test (`dd-smoke-tests/`) modules are exempt from this warning.
125+
* Fail the PR if it introduces any new `.groovy` file, including added, copied, or renamed files whose previous name was not already `.groovy`.
127126

128-
_Recovery:_ Re-write the Groovy test files in Java / JUnit 5. To override this check entirely, add the `tag: override-groovy-enforcement` label to the PR. Remove the label to re-enable enforcement.
127+
_Recovery:_ Re-write the new Groovy files in Java / JUnit. To override this check entirely, add the `tag: override-groovy-enforcement` label to the PR. Remove the label to re-enable enforcement.
129128

130-
_Notes:_ The migrated modules list is always read from `master`. Add a new entry to `.github/g2j-migrated-modules.txt` each time a module is migrated to Java / JUnit 5.
129+
_Notes:_ The override label skips the workflow entirely.
131130

132131
## Code Quality and Security
133132

‎.github/workflows/enforce-groovy-migration.yaml‎

Lines changed: 49 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -16,75 +16,64 @@ jobs:
1616
permissions:
1717
issues: write # Required to create a comment on the pull request
1818
pull-requests: write # Required to create a comment on the pull request
19-
contents: read # Required to read migrated modules file
2019
runs-on: ubuntu-latest
2120
steps:
22-
- name: Check for Groovy regressions
21+
- name: Check for new Groovy files
2322
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # 9.0.0
2423
with:
2524
github-token: ${{ secrets.GITHUB_TOKEN }}
2625
script: |
27-
// Skip draft pull requests
28-
if (context.payload.pull_request.draft) {
29-
return
26+
const managedMarker = '<!-- dd-trace-java-groovy-enforcement -->'
27+
28+
const deleteManagedComment = async () => {
29+
const comments = await github.rest.issues.listComments({
30+
issue_number: context.payload.pull_request.number,
31+
owner: context.repo.owner,
32+
repo: context.repo.repo
33+
})
34+
35+
const existingComment = comments.data.find(comment =>
36+
comment.body.includes(managedMarker)
37+
)
38+
39+
if (existingComment) {
40+
await github.rest.issues.deleteComment({
41+
comment_id: existingComment.id,
42+
owner: context.repo.owner,
43+
repo: context.repo.repo
44+
})
45+
}
3046
}
3147
3248
// Check for override label — skip all checks if label present
3349
const labels = context.payload.pull_request.labels.map(l => l.name)
3450
if (labels.includes('tag: override-groovy-enforcement')) {
51+
await deleteManagedComment()
3552
console.log('tag: override-groovy-enforcement label detected — skipping all checks.')
3653
return
3754
}
3855
39-
// Read migrated modules list from master
40-
const migratedMods = await github.rest.repos.getContent({
41-
owner: context.repo.owner,
42-
repo: context.repo.repo,
43-
path: '.github/g2j-migrated-modules.txt',
44-
ref: 'master'
45-
})
46-
const migratedPrefixes = Buffer.from(migratedMods.data.content, 'base64')
47-
.toString()
48-
.split('\n')
49-
.map(l => l.trim())
50-
.filter(l => l && !l.startsWith('#'))
51-
5256
// Get all files changed in this PR
5357
const allFiles = await github.paginate(github.rest.pulls.listFiles, {
5458
owner: context.repo.owner,
5559
repo: context.repo.repo,
5660
pull_number: context.payload.pull_request.number
5761
})
5862
59-
// Filter these changed files to newly added Groovy files in any test source set
60-
const addedGroovy = allFiles.filter(f =>
61-
f.status === 'added' &&
62-
/\/src\/[^/]*[tT]est[^/]*\/groovy\/.*\.groovy$/.test(f.filename)
63-
)
64-
65-
// Extract module prefix from file path (everything before /src/(test|testFixtures)/groovy/)
66-
const moduleOf = path => {
67-
const m = path.match(/^(.*?)\/src\/(test|testFixtures)\/groovy\//)
68-
return m ? m[1] : null
69-
}
63+
// Fail if the PR introduces any new .groovy file
64+
// "renamed" only counts when the previous filename was not already Groovy.
65+
const introducedGroovy = allFiles.filter(file => {
66+
if (!file.filename.endsWith('.groovy')) {
67+
return false
68+
}
7069
71-
// Classify each added Groovy file
72-
const regressions = []
73-
const warnings = []
74-
for (const file of addedGroovy) {
75-
const path = file.filename
76-
const mod = moduleOf(path)
77-
if (migratedPrefixes.some(prefix => path.startsWith(prefix + '/'))) {
78-
regressions.push({ path, mod })
79-
} else if (
80-
path.startsWith('dd-java-agent/instrumentation/') ||
81-
path.startsWith('dd-smoke-tests/')
82-
) {
83-
// ignore Groovy file additions to instrumentations and smoke-tests for now
84-
} else {
85-
warnings.push({ path, mod })
70+
if (file.status === 'added' || file.status === 'copied') {
71+
return true
8672
}
87-
}
73+
74+
return file.status === 'renamed' &&
75+
(!file.previous_filename || !file.previous_filename.endsWith('.groovy'))
76+
})
8877
8978
// Fetch existing comments once
9079
const comments = await github.rest.issues.listComments({
@@ -93,57 +82,21 @@ jobs:
9382
repo: context.repo.repo
9483
})
9584
96-
const regressionMarker = '<!-- dd-trace-java-groovy-regression -->'
97-
const warningMarker = '<!-- dd-trace-java-groovy-warning -->'
98-
const existingRegressionComment = comments.data.find(c => c.body.includes(regressionMarker))
99-
const existingWarningComment = comments.data.find(c => c.body.includes(warningMarker))
100-
101-
// Handle regression comment
102-
if (regressions.length > 0) {
103-
const fileList = regressions
104-
.map(({ path, mod }) => `- \`${path}\` (module: \`${mod}\`)`)
105-
.join('\n')
106-
const body = `**❌ Groovy Test Regression Detected**\n\n` +
107-
`The following files add Groovy tests to modules that have been fully migrated to Java / JUnit 5:\n\n` +
108-
`${fileList}\n\n` +
109-
`These modules no longer accept Groovy test files. Please rewrite the test in Java / JUnit 5 instead.\n\n` +
110-
regressionMarker
111-
if (existingRegressionComment) {
112-
await github.rest.issues.updateComment({
113-
comment_id: existingRegressionComment.id,
114-
owner: context.repo.owner,
115-
repo: context.repo.repo,
116-
body
117-
})
118-
} else {
119-
await github.rest.issues.createComment({
120-
issue_number: context.payload.pull_request.number,
121-
owner: context.repo.owner,
122-
repo: context.repo.repo,
123-
body
124-
})
125-
}
126-
} else if (existingRegressionComment) {
127-
await github.rest.issues.deleteComment({
128-
comment_id: existingRegressionComment.id,
129-
owner: context.repo.owner,
130-
repo: context.repo.repo
131-
})
132-
}
85+
const existingComment = comments.data.find(comment => comment.body.includes(managedMarker))
13386
134-
// Handle warning comment
135-
if (warnings.length > 0) {
136-
const fileList = warnings
137-
.map(({ path, mod }) => `- \`${path}\` (module: \`${mod}\`)`)
87+
if (introducedGroovy.length > 0) {
88+
const fileList = introducedGroovy
89+
.map(({ filename }) => `- \`${filename}\``)
13890
.join('\n')
139-
const body = `**⚠️ New Groovy Test Files Added**\n\n` +
140-
`The following files add Groovy tests to modules that are candidates for migration to Java / JUnit 5:\n\n` +
91+
const body = `**❌ New Groovy Files Detected**\n\n` +
92+
`Please avoid introducing new \`.groovy\` files to this repository.\n\n` +
14193
`${fileList}\n\n` +
142-
`Consider writing these tests in Java / JUnit 5 instead to help with the ongoing migration effort.\n\n` +
143-
warningMarker
144-
if (existingWarningComment) {
94+
`Instead, rewrite the new file(s) in Java / JUnit. See the [How to Test With JUnit Guide](https://github.com/DataDog/dd-trace-java/blob/master/docs/how_to_test_with_junit.md) for more details.\n\n` +
95+
`If this PR needs an exception, add the \`tag: override-groovy-enforcement\` label to bypass this workflow.\n\n` +
96+
managedMarker
97+
if (existingComment) {
14598
await github.rest.issues.updateComment({
146-
comment_id: existingWarningComment.id,
99+
comment_id: existingComment.id,
147100
owner: context.repo.owner,
148101
repo: context.repo.repo,
149102
body
@@ -156,15 +109,14 @@ jobs:
156109
body
157110
})
158111
}
159-
} else if (existingWarningComment) {
112+
} else if (existingComment) {
160113
await github.rest.issues.deleteComment({
161-
comment_id: existingWarningComment.id,
114+
comment_id: existingComment.id,
162115
owner: context.repo.owner,
163116
repo: context.repo.repo
164117
})
165118
}
166119
167-
// Fail the check if there are regressions
168-
if (regressions.length > 0) {
169-
core.setFailed(`${regressions.length} Groovy regression(s) detected in migrated module(s). See PR comment for details. To skip this check entirely, add the 'tag: override-groovy-enforcement' label.`)
120+
if (introducedGroovy.length > 0) {
121+
core.setFailed(`${introducedGroovy.length} new Groovy file(s) detected. See PR comment for details. To bypass this workflow, add the 'tag: override-groovy-enforcement' label.`)
170122
}

0 commit comments

Comments
 (0)