Merge pull request #7649 from element-hq/feature/bma/replaceDangerByGhAction

Replace Danger with actions/github-script
2026-10-31 08:15:02 +00:00 · 2026-09-09 11:30:13 +02:00
parent 3106fec05f d0d30dc6e2
commit 8e1a34cff8
9 changed files with 287 additions and 326 deletions
@@ -1,33 +0,0 @@
-name: Danger CI
-
-on: [pull_request, merge_group]
-
-permissions: {}
-
-jobs:
-  build:
-    runs-on: ubuntu-latest
-    name: Danger main check
-    # Skip in forks, it doesn't work even with the fallback token
-    if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'element-hq/element-x-android' }}
-    steps:
-      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
-        with:
-          persist-credentials: false
-      - name: Add SSH private keys for submodule repositories
-        uses: webfactory/ssh-agent@e83874834305fe9a4a2997156cb26c5de65a8555 # v0.10.0
-        with:
-          ssh-private-key: ${{ secrets.ELEMENT_ENTERPRISE_DEPLOY_KEY }}
-      - name: Clone submodules
-        if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'element-hq/element-x-android' }}
-        run: git submodule update --init --recursive
-      - run: |
-          npm install --save-dev @babel/plugin-transform-flow-strip-types
-      - name: Danger
-        uses: danger/danger-js@a236e3b8c41f0babbf133568db851a20a34f9a7b # 14.0.5
-        with:
-          args: "--dangerfile ./tools/danger/dangerfile.js"
-        env:
-          DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }}
-          # Fallback for forks
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
@@ -0,0 +1,126 @@
+name: PR Checks
+
+on:
+  # Privilege escalation necessary to comment on pull requests from forks.
+  # 🚨 We must not check out or execute any code here, and be careful around use of user-controlled inputs.
+  pull_request_target: # zizmor: ignore[dangerous-triggers]
+    types: [ opened, reopened, edited, synchronize, labeled, unlabeled ]
+
+permissions: {}
+
+jobs:
+  pr-checks:
+    name: PR Checks
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+
+    concurrency:
+      # Only allow a single run of this workflow on each branch, automatically cancelling older runs.
+      group: ${{ format('pr-checks-{0}', github.ref) }}
+      cancel-in-progress: true
+
+    permissions:
+      # Required to comment on the pull request.
+      pull-requests: write
+
+    steps:
+      - name: Check the pull request
+        uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
+        with:
+          script: |
+            // Classes that must not be modified anymore. Add the file name suffix here to freeze a class.
+            const FROZEN_CLASSES = [
+            ];
+
+            // Only this user is allowed to edit the translation files, since they come from Localazy.
+            const TRANSLATION_ALLOW_LIST = ["ElementBot"];
+
+            const pr = context.payload.pull_request;
+            const body = pr.body ?? "";
+
+            const files = await github.paginate(github.rest.pulls.listFiles, {
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              pull_number: pr.number
+            });
+            // Deleted files are not interesting for any of the checks below.
+            const paths = files.filter(file => file.status !== "removed").map(file => file.filename);
+
+            const warnings = [];
+            const failures = [];
+
+            if (!body) {
+              warnings.push("Please provide a description for this PR.");
+            }
+
+            if (paths.length > 50) {
+              warnings.push("This pull request seems relatively large. Please consider splitting it into multiple smaller ones.");
+            }
+
+            if (pr.title.endsWith("…")) {
+              failures.push("Please provide a complete title that can be used as a changelog entry.");
+            }
+
+            if (pr.labels.filter(label => label.name.startsWith("PR-")).length !== 1) {
+              failures.push("Please add a single `PR-` label to categorise the changelog entry.");
+            }
+
+            for (const frozen of FROZEN_CLASSES) {
+              if (paths.some(path => path.endsWith(frozen))) {
+                failures.push(`Frozen class \`${frozen}\` has been modified. Please do not modify frozen class.`);
+              }
+            }
+
+            // Exclude the recorded screenshots, which are legitimately pngs.
+            if (paths.some(path => path.toLowerCase().endsWith(".png") && !path.includes("snapshots/images/"))) {
+              warnings.push("You seem to have made changes to some images. Please consider using a vector drawable.");
+            }
+
+            if (!TRANSLATION_ALLOW_LIST.includes(pr.user.login) && paths.some(path => path.endsWith("translations.xml"))) {
+              failures.push("Some translation files have been edited. Only user `ElementBot` (i.e. translations coming from Localazy) is allowed to do that. " +
+                "Please read more about translations management [in the doc](https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#strings).");
+            }
+
+            warnings.forEach(warning => core.warning(warning));
+            failures.forEach(failure => core.error(failure));
+
+            // Keep a single comment up to date rather than adding a new one on every run.
+            const marker = "<!-- pr-checks -->";
+            const comments = await github.paginate(github.rest.issues.listComments, {
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              issue_number: pr.number
+            });
+            const existing = comments.find(comment => comment.body.includes(marker));
+
+            if (warnings.length || failures.length) {
+              const lines = [...failures.map(failure => `- ❌ ${failure}`),
+                             ...warnings.map(warning => `- ⚠️ ${warning}`)];
+              const commentBody = `${marker}\n### PR Checks\n\n${lines.join("\n")}`;
+
+              if (existing) {
+                await github.rest.issues.updateComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  comment_id: existing.id,
+                  body: commentBody
+                });
+              } else {
+                await github.rest.issues.createComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  issue_number: pr.number,
+                  body: commentBody
+                });
+              }
+            } else if (existing) {
+              await github.rest.issues.deleteComment({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                comment_id: existing.id
+              });
+            }
+
+            if (failures.length) {
+              core.setFailed(`${failures.length} check(s) failed, see the comment on the pull request.`);
+            }
@@ -342,28 +342,166 @@ jobs:
    name: Project Check Suite
    runs-on: ubuntu-latest
    needs: [konsist, lint, ktlint, detekt]
-    if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'element-hq/element-x-android' }}
+    # Run even if a previous job failed: that is precisely when there are issues to report.
+    if: ${{ !cancelled() }}
+    permissions:
+      # Required to list the files modified by the pull request.
+      pull-requests: read
    steps:
-      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
-        with:
-          # Ensure we are building the branch and not the branch after being merged on develop
-          # https://github.com/actions/checkout/issues/881
-          ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.ref }}
-          persist-credentials: false
      - name: Download reports from previous jobs
        uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
-      - name: Prepare Danger
-        if: always()
-        run: |
-          npm install --save-dev @babel/core
-          npm install --save-dev @babel/plugin-transform-flow-strip-types
-          yarn add danger-plugin-lint-report --dev
-      - name: Danger lint
-        if: always()
-        uses: danger/danger-js@a236e3b8c41f0babbf133568db851a20a34f9a7b # 14.0.5
+      - name: Annotate the lint, ktlint and detekt issues
+        uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
        with:
-          args: "--dangerfile ./tools/danger/dangerfile-lint.js"
-        env:
-          DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }}
-          # Fallback for forks
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+          script: |
+            // GitHub only surfaces a handful of annotations per level and per step, so cap what we emit
+            // and let the job summary carry the full list.
+            const MAX_ANNOTATIONS = 20;
+            const MAX_SUMMARY_ROWS = 200;
+
+            function unescapeXml(value) {
+              return value
+                .replace(/&lt;/g, "<")
+                .replace(/&gt;/g, ">")
+                .replace(/&quot;/g, "\"")
+                .replace(/&apos;/g, "'")
+                .replace(/&#(\d+);/g, (_, code) => String.fromCharCode(Number(code)))
+                .replace(/&amp;/g, "&");
+            }
+
+            function attribute(tag, name) {
+              const match = new RegExp(`\\b${name}="([^"]*)"`).exec(tag);
+              return match ? unescapeXml(match[1]) : undefined;
+            }
+
+            // Paths in the reports are absolute paths on the runner which generated them, i.e.
+            // /home/runner/work/<repo>/<repo>/<path>.
+            const workspace = `/work/${context.repo.repo}/${context.repo.repo}/`;
+            function repoRelative(path) {
+              const index = path.indexOf(workspace);
+              return index === -1 ? path : path.slice(index + workspace.length);
+            }
+
+            // Checkstyle XML, as produced by ktlint and detekt:
+            //   <file name="…"><error line=".." column=".." severity=".." message=".." source=".."/></file>
+            function parseCheckstyle(content) {
+              const issues = [];
+              const fileRegex = /<file\s+([^>]*?)(\/>|>([\s\S]*?)<\/file>)/g;
+              let file;
+              while ((file = fileRegex.exec(content)) !== null) {
+                const path = attribute(file[1], "name");
+                if (!path) continue;
+                const errorRegex = /<error\s+([^>]*?)\/?>/g;
+                let error;
+                while ((error = errorRegex.exec(file[3] ?? "")) !== null) {
+                  issues.push({
+                    file: repoRelative(path),
+                    line: Number(attribute(error[1], "line")) || 1,
+                    column: Number(attribute(error[1], "column")) || undefined,
+                    severity: (attribute(error[1], "severity") ?? "warning").toLowerCase(),
+                    rule: attribute(error[1], "source"),
+                    message: attribute(error[1], "message") ?? "",
+                  });
+                }
+              }
+              return issues;
+            }
+
+            // Android lint XML, which is not Checkstyle:
+            //   <issue id=".." severity=".." message=".."><location file=".." line=".." column=".."/></issue>
+            function parseAndroidLint(content) {
+              const issues = [];
+              const issueRegex = /<issue\s+([^>]*?)(\/>|>([\s\S]*?)<\/issue>)/g;
+              let issue;
+              while ((issue = issueRegex.exec(content)) !== null) {
+                const location = /<location\s+([^>]*?)\/?>/.exec(issue[3] ?? "");
+                if (!location) continue;
+                const path = attribute(location[1], "file");
+                if (!path) continue;
+                issues.push({
+                  file: repoRelative(path),
+                  line: Number(attribute(location[1], "line")) || 1,
+                  column: Number(attribute(location[1], "column")) || undefined,
+                  severity: (attribute(issue[1], "severity") ?? "warning").toLowerCase(),
+                  rule: attribute(issue[1], "id"),
+                  message: attribute(issue[1], "message") ?? "",
+                });
+              }
+              return issues;
+            }
+
+            const reports = await (await glob.create("**/reports/**/*.xml")).glob();
+            core.info(`Found ${reports.length} XML report(s).`);
+
+            const fs = require("fs");
+            let issues = [];
+            for (const report of reports) {
+              const content = fs.readFileSync(report, "utf8");
+              if (content.includes("<checkstyle")) {
+                issues.push(...parseCheckstyle(content));
+              } else if (content.includes("<issues")) {
+                issues.push(...parseAndroidLint(content));
+              }
+            }
+
+            // Only report on the files touched by the pull request.
+            const pr = context.payload.pull_request;
+            if (pr) {
+              const files = await github.paginate(github.rest.pulls.listFiles, {
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                pull_number: pr.number
+              });
+              const changed = new Set(files.map(file => file.filename));
+              issues = issues.filter(issue => changed.has(issue.file));
+            }
+
+            const seen = new Set();
+            issues = issues.filter(issue => {
+              const key = `${issue.file}:${issue.line}:${issue.rule}:${issue.message}`;
+              if (seen.has(key)) return false;
+              seen.add(key);
+              return true;
+            });
+
+            if (issues.length === 0) {
+              core.info("No issue found in the files modified by this pull request.");
+              return;
+            }
+
+            const level = severity => {
+              if (severity === "error" || severity === "fatal") return "error";
+              if (severity === "warning") return "warning";
+              return "notice";
+            };
+            const rank = { error: 0, warning: 1, notice: 2 };
+            issues.sort((a, b) =>
+              rank[level(a.severity)] - rank[level(b.severity)] ||
+              a.file.localeCompare(b.file) ||
+              a.line - b.line
+            );
+
+            issues.slice(0, MAX_ANNOTATIONS).forEach(issue => {
+              const annotation = {
+                title: issue.rule,
+                file: issue.file,
+                startLine: issue.line,
+                startColumn: issue.column,
+              };
+              core[level(issue.severity)](issue.message, annotation);
+            });
+
+            core.summary.addHeading(`${issues.length} code quality issue(s)`, 3);
+            core.summary.addTable([
+              [{ data: "Severity", header: true }, { data: "File", header: true }, { data: "Rule", header: true }, { data: "Message", header: true }],
+              ...issues.slice(0, MAX_SUMMARY_ROWS).map(issue => [
+                level(issue.severity),
+                `${issue.file}:${issue.line}`,
+                issue.rule ?? "",
+                issue.message,
+              ]),
+            ]);
+            if (issues.length > MAX_SUMMARY_ROWS) {
+              core.summary.addRaw(`…and ${issues.length - MAX_SUMMARY_ROWS} more, see the uploaded reports.`, true);
+            }
+            await core.summary.write();