From 56691f613e76c947d943f5abe2a7d71dea97fdc3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 29 Jul 2022 10:00:49 +0200 Subject: [PATCH] Setup Danger to Element Android project. --- .github/workflows/danger.yml | 18 ++++++ .github/workflows/quality.yml | 90 ++++++++++++---------------- .gitignore | 5 ++ Gemfile | 1 + Gemfile.lock | 39 ++++++++++++ build.gradle | 5 ++ docs/danger.md | 102 ++++++++++++++++++++++++++++++++ tools/danger/dangerfile-lint.js | 29 +++++++++ tools/danger/dangerfile.js | 95 +++++++++++++++++++++++++++++ 9 files changed, 333 insertions(+), 51 deletions(-) create mode 100644 .github/workflows/danger.yml create mode 100644 docs/danger.md create mode 100644 tools/danger/dangerfile-lint.js create mode 100644 tools/danger/dangerfile.js diff --git a/.github/workflows/danger.yml b/.github/workflows/danger.yml new file mode 100644 index 0000000000..a1d754b4de --- /dev/null +++ b/.github/workflows/danger.yml @@ -0,0 +1,18 @@ +name: Danger CI + +on: [pull_request] + +jobs: + build: + runs-on: ubuntu-latest + name: Danger + steps: + - uses: actions/checkout@v3 + - run: | + npm install --save-dev @babel/plugin-transform-flow-strip-types + - name: Danger + uses: danger/danger-js@11.1.1 + with: + args: "--dangerfile tools/danger/dangerfile.js" + env: + DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }} diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 8bc5efe860..c7c046479e 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -103,6 +103,19 @@ jobs: repo: context.repo.repo, comment_id: ${{ steps.fc.outputs.comment-id }} }) + - 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@11.1.1 + with: + args: "--dangerfile tools/danger/dangerfile-lint.js" + env: + DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }} # Gradle dependency analysis using https://github.com/autonomousapps/dependency-analysis-android-gradle-plugin dependency-analysis: @@ -123,34 +136,6 @@ jobs: name: dependency-analysis path: build/reports/dependency-check-report.html - # Lint for main module - android-lint: - name: Android Linter - runs-on: ubuntu-latest - # Allow all jobs on main and develop. Just one per PR. - concurrency: - group: ${{ github.ref == 'refs/heads/main' && format('android-lint-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('android-lint-develop-{0}', github.sha) || format('android-lint-{0}', github.ref) }} - cancel-in-progress: true - steps: - - uses: actions/checkout@v3 - - uses: actions/cache@v3 - with: - path: | - ~/.gradle/caches - ~/.gradle/wrapper - key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle*', '**/gradle-wrapper.properties') }} - restore-keys: | - ${{ runner.os }}-gradle- - - name: Lint analysis - run: ./gradlew clean :vector:lint --stacktrace $CI_GRADLE_ARG_PROPERTIES - - name: Upload reports - if: always() - uses: actions/upload-artifact@v3 - with: - name: lint-report - path: | - vector/build/reports/*.* - # Lint for Gplay and Fdroid release APK apk-lint: name: Lint APK (${{ matrix.target }}) @@ -183,6 +168,19 @@ jobs: name: release-lint-report-${{ matrix.target }} path: | vector/build/reports/*.* + - 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@11.1.1 + with: + args: "--dangerfile tools/danger/dangerfile-lint.js" + env: + DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }} detekt: name: Detekt Analysis @@ -203,26 +201,16 @@ jobs: name: detekt-report path: | */build/reports/detekt/detekt.html - -# towncrier: -# name: Towncrier check -# runs-on: ubuntu-latest -# if: github.event_name == 'pull_request' && github.head_ref == 'develop' -# steps: -# - uses: actions/checkout@v3 -# - name: Set up Python 3.8 -# uses: actions/setup-python@v4 -# with: -# python-version: 3.8 -# - name: Install towncrier -# run: | -# python3 -m pip install towncrier -# - name: Run towncrier -# # Fetch the pull request' base branch so towncrier will be able to -# # compare the current branch with the base branch. -# # Source: https://github.com/actions/checkout/#fetch-all-branches. -# run: | -# git fetch --no-tags origin +refs/heads/${BASE_BRANCH}:refs/remotes/origin/${BASE_BRANCH} -# towncrier check --compare-with origin/${BASE_BRANCH} -# env: -# BASE_BRANCH: ${{ github.base_ref }} + - 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@11.1.1 + with: + args: "--dangerfile tools/danger/dangerfile-lint.js" + env: + DANGER_GITHUB_API_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }} diff --git a/.gitignore b/.gitignore index 8313fb5c63..f1c0b99b58 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,8 @@ /fastlane/report.xml /**/build + +# Added by yarn +/package.json +/yarn.lock +/node_modules diff --git a/Gemfile b/Gemfile index 7a118b49be..c90138ee44 100644 --- a/Gemfile +++ b/Gemfile @@ -1,3 +1,4 @@ source "https://rubygems.org" gem "fastlane" +gem 'danger' diff --git a/Gemfile.lock b/Gemfile.lock index 345b4c1502..90e846860e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -24,10 +24,29 @@ GEM aws-eventstream (~> 1, >= 1.0.2) babosa (1.0.4) claide (1.0.3) + claide-plugins (0.9.2) + cork + nap + open4 (~> 1.3) colored (1.2) colored2 (3.1.2) commander (4.6.0) highline (~> 2.0.0) + cork (0.3.0) + colored2 (~> 3.1) + danger (8.6.1) + claide (~> 1.0) + claide-plugins (>= 0.9.2) + colored2 (~> 3.1) + cork (~> 0.1) + faraday (>= 0.9.0, < 2.0) + faraday-http-cache (~> 2.0) + git (~> 1.7) + kramdown (~> 2.3) + kramdown-parser-gfm (~> 1.0) + no_proxy_fix + octokit (~> 4.7) + terminal-table (>= 1, < 4) declarative (0.0.20) digest-crc (0.6.3) rake (>= 12.0.0, < 14.0.0) @@ -52,6 +71,8 @@ GEM faraday-em_http (1.0.0) faraday-em_synchrony (1.0.0) faraday-excon (1.1.0) + faraday-http-cache (2.4.0) + faraday (>= 0.8) faraday-httpclient (1.0.1) faraday-net_http (1.0.1) faraday-net_http_persistent (1.2.0) @@ -98,6 +119,8 @@ GEM xcpretty (~> 0.3.0) xcpretty-travis-formatter (>= 0.0.3) gh_inspector (1.1.3) + git (1.11.0) + rchardet (~> 1.8) google-apis-androidpublisher_v3 (0.8.0) google-apis-core (>= 0.4, < 2.a) google-apis-core (0.4.0) @@ -143,17 +166,28 @@ GEM jmespath (1.4.0) json (2.5.1) jwt (2.2.3) + kramdown (2.4.0) + rexml + kramdown-parser-gfm (1.1.0) + kramdown (~> 2.0) memoist (0.16.2) mini_magick (4.11.0) mini_mime (1.1.0) multi_json (1.15.0) multipart-post (2.0.0) nanaimo (0.3.0) + nap (1.1.0) naturally (2.2.1) + no_proxy_fix (0.1.2) + octokit (4.25.1) + faraday (>= 1, < 3) + sawyer (~> 0.9) + open4 (1.3.4) os (1.1.1) plist (3.6.0) public_suffix (4.0.6) rake (13.0.6) + rchardet (1.8.0) representable (3.1.1) declarative (< 0.1.0) trailblazer-option (>= 0.1.1, < 0.2.0) @@ -163,6 +197,9 @@ GEM rouge (2.0.7) ruby2_keywords (0.0.5) rubyzip (2.3.2) + sawyer (0.9.2) + addressable (>= 2.3.5) + faraday (>= 0.17.3, < 3) security (0.1.3) signet (0.15.0) addressable (~> 2.3) @@ -200,9 +237,11 @@ GEM xcpretty (~> 0.2, >= 0.0.7) PLATFORMS + universal-darwin-21 x86_64-darwin-20 DEPENDENCIES + danger fastlane BUNDLED WITH diff --git a/build.gradle b/build.gradle index e8472097d5..e3357958cd 100644 --- a/build.gradle +++ b/build.gradle @@ -126,6 +126,11 @@ allprojects { enableExperimentalRules = true // display the corresponding rule verbose = true + reporters { + reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.PLAIN) + // To have XML report for Danger + reporter(org.jlleitschuh.gradle.ktlint.reporter.ReporterType.CHECKSTYLE) + } disabledRules = [ // TODO Re-enable these 4 rules after reformatting project "indent", diff --git a/docs/danger.md b/docs/danger.md new file mode 100644 index 0000000000..19728f00e9 --- /dev/null +++ b/docs/danger.md @@ -0,0 +1,102 @@ +## 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 file for towncrier and extension is checked +- PR contains a Sign-Off, with exception for Element employee contributors +- PR with change on layout should include screenshot in the description +- PR which adds png file warn about the usage of vector drawables +- non draft PR should have a reviewer + +### 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/vector-im/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/vector-im/element-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. + +## 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 new file mode 100644 index 0000000000..b0531fef9b --- /dev/null +++ b/tools/danger/dangerfile-lint.js @@ -0,0 +1,29 @@ +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?: "" +})) diff --git a/tools/danger/dangerfile.js b/tools/danger/dangerfile.js new file mode 100644 index 0000000000..407cedc34b --- /dev/null +++ b/tools/danger/dangerfile.js @@ -0,0 +1,95 @@ +const {danger, warn} = require('danger') + +/** + * 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 modified = danger.git.modified_files +const created = danger.git.created_files +let 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) { + warn("This pull request seems relatively large. Please consider splitting it into multiple smaller ones.") +} + +// Request a changelog for each PR +let changelogFiles = editedFiles.filter(file => file.startsWith("changelog.d/")) + +if (changelogFiles.length == 0) { + warn("Please add a changelog. See instructions [here](https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog)") +} else { + const validTowncrierExtensions = [ + "bugfix", + "doc", + "feature", + "misc", + "sdk", + "wip", + ] + if (!changelogFiles.every(file => validTowncrierExtensions.includes(file.split(".").pop()))) { + fail("Invalid extension for changelog. See instructions [here](https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog)") + } +} + +// Check for a sign-off +const signOff = "Signed-off-by:" + +// Please add new names following the alphabetical order. +const allowList = [ + "aringenbach", + "BillCarsonFr", + "bmarty", + "Claire1817", + "ericdecanini", + "fedrunov", + "Florian14", + "ganfra", + "jmartinesp", + "langleyd", + "MadLittleMods", + "manuroe", + "mnaturel", + "onurays", + "ouchadam", + "stefanceriu", + "yostyle", +] + +let requiresSignOff = !allowList.includes(pr.user.login) + +if (requiresSignOff) { + let hasPRBodySignOff = pr.body.includes(signOff) + let hasCommitSignOff = danger.git.commits.every(commit => commit.message.includes(signOff)) + if (!hasPRBodySignOff && !hasCommitSignOff) { + fail("Please add a sign-off to either the PR description or to the commits themselves.") + } +} + +// Check for screenshots on view changes +let hasChangedViews = editedFiles.filter(file => file.includes("/layout")).length > 0 +if (hasChangedViews) { + if (!pr.body.includes("user-images")) { + warn("You seem to have made changes to views. Please consider adding screenshots.") + } +} + +// Check for pngs on resources +let hasPngs = editedFiles.filter(file => file.toLowerCase().endsWith(".png")).length > 0 +if (hasPngs) { + warn("You seem to have made changes to some images. Please consider using an vector drawable.") +} + +// Check for reviewers +if (pr.requested_reviewers.length == 0 && !pr.draft) { + fail("Please add a reviewer to your PR.") +}