diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml deleted file mode 100644 index bf1a302645..0000000000 --- a/.github/workflows/danger.yml +++ /dev/null @@ -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 }} diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml new file mode 100644 index 0000000000..175cf43509 --- /dev/null +++ b/.github/workflows/pr-checks.yml @@ -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 = ""; + 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.`); + } diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 91ba6a6e1d..b3a1921c3e 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -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(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, "\"") + .replace(/'/g, "'") + .replace(/&#(\d+);/g, (_, code) => String.fromCharCode(Number(code))) + .replace(/&/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///. + 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: + // + function parseCheckstyle(content) { + const issues = []; + const fileRegex = /]*?)(\/>|>([\s\S]*?)<\/file>)/g; + let file; + while ((file = fileRegex.exec(content)) !== null) { + const path = attribute(file[1], "name"); + if (!path) continue; + const errorRegex = /]*?)\/?>/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: + // + function parseAndroidLint(content) { + const issues = []; + const issueRegex = /]*?)(\/>|>([\s\S]*?)<\/issue>)/g; + let issue; + while ((issue = issueRegex.exec(content)) !== null) { + const location = /]*?)\/?>/.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(" 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(); diff --git a/Gemfile b/Gemfile deleted file mode 100644 index a432dd0319..0000000000 --- a/Gemfile +++ /dev/null @@ -1,3 +0,0 @@ -source "https://rubygems.org" - -gem 'danger' diff --git a/build.gradle.kts b/build.gradle.kts index 6bbc568c66..876383e325 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -72,7 +72,7 @@ allprojects { verbose = true reporters { reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.PLAIN) - // To have XML report for Danger + // To have XML report for the CI to annotate the PR, see .github/workflows/quality.yml reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.CHECKSTYLE) } val generatedPath = "${layout.buildDirectory.asFile.get()}/generated/" diff --git a/docs/continuous_integration.md b/docs/continuous_integration.md index 85a869cbe9..59ee8aac35 100644 --- a/docs/continuous_integration.md +++ b/docs/continuous_integration.md @@ -44,7 +44,7 @@ The CI checks that: 2. The tests are passing 3. The code quality is good (detekt, ktlint, lint) 4. The code is running and smoke tests are passing (maestro) -5. The PullRequest itself is good (with danger) +5. The PullRequest itself is good (see the [PR Checks workflow](../.github/workflows/pr-checks.yml)) 6. Files that must be added with git-lfs are added with git-lfs ## What is the CI reporting diff --git a/docs/danger.md b/docs/danger.md deleted file mode 100644 index 9adb382a3a..0000000000 --- a/docs/danger.md +++ /dev/null @@ -1,105 +0,0 @@ -## Danger - - - -* [What does danger checks](#what-does-danger-checks) - * [PR check](#pr-check) - * [Quality check](#quality-check) -* [Setup](#setup) -* [Run danger locally](#run-danger-locally) -* [Danger user](#danger-user) -* [Useful links](#useful-links) - - - -## What does danger checks - -### PR check - -See the [dangerfile](../tools/danger/dangerfile.js). If you add rules in the dangerfile, please update the list below! - -Here are the checks that Danger does so far: - -- PR description is not empty -- Big PR got a warning to recommend to split -- PR contains a correct title and a label to categorize the release note -- PR does not modify frozen classes -- PR with change on layout should include screenshot in the description (TODO Not supported yet!) -- PR which adds png file warn about the usage of vector drawables -- non draft PR should have a reviewer -- files containing translations are not modified by developers - -### Quality check - -After all the checks that generate checkstyle XML report, such as Ktlint, lint, or Detekt, Danger is run with this [dangerfile](../tools/danger/dangerfile-lint.js), in order to post comments to the PR with the detected error and warnings. - -To run locally, you will have to install the plugin `danger-plugin-lint-report` using: - -```shell -yarn add danger-plugin-lint-report --dev -``` - -## Setup - -This operation should not be necessary, since Danger is already setup for the project. - -To setup danger to the project, run: - -```shell -bundle exec danger init -``` - -## Run danger locally - -When modifying the [dangerfile](../tools/danger/dangerfile.js), you can check it by running Danger locally. - -To run danger locally, install it and run: - -```shell -bundle exec danger pr --dangerfile=./tools/danger/dangerfile.js -``` - -For instance: - -```shell -bundle exec danger pr https://github.com/element-hq/element-android/pull/6637 --dangerfile=./tools/danger/dangerfile.js -``` - -We may need to create a GitHub token to have less API rate limiting, and then set the env var: - -```shell -export DANGER_GITHUB_API_TOKEN='YOUR_TOKEN' -``` - -Swift and Kotlin (just in case) - -```shell -bundle exec danger-swift pr --dangerfile=./tools/danger/dangerfile.js -bundle exec danger-kotlin pr --dangerfile=./tools/danger/dangerfile.js -``` - -## Danger user - -To let Danger check all the PRs, including PRs form forks, a GitHub account have been created: -- login: ElementBot -- password: Stored on Passbolt -- GitHub token: A token with limited access has been created and added to the repository https://github.com/element-hq/element-x-android as secret DANGER_GITHUB_API_TOKEN. This token is not saved anywhere else. In case of problem, just delete it and create a new one, then update the secret. - -PRs from forks do not always have access to the secret `secrets.DANGER_GITHUB_API_TOKEN`, so `secrets.GITHUB_TOKEN` is also provided to the job environment. If `secrets.DANGER_GITHUB_API_TOKEN` is available, it will be used, so user `ElementBot` will comment the PR. Else `secrets.GITHUB_TOKEN` will be used, and bot `github-actions` will comment the PR. - -## Useful links - -- https://danger.systems/ -- https://danger.systems/js/ -- https://danger.systems/js/guides/getting_started.html -- https://danger.systems/js/reference.html -- https://github.com/danger/awesome-danger - -Some danger files to get inspired from - -- https://github.com/artsy/emission/blob/master/dangerfile.ts -- https://github.com/facebook/react-native/blob/master/bots/dangerfile.js -- https://github.com/apollographql/apollo-client/blob/master/config/dangerfile.ts -- https://github.com/styleguidist/react-styleguidist/blob/master/dangerfile.js -- https://github.com/storybooks/storybook/blob/master/dangerfile.js -- https://github.com/ReactiveX/rxjs/blob/master/dangerfile.js diff --git a/tools/danger/dangerfile-lint.js b/tools/danger/dangerfile-lint.js deleted file mode 100644 index 3023918656..0000000000 --- a/tools/danger/dangerfile-lint.js +++ /dev/null @@ -1,41 +0,0 @@ -/** - * Copyright (c) 2025 Element Creations Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. - * Please see LICENSE files in the repository root for full details. - */ - -import { schedule } from 'danger' - -/** - * Ref and documentation: https://github.com/damian-burke/danger-plugin-lint-report - * This file will check all the error in XML Checkstyle format. - * It covers, lint, ktlint, and detekt errors - */ - -const reporter = require("danger-plugin-lint-report") -schedule(reporter.scan({ - /** - * File mask used to find XML checkstyle reports. - */ - fileMask: "**/reports/**/**.xml", - /** - * If set to true, the severity will be used to switch between the different message formats (message, warn, fail). - */ - reportSeverity: true, - /** - * If set to true, only issues will be reported that are contained in the current changeset (line comparison). - * If set to false, all issues that are in modified files will be reported. - */ - requireLineModification: false, - /** - * Optional: Sets a prefix foreach violation message. - * This can be useful if there are multiple reports being parsed to make them distinguishable. - */ - // outputPrefix?: "" - - /** - * Optional: If set to true, it will remove duplicate violations. - */ - removeDuplicates: true, -})) diff --git a/tools/danger/dangerfile.js b/tools/danger/dangerfile.js deleted file mode 100644 index 634696d432..0000000000 --- a/tools/danger/dangerfile.js +++ /dev/null @@ -1,121 +0,0 @@ -/** - * Copyright (c) 2025 Element Creations Ltd. - * - * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. - * Please see LICENSE files in the repository root for full details. - */ - -const {danger, warn} = require('danger') -const fs = require('fs') -const path = require('path') - -/** - * Note: if you update the checks in this file, please also update the file ./docs/danger.md - */ - -// Useful to see what we got in danger object -// warn(JSON.stringify(danger)) - -const pr = danger.github.pr -const github = danger.github -// User who has created the PR. -const user = pr.user.login -const modified = danger.git.modified_files -const created = danger.git.created_files -const editedFiles = [...modified, ...created] - -// Check that the PR has a description -if (pr.body.length == 0) { - warn("Please provide a description for this PR.") -} - -// Warn when there is a big PR -if (editedFiles.length > 50) { - message("This pull request seems relatively large. Please consider splitting it into multiple smaller ones.") -} - -// Request a correct title for each PR -if (pr.title.endsWith("…")) { - fail("Please provide a complete title that can be used as a changelog entry.") -} - -// Request a `PR-` label for each PR -if (pr.labels.filter((label) => label.name.startsWith("PR-")).length != 1) { - fail("Please add a `PR-` label to categorise the changelog entry.") -} - -// check that frozen classes have not been modified -const frozenClasses = [ -] - -frozenClasses.forEach(frozen => { - if (editedFiles.some(file => file.endsWith(frozen))) { - fail("Frozen class `" + frozen + "` has been modified. Please do not modify frozen class.") - } - } -) - -const previewAnnotations = [ - 'androidx.compose.ui.tooling.preview.Preview', - 'io.element.android.libraries.designsystem.preview.PreviewWithLargeHeight', - 'io.element.android.libraries.designsystem.preview.PreviewsDayNight' -] - -const filesWithPreviews = editedFiles.filter(file => file.endsWith(".kt")).filter(file => { - const content = fs.readFileSync(file); - return previewAnnotations.some((ann) => content.includes("import " + ann)); -}) - -const composablePreviewProviderContents = fs.readFileSync('tests/uitests/src/test/kotlin/base/ComposablePreviewProvider.kt'); -const packageTreesRegex = /private val PACKAGE_TREES = arrayOf\(([\w\W]+?)\n\)/gm; -const packageTreesMatch = packageTreesRegex.exec(composablePreviewProviderContents)[1]; -const scannedPreviewPackageTrees = packageTreesMatch - .replaceAll("\"", "") - .replaceAll(",", "") - .split('\n').map((line) => line.trim()) - .filter((line) => line.length > 0); - -const previewPackagesNotIncludedInScreenshotTests = filesWithPreviews.map((file) => { - const content = fs.readFileSync(file); - const packageRegex = /package\s+([a-zA-Z0-9.]+)/; - const packageMatch = packageRegex.exec(content); - - if (!packageMatch || packageMatch.length != 2) { - return null; - } - - return packageMatch[1]; - - -}).filter((package) => { - if (!package) { - return false; - } - if (!scannedPreviewPackageTrees.some((prefix) => package.includes(prefix))) { - return true; - } -}); - -if (previewPackagesNotIncludedInScreenshotTests.length > 0) { - const packagesList = previewPackagesNotIncludedInScreenshotTests.map((p) => '- `' + p + '`').join("\n"); - warn("You have made changes to a file containing a `@Preview` annotated function but its package name prefix is not included in the `ComposablePreviewProvider`.\nPackages missing in `tests/uitests/src/test/kotlin/base/ComposablePreviewProvider.kt`: \n" + packagesList); -} - -// Check for pngs on resources -const hasPngs = editedFiles.filter(file => { - file.toLowerCase().endsWith(".png") && !file.includes("snapshots/images/") // Exclude screenshots -}).length > 0 -if (hasPngs) { - warn("You seem to have made changes to some images. Please consider using an vector drawable.") -} - -// Check that translations have not been modified by developers -const translationAllowList = [ - "ElementBot", -] - -if (!translationAllowList.includes(user)) { - if (editedFiles.some(file => file.endsWith("translations.xml"))) { - fail("Some translation files have been edited. Only user `ElementBot` (i.e. translations coming from Localazy) is allowed to do that.\nPlease read more about translations management [in the doc](https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#strings).") - } -}