From 538936df9c4d70590747f9d1c9e11965b178a787 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 13 Dec 2024 16:10:21 +0100 Subject: [PATCH 1/3] We do not require Sign-off anymore, but use CLA instead. --- .github/pull_request_template.md | 1 - docs/danger.md | 1 - docs/pull_request.md | 2 +- tools/danger/dangerfile.js | 66 -------------------------------- 4 files changed, 1 insertion(+), 69 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index ea44e3a5d0..4acb89bf11 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -50,5 +50,4 @@ Uncomment this markdown table below and edit the last line `|||`: - [ ] Pull request is based on the develop branch - [ ] Pull request title will be used in the release note, it clearly define what will change for the user - [ ] Pull request includes screenshots or videos if containing UI changes -- [ ] Pull request includes a [sign off](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#sign-off) - [ ] You've made a self review of your PR diff --git a/docs/danger.md b/docs/danger.md index 536732a0b0..9adb382a3a 100644 --- a/docs/danger.md +++ b/docs/danger.md @@ -24,7 +24,6 @@ Here are the checks that Danger does so far: - 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 contains a Sign-Off, with exception for Element employee contributors - 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 diff --git a/docs/pull_request.md b/docs/pull_request.md index 7ff71a30e7..97314b2a73 100644 --- a/docs/pull_request.md +++ b/docs/pull_request.md @@ -173,7 +173,7 @@ But comment in PR from the community are always appreciated! 4. Fork consideration. Will configuration of forks be easy? Some documentation may help in some cases. 5. We are building long term products. "Quick and dirty" code must be avoided. 6. The PR includes new tests for the added code, updated test for the existing code -7. All PRs from external contributors **MUST** include a sign-off. It's in the checklist, and sometimes it's checked by the submitter, but there is actually no sign-off. In this case, ask nicely for a sign-off and request changes (do not approve the PR, even if everything else is fine). +7. All commit authors must have signed the CLA. Please open https://cla-assistant.io/element-hq/element-x-android to agree to the CLA. ### Rules diff --git a/tools/danger/dangerfile.js b/tools/danger/dangerfile.js index 377f943e50..be723372e4 100644 --- a/tools/danger/dangerfile.js +++ b/tools/danger/dangerfile.js @@ -48,72 +48,6 @@ frozenClasses.forEach(frozen => { } ) -// Check for a sign-off -const signOff = "Signed-off-by:" - -// Please add new names following the alphabetical order. -const allowList = [ - "aringenbach", - "BillCarsonFr", - "bmarty", - "csmith", - "dependabot[bot]", - "Florian14", - "ganfra", - "github-actions[bot]", - "jmartinesp", - "jonnyandrew", - "julioromano", - "kittykat", - "langleyd", - "MadLittleMods", - "manuroe", - "renovate[bot]", - "stefanceriu", - "yostyle", -] - -function signoff_needed(reason) { - message("Sign-off required, " + reason) - const hasPRBodySignOff = pr.body.includes(signOff) - const 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. See instructions [here](https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#sign-off).") - } -} - -function signoff_unneeded(reason) { - message("Sign-off not required, " + reason) -} - -// Somewhat awkward phrasing, dangerfile is not in an async context. -if (allowList.includes(user)) { - signoff_unneeded("allow-list") -} else { -// github.api.rest.orgs.checkMembershipForUser({ -// org: "element-hq", -// username: user, -// }).then((result) => { - github.api.rest.teams.getMembershipForUserInOrg({ - org: "element-hq", - team_slug: "vector-core", - username: user, - }).then((result) => { - if (result.status == 204 || result.status == 200) { - signoff_unneeded("team-member") - } - else { - signoff_needed("not-team-member") - } - }).catch((error) => { - if (error.response.status == 404) { - signoff_needed("not-team-member"); - } else { - console.log(error); signoff_needed("error") - } - }) -} - const previewAnnotations = [ 'androidx.compose.ui.tooling.preview.Preview', 'io.element.android.libraries.designsystem.preview.PreviewWithLargeHeight', From e60efc68f8575ecb916a2a0f94a64205112b07ba Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 13 Dec 2024 16:10:53 +0100 Subject: [PATCH 2/3] We are now min API 24 --- .github/pull_request_template.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 4acb89bf11..ed5d45c62c 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -44,7 +44,7 @@ Uncomment this markdown table below and edit the last line `|||`: -- [ ] Changes have been tested on an Android device or Android emulator with API 23 +- [ ] Changes have been tested on an Android device or Android emulator with API 24 - [ ] UI change has been tested on both light and dark themes - [ ] Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility - [ ] Pull request is based on the develop branch From d740dcad0d75f80e4a65b71b831f379febf1ccf3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 13 Dec 2024 16:15:31 +0100 Subject: [PATCH 3/3] Update danger check on translations. --- tools/danger/dangerfile.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tools/danger/dangerfile.js b/tools/danger/dangerfile.js index be723372e4..1d5945ec15 100644 --- a/tools/danger/dangerfile.js +++ b/tools/danger/dangerfile.js @@ -104,17 +104,11 @@ if (hasPngs) { // Check that translations have not been modified by developers const translationAllowList = [ - "RiotTranslateBot", - "github-actions[bot]", + "ElementBot", ] if (!translationAllowList.includes(user)) { - if (editedFiles.some(file => file.endsWith("strings.xml") && !file.endsWith("values/strings.xml"))) { - fail("Some translation files have been edited. Only user `RiotTranslateBot` (i.e. translations coming from Weblate) or `github-actions[bot]` (i.e. translations coming from automation) are allowed to do that.\nPlease read more about translations management [in the doc](https://github.com/element-hq/element-android/blob/develop/CONTRIBUTING.md#internationalisation).") - } - - // Check that new strings are not added to `values/strings.xml` - if (editedFiles.some(file => file.endsWith("ui-strings/src/main/res/values/strings.xml"))) { - fail("`ui-strings/src/main/res/values/strings.xml` has been edited. This file will be overridden in the next strings synchronisation. Please add new strings in the file `values/strings_eax.xml` instead.") + 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).") } }