Skip to content

Commit d0d30dc

Browse files
bmartyclaude
andcommitted
Replace Danger with actions/github-script
The PR metadata checks move to a new PR Checks workflow, and the lint, ktlint and detekt reports are now surfaced as inline annotations from the quality workflow. This removes the danger-js dependency, its ad-hoc npm installs and the need for the ElementBot token, so the checks also run on pull requests from forks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0e12d4e commit d0d30dc

9 files changed

Lines changed: 288 additions & 327 deletions

File tree

.github/workflows/danger.yml

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

.github/workflows/pr-checks.yml

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
name: PR Checks
2+
3+
on:
4+
# Privilege escalation necessary to comment on pull requests from forks.
5+
# 🚨 We must not check out or execute any code here, and be careful around use of user-controlled inputs.
6+
pull_request_target: # zizmor: ignore[dangerous-triggers]
7+
types: [ opened, reopened, edited, synchronize, labeled, unlabeled ]
8+
9+
permissions: {}
10+
11+
jobs:
12+
pr-checks:
13+
name: PR Checks
14+
runs-on: ubuntu-latest
15+
timeout-minutes: 5
16+
17+
concurrency:
18+
# Only allow a single run of this workflow on each branch, automatically cancelling older runs.
19+
group: ${{ format('pr-checks-{0}', github.ref) }}
20+
cancel-in-progress: true
21+
22+
permissions:
23+
# Required to comment on the pull request.
24+
pull-requests: write
25+
26+
steps:
27+
- name: Check the pull request
28+
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
29+
with:
30+
script: |
31+
// Classes that must not be modified anymore. Add the file name suffix here to freeze a class.
32+
const FROZEN_CLASSES = [
33+
];
34+
35+
// Only this user is allowed to edit the translation files, since they come from Localazy.
36+
const TRANSLATION_ALLOW_LIST = ["ElementBot"];
37+
38+
const pr = context.payload.pull_request;
39+
const body = pr.body ?? "";
40+
41+
const files = await github.paginate(github.rest.pulls.listFiles, {
42+
owner: context.repo.owner,
43+
repo: context.repo.repo,
44+
pull_number: pr.number
45+
});
46+
// Deleted files are not interesting for any of the checks below.
47+
const paths = files.filter(file => file.status !== "removed").map(file => file.filename);
48+
49+
const warnings = [];
50+
const failures = [];
51+
52+
if (!body) {
53+
warnings.push("Please provide a description for this PR.");
54+
}
55+
56+
if (paths.length > 50) {
57+
warnings.push("This pull request seems relatively large. Please consider splitting it into multiple smaller ones.");
58+
}
59+
60+
if (pr.title.endsWith("…")) {
61+
failures.push("Please provide a complete title that can be used as a changelog entry.");
62+
}
63+
64+
if (pr.labels.filter(label => label.name.startsWith("PR-")).length !== 1) {
65+
failures.push("Please add a single `PR-` label to categorise the changelog entry.");
66+
}
67+
68+
for (const frozen of FROZEN_CLASSES) {
69+
if (paths.some(path => path.endsWith(frozen))) {
70+
failures.push(`Frozen class \`${frozen}\` has been modified. Please do not modify frozen class.`);
71+
}
72+
}
73+
74+
// Exclude the recorded screenshots, which are legitimately pngs.
75+
if (paths.some(path => path.toLowerCase().endsWith(".png") && !path.includes("snapshots/images/"))) {
76+
warnings.push("You seem to have made changes to some images. Please consider using a vector drawable.");
77+
}
78+
79+
if (!TRANSLATION_ALLOW_LIST.includes(pr.user.login) && paths.some(path => path.endsWith("translations.xml"))) {
80+
failures.push("Some translation files have been edited. Only user `ElementBot` (i.e. translations coming from Localazy) is allowed to do that. " +
81+
"Please read more about translations management [in the doc](https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#strings).");
82+
}
83+
84+
warnings.forEach(warning => core.warning(warning));
85+
failures.forEach(failure => core.error(failure));
86+
87+
// Keep a single comment up to date rather than adding a new one on every run.
88+
const marker = "<!-- pr-checks -->";
89+
const comments = await github.paginate(github.rest.issues.listComments, {
90+
owner: context.repo.owner,
91+
repo: context.repo.repo,
92+
issue_number: pr.number
93+
});
94+
const existing = comments.find(comment => comment.body.includes(marker));
95+
96+
if (warnings.length || failures.length) {
97+
const lines = [...failures.map(failure => `- ❌ ${failure}`),
98+
...warnings.map(warning => `- ⚠️ ${warning}`)];
99+
const commentBody = `${marker}\n### PR Checks\n\n${lines.join("\n")}`;
100+
101+
if (existing) {
102+
await github.rest.issues.updateComment({
103+
owner: context.repo.owner,
104+
repo: context.repo.repo,
105+
comment_id: existing.id,
106+
body: commentBody
107+
});
108+
} else {
109+
await github.rest.issues.createComment({
110+
owner: context.repo.owner,
111+
repo: context.repo.repo,
112+
issue_number: pr.number,
113+
body: commentBody
114+
});
115+
}
116+
} else if (existing) {
117+
await github.rest.issues.deleteComment({
118+
owner: context.repo.owner,
119+
repo: context.repo.repo,
120+
comment_id: existing.id
121+
});
122+
}
123+
124+
if (failures.length) {
125+
core.setFailed(`${failures.length} check(s) failed, see the comment on the pull request.`);
126+
}

.github/workflows/quality.yml

Lines changed: 160 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -342,28 +342,166 @@ jobs:
342342
name: Project Check Suite
343343
runs-on: ubuntu-latest
344344
needs: [konsist, lint, ktlint, detekt]
345-
if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'element-hq/element-x-android' }}
345+
# Run even if a previous job failed: that is precisely when there are issues to report.
346+
if: ${{ !cancelled() }}
347+
permissions:
348+
# Required to list the files modified by the pull request.
349+
pull-requests: read
346350
steps:
347-
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
348-
with:
349-
# Ensure we are building the branch and not the branch after being merged on develop
350-
# https://github.com/actions/checkout/issues/881
351-
ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }}
352-
persist-credentials: false
353351
- name: Download reports from previous jobs
354352
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
355-
- name: Prepare Danger
356-
if: always()
357-
run: |
358-
npm install --save-dev @babel/core
359-
npm install --save-dev @babel/plugin-transform-flow-strip-types
360-
yarn add danger-plugin-lint-report --dev
361-
- name: Danger lint
362-
if: always()
363-
uses: danger/danger-js@a236e3b8c41f0babbf133568db851a20a34f9a7b # 14.0.5
364-
with:
365-
args: "--dangerfile ./tools/danger/dangerfile-lint.js"
366-
env:
367-
DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }}
368-
# Fallback for forks
369-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
353+
- name: Annotate the lint, ktlint and detekt issues
354+
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
355+
with:
356+
script: |
357+
// GitHub only surfaces a handful of annotations per level and per step, so cap what we emit
358+
// and let the job summary carry the full list.
359+
const MAX_ANNOTATIONS = 20;
360+
const MAX_SUMMARY_ROWS = 200;
361+
362+
function unescapeXml(value) {
363+
return value
364+
.replace(/&lt;/g, "<")
365+
.replace(/&gt;/g, ">")
366+
.replace(/&quot;/g, "\"")
367+
.replace(/&apos;/g, "'")
368+
.replace(/&#(\d+);/g, (_, code) => String.fromCharCode(Number(code)))
369+
.replace(/&amp;/g, "&");
370+
}
371+
372+
function attribute(tag, name) {
373+
const match = new RegExp(`\\b${name}="([^"]*)"`).exec(tag);
374+
return match ? unescapeXml(match[1]) : undefined;
375+
}
376+
377+
// Paths in the reports are absolute paths on the runner which generated them, i.e.
378+
// /home/runner/work/<repo>/<repo>/<path>.
379+
const workspace = `/work/${context.repo.repo}/${context.repo.repo}/`;
380+
function repoRelative(path) {
381+
const index = path.indexOf(workspace);
382+
return index === -1 ? path : path.slice(index + workspace.length);
383+
}
384+
385+
// Checkstyle XML, as produced by ktlint and detekt:
386+
// <file name="…"><error line=".." column=".." severity=".." message=".." source=".."/></file>
387+
function parseCheckstyle(content) {
388+
const issues = [];
389+
const fileRegex = /<file\s+([^>]*?)(\/>|>([\s\S]*?)<\/file>)/g;
390+
let file;
391+
while ((file = fileRegex.exec(content)) !== null) {
392+
const path = attribute(file[1], "name");
393+
if (!path) continue;
394+
const errorRegex = /<error\s+([^>]*?)\/?>/g;
395+
let error;
396+
while ((error = errorRegex.exec(file[3] ?? "")) !== null) {
397+
issues.push({
398+
file: repoRelative(path),
399+
line: Number(attribute(error[1], "line")) || 1,
400+
column: Number(attribute(error[1], "column")) || undefined,
401+
severity: (attribute(error[1], "severity") ?? "warning").toLowerCase(),
402+
rule: attribute(error[1], "source"),
403+
message: attribute(error[1], "message") ?? "",
404+
});
405+
}
406+
}
407+
return issues;
408+
}
409+
410+
// Android lint XML, which is not Checkstyle:
411+
// <issue id=".." severity=".." message=".."><location file=".." line=".." column=".."/></issue>
412+
function parseAndroidLint(content) {
413+
const issues = [];
414+
const issueRegex = /<issue\s+([^>]*?)(\/>|>([\s\S]*?)<\/issue>)/g;
415+
let issue;
416+
while ((issue = issueRegex.exec(content)) !== null) {
417+
const location = /<location\s+([^>]*?)\/?>/.exec(issue[3] ?? "");
418+
if (!location) continue;
419+
const path = attribute(location[1], "file");
420+
if (!path) continue;
421+
issues.push({
422+
file: repoRelative(path),
423+
line: Number(attribute(location[1], "line")) || 1,
424+
column: Number(attribute(location[1], "column")) || undefined,
425+
severity: (attribute(issue[1], "severity") ?? "warning").toLowerCase(),
426+
rule: attribute(issue[1], "id"),
427+
message: attribute(issue[1], "message") ?? "",
428+
});
429+
}
430+
return issues;
431+
}
432+
433+
const reports = await (await glob.create("**/reports/**/*.xml")).glob();
434+
core.info(`Found ${reports.length} XML report(s).`);
435+
436+
const fs = require("fs");
437+
let issues = [];
438+
for (const report of reports) {
439+
const content = fs.readFileSync(report, "utf8");
440+
if (content.includes("<checkstyle")) {
441+
issues.push(...parseCheckstyle(content));
442+
} else if (content.includes("<issues")) {
443+
issues.push(...parseAndroidLint(content));
444+
}
445+
}
446+
447+
// Only report on the files touched by the pull request.
448+
const pr = context.payload.pull_request;
449+
if (pr) {
450+
const files = await github.paginate(github.rest.pulls.listFiles, {
451+
owner: context.repo.owner,
452+
repo: context.repo.repo,
453+
pull_number: pr.number
454+
});
455+
const changed = new Set(files.map(file => file.filename));
456+
issues = issues.filter(issue => changed.has(issue.file));
457+
}
458+
459+
const seen = new Set();
460+
issues = issues.filter(issue => {
461+
const key = `${issue.file}:${issue.line}:${issue.rule}:${issue.message}`;
462+
if (seen.has(key)) return false;
463+
seen.add(key);
464+
return true;
465+
});
466+
467+
if (issues.length === 0) {
468+
core.info("No issue found in the files modified by this pull request.");
469+
return;
470+
}
471+
472+
const level = severity => {
473+
if (severity === "error" || severity === "fatal") return "error";
474+
if (severity === "warning") return "warning";
475+
return "notice";
476+
};
477+
const rank = { error: 0, warning: 1, notice: 2 };
478+
issues.sort((a, b) =>
479+
rank[level(a.severity)] - rank[level(b.severity)] ||
480+
a.file.localeCompare(b.file) ||
481+
a.line - b.line
482+
);
483+
484+
issues.slice(0, MAX_ANNOTATIONS).forEach(issue => {
485+
const annotation = {
486+
title: issue.rule,
487+
file: issue.file,
488+
startLine: issue.line,
489+
startColumn: issue.column,
490+
};
491+
core[level(issue.severity)](issue.message, annotation);
492+
});
493+
494+
core.summary.addHeading(`${issues.length} code quality issue(s)`, 3);
495+
core.summary.addTable([
496+
[{ data: "Severity", header: true }, { data: "File", header: true }, { data: "Rule", header: true }, { data: "Message", header: true }],
497+
...issues.slice(0, MAX_SUMMARY_ROWS).map(issue => [
498+
level(issue.severity),
499+
`${issue.file}:${issue.line}`,
500+
issue.rule ?? "",
501+
issue.message,
502+
]),
503+
]);
504+
if (issues.length > MAX_SUMMARY_ROWS) {
505+
core.summary.addRaw(`…and ${issues.length - MAX_SUMMARY_ROWS} more, see the uploaded reports.`, true);
506+
}
507+
await core.summary.write();

Gemfile

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

build.gradle.kts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ allprojects {
7272
verbose = true
7373
reporters {
7474
reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.PLAIN)
75-
// To have XML report for Danger
75+
// To have XML report for the CI to annotate the PR, see .github/workflows/quality.yml
7676
reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.CHECKSTYLE)
7777
}
7878
val generatedPath = "${layout.buildDirectory.asFile.get()}/generated/"

docs/continuous_integration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ The CI checks that:
4444
2. The tests are passing
4545
3. The code quality is good (detekt, ktlint, lint)
4646
4. The code is running and smoke tests are passing (maestro)
47-
5. The PullRequest itself is good (with danger)
47+
5. The PullRequest itself is good (see the [PR Checks workflow](../.github/workflows/pr-checks.yml))
4848
6. Files that must be added with git-lfs are added with git-lfs
4949

5050
## What is the CI reporting

0 commit comments

Comments
 (0)