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..56a7c8b97b 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -29,80 +29,50 @@ jobs: - uses: actions/checkout@v3 - name: Run knit run: | - ./gradlew knit + ./gradlew knitCheck - # ktlint for all the modules - ktlint: - name: Kotlin Linter + # Check the project: ktlint, detekt, lint + 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('ktlint-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('ktlint-develop-{0}', github.sha) || format('ktlint-{0}', github.ref) }} + group: ${{ github.ref == 'refs/heads/main' && format('lint-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('lint-develop-{0}', github.sha) || format('lint-{0}', github.ref) }} cancel-in-progress: true steps: - uses: actions/checkout@v3 - name: Run ktlint run: | ./gradlew ktlintCheck --continue + - name: Run detekt + if: always() + run: | + ./gradlew detekt $CI_GRADLE_ARG_PROPERTIES + - name: Run lint + # Not always, if ktlint or detekt fail, avoid running the long lint check. + run: | + ./gradlew lintGplayRelease --stacktrace $CI_GRADLE_ARG_PROPERTIES + ./gradlew lintFdroidRelease --stacktrace $CI_GRADLE_ARG_PROPERTIES - name: Upload reports if: always() uses: actions/upload-artifact@v3 with: - name: ktlinting-report + name: linting-report path: | - */build/reports/ktlint/ktlint*/ktlint*.txt - - name: Handle Results + */build/reports/**/*.* + - name: Prepare Danger if: always() - id: ktlint-results run: | - results="$(cat */*/build/reports/ktlint/ktlint*/ktlint*.txt */build/reports/ktlint/ktlint*/ktlint*.txt | sed -r "s/\x1B\[([0-9]{1,3}(;[0-9]{1,2})?)?[mGK]//g")" - if [ -z "$results" ]; then - echo "::set-output name=add_comment::false" - else - body="👎\`Failed${results}\`" - body="${body//'%'/'%25'}" - body="${body//$'\n'/'%0A'}" - body="${body//$'\r'/'%0D'}" - body="$( echo $body | sed 's/\/home\/runner\/work\/element-android\/element-android\//\`\`/g')" - body="$( echo $body | sed 's/\/src\/main\/java\// 🔸 /g')" - body="$( echo $body | sed 's/im\/vector\/app\///g')" - body="$( echo $body | sed 's/im\/vector\/lib\/attachmentviewer\///g')" - body="$( echo $body | sed 's/im\/vector\/lib\/multipicker\///g')" - body="$( echo $body | sed 's/im\/vector\/lib\///g')" - body="$( echo $body | sed 's/org\/matrix\/android\/sdk\///g')" - body="$( echo $body | sed 's/\/src\/androidTest\/java\// 🔸 /g')" - echo "::set-output name=add_comment::true" - echo "::set-output name=body::$body" - fi - - name: Find Comment - if: always() && github.event_name == 'pull_request' - uses: peter-evans/find-comment@v2 - id: fc + 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: - issue-number: ${{ github.event.pull_request.number }} - comment-author: 'github-actions[bot]' - body-includes: Ktlint Results - - name: Add comment if needed - if: always() && github.event_name == 'pull_request' && steps.ktlint-results.outputs.add_comment == 'true' - uses: peter-evans/create-or-update-comment@v2 - with: - comment-id: ${{ steps.fc.outputs.comment-id }} - issue-number: ${{ github.event.pull_request.number }} - body: | - ### Ktlint Results - - ${{ steps.ktlint-results.outputs.body }} - edit-mode: replace - - name: Delete comment if needed - if: always() && github.event_name == 'pull_request' && steps.fc.outputs.comment-id != '' && steps.ktlint-results.outputs.add_comment == 'false' - uses: actions/github-script@v3 - with: - script: | - github.issues.deleteComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: ${{ steps.fc.outputs.comment-id }} - }) + 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: @@ -122,107 +92,3 @@ jobs: with: 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 }}) - runs-on: ubuntu-latest - if: github.ref != 'refs/heads/main' - strategy: - fail-fast: false - matrix: - target: [ Gplay, Fdroid ] - # Allow all jobs on develop. Just one per PR. - concurrency: - group: ${{ github.ref == 'refs/heads/develop' && format('apk-lint-develop-{0}-{1}', matrix.target, github.sha) || format('apk-lint-{0}-{1}', matrix.target, 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 ${{ matrix.target }} release - run: ./gradlew clean lint${{ matrix.target }}Release --stacktrace $CI_GRADLE_ARG_PROPERTIES - - name: Upload ${{ matrix.target }} linting report - if: always() - uses: actions/upload-artifact@v3 - with: - name: release-lint-report-${{ matrix.target }} - path: | - vector/build/reports/*.* - - detekt: - name: Detekt Analysis - runs-on: ubuntu-latest - # Allow all jobs on main and develop. Just one per PR. - concurrency: - group: ${{ github.ref == 'refs/heads/main' && format('detekt-main-{0}', github.sha) || github.ref == 'refs/heads/develop' && format('detekt-develop-{0}', github.sha) || format('detekt-{0}', github.ref) }} - cancel-in-progress: true - steps: - - uses: actions/checkout@v3 - - name: Run detekt - run: | - ./gradlew detekt $CI_GRADLE_ARG_PROPERTIES - - name: Upload reports - if: always() - uses: actions/upload-artifact@v3 - with: - 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 }} 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/.travis.yml b/.travis.yml deleted file mode 100644 index 6e67639284..0000000000 --- a/.travis.yml +++ /dev/null @@ -1,18 +0,0 @@ -# FTR: Configuration on https://travis-ci.org/github/vector-im/element-android/settings -# -# - Build only if .travis.yml is present -> On -# - Limit concurrent jobs -> Off -# - Build pushed branches -> On (build the branch) -# - Build pushed pull request -> On (build the PR after auto-merge) -# -# - Auto cancel branch builds -> On -# - Auto cancel pull request builds -> On - -sudo: false - -notifications: - email: false - -# Just run a simple script here -script: - - ./tools/travis/check_pr.sh 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 0899392d8f..fb0b3b8b59 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/changelog.d/6637.misc b/changelog.d/6637.misc new file mode 100644 index 0000000000..7fc5ffad98 --- /dev/null +++ b/changelog.d/6637.misc @@ -0,0 +1 @@ +Setup Danger to the project 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.") +} diff --git a/tools/travis/check_pr.sh b/tools/travis/check_pr.sh deleted file mode 100755 index d8d95879e2..0000000000 --- a/tools/travis/check_pr.sh +++ /dev/null @@ -1,45 +0,0 @@ -#!/usr/bin/env bash - -# -# Copyright 2018 New Vector Ltd -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# - -branch=${TRAVIS_BRANCH} - -# echo ${TRAVIS_BRANCH} - -# If not on develop, exit, else we cannot get the list of modified files -# It is ok to check only when on develop branch -if [[ "${branch}" -eq 'develop' ]]; then - echo "Check that a file has been added to /changelog.d" -else - echo "Not on develop branch" - exit 0 -fi - -# git status - -listOfModifiedFiles=`git diff --name-only HEAD ${branch}` - -# echo "List of modified files by this PR:" -# echo ${listOfModifiedFiles} - - -if [[ ${listOfModifiedFiles} = *"changelog.d"* ]]; then - echo "A file has been added to /changelog.d!" -else - echo "❌ Please add a file describing your changes in /changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog" - exit 1 -fi diff --git a/vector/build.gradle b/vector/build.gradle index 8b28246b71..0b7973a148 100644 --- a/vector/build.gradle +++ b/vector/build.gradle @@ -27,6 +27,7 @@ knit { exclude '**/.gradle/**' exclude '**/towncrier/template.md' exclude '**/CHANGES.md' + exclude '/node_modules' } } diff --git a/vector/lint.xml b/vector/lint.xml index d9ca761e7a..b4b6ebc12f 100644 --- a/vector/lint.xml +++ b/vector/lint.xml @@ -102,6 +102,7 @@ + diff --git a/vector/src/main/res/values/strings.xml b/vector/src/main/res/values/strings.xml index 770df1f770..956956b63a 100644 --- a/vector/src/main/res/values/strings.xml +++ b/vector/src/main/res/values/strings.xml @@ -3018,7 +3018,7 @@ Voice Message (%1$s) %1$s, %2$s, %3$s - %1$d minutes %2$d seconds + %1$d minutes %2$d seconds Play %1$s Pause %1$s Unable to play %1$s