From 2c076cdbdfb81070e663eb104f2338314a4437d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A9ctor=20Ramos?= Date: Wed, 16 Aug 2017 20:55:18 -0700 Subject: [PATCH] Flag large pull requests, add large-pr command Summary: We sometimes get large PRs that sit for a long time without being reviewed. In some cases, the PR touches too many files or lines, and the author would benefit from splitting the PR into smaller easier-to-review PRs. In this PR, we automatically flag such PRs using the existing Danger PR automation functionality. This PR achieves the objective by doing the following: * Add facebook-open-source-bot to `IssueCommands.txt`, allowing it to add labels and close issues/PRs via facebook-github-bot commands. * Adds a `large-pr` command to facebook-github-bot. This command will add a short blurb, apply a new "Large PR" label, and close the PR. * Updates the Dangerfile to check for PRs that touch over 600 lines and/or files. In such cases it will warn and then issue the large-pr command. Additional changes: * Tag core contributor PRs with the "Core Team" label. This is a slight change in policy, in that the label used to be applied after the fact (after the PR was escalated for review). It's more convenient to monitor labels, so we'll start checking for PRs tagged as such and ensure they get escalated as needed. cc skevy Thanks to TheSavior for suggesting this change. Testing against PR #10084 which touches over 600 lines: ``` $ cd danger $ DANGER_GITHUB_API_TOKEN="e622517d9f1136ea8900""07c6373666312cdfaa69" npm run danger pr https://github.com/facebook/react-native/pull/10084 > @ danger /Users/hramos/git/react-native/danger > node ./node_modules/.bin/danger "pr" "https://github.com/facebook/react-native/pull/10084" { fails: [], warnings: [ { message: ":clipboard: Test Plan - This PR appears to be missing a Test Plan." }, { message: ":exclamation: Big PR - This PR is extremely unlikely to get reviewed because it touches 613 lines." } ], messages: [], markdowns: ["facebook-github-bot label Needs more information", "facebook-github-bot large-pr"] } ``` Result: successfully flagged as being a large PR. Closes https://github.com/facebook/react-native/pull/15519 Differential Revision: D5647085 Pulled By: hramos fbshipit-source-id: dfa0f6580b9373085eba856de257a8d2452efbdc --- bots/IssueCommands.txt | 7 +++- danger/dangerfile.js | 72 ++++++++++++++++++++++++++++-------------- docs/Maintainers.md | 8 +++++ 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/bots/IssueCommands.txt b/bots/IssueCommands.txt index 8c9506add..bb2e829f6 100644 --- a/bots/IssueCommands.txt +++ b/bots/IssueCommands.txt @@ -1,4 +1,4 @@ -React Native GitHub Issue Task Force: AndrewJack, astreet, bestander, brentvatne, browniefed, cancan101, charpeni, chirag04, christopherdro, corbt, cosmith, damusnet, DanielMSchmidt, davidaurelio, dmmiller, dsibiski, foghina, frantic, gantman, geirman, grabbou, gre, ide, janicduplessis, javache, jaygarcia, jsierles, kmagiera, knowbody, kmagiera, Kureev, lelandrichardson, martinbigio, melihmucuk, mkonicek, ncuillery, rmevans9, rt2zz, ryankask, satya164, skevy, tabrindle, vjeux +React Native GitHub Issue Task Force: AndrewJack, astreet, bestander, brentvatne, browniefed, cancan101, charpeni, chirag04, christopherdro, corbt, cosmith, damusnet, DanielMSchmidt, davidaurelio, dmmiller, dsibiski, facebook-open-source-bot, foghina, frantic, gantman, geirman, grabbou, gre, ide, janicduplessis, javache, jaygarcia, jsierles, kmagiera, knowbody, kmagiera, Kureev, lelandrichardson, martinbigio, melihmucuk, mkonicek, ncuillery, rmevans9, rt2zz, ryankask, satya164, skevy, tabrindle, vjeux @facebook-github-bot answered comment Closing this issue as {author} says the question asked has been answered. @@ -34,6 +34,11 @@ close comment {author} has closed this issue. If you think it should remain open, let us know why. See ["What to Expect from Maintainers"](https://facebook.github.io/react-native/docs/maintainers.html#handling-issues). close +@facebook-github-bot large-pr +comment Thank you for your contribution. Unfortunately, this pull request seems relatively large.

In order to reduce the load on maintainers, it would be valuable if you could [split this into small, targeted PRs that changed one thing at a time](https://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/).

If doing this requires more than a few pull requests, please open (or update) an issue specifying your goal and the different steps you will take in your PRs. This will ensure maintainers have context on the relationship between the PRs.

We have added the tag large-pr and closed this task. If this is a codemod or other formatting change that is simple but inherently touches many files, please comment on this and let us know and we will reopen the PR. +add-label Large PR +close + @facebook-github-bot bugfix comment Hey {issue_author}, if you're sure this is a bug, can you send a pull request with a fix? add-label Help Wanted diff --git a/danger/dangerfile.js b/danger/dangerfile.js index 1daf482ab..23ad03ff7 100644 --- a/danger/dangerfile.js +++ b/danger/dangerfile.js @@ -13,7 +13,7 @@ const fs = require('fs'); const includes = require('lodash.includes'); const minimatch = require('minimatch'); -import { danger, fail, markdown, warn } from 'danger'; +import { danger, fail, markdown, message, warn } from 'danger'; const isDocsFile = path => includes(path, 'docs/'); const editsDocs = danger.git.modified_files.filter(isDocsFile).length > 0; @@ -21,7 +21,12 @@ const addsDocs = danger.git.created_files.filter(isDocsFile).length > 0; if (addsDocs || editsDocs) { // Note, this does not yet cover edits to the autogenerated docs // (e.g. comments within JS source files) - markdown(':page_facing_up: Thanks for your contribution to the docs!'); + const title = ':page_facing_up: Docs'; + const idea = 'Thanks for your contribution to the docs!'; + message(`${title} - ${idea}`); + + // Add the Documentation label via bot, as @facebook-open-source-bot does not have write privileges on the repo. + markdown('@facebook-github-bot label Documentation'); } const isBlogFile = path => includes(path, 'blog/'); @@ -30,41 +35,42 @@ const isBlogFile = path => includes(path, 'blog/'); // not belong to the Facebook org (on purpose) const addsBlogPost = danger.git.created_files.filter(isBlogFile).length > 0; if (addsBlogPost) { - const message = ':memo: Blog post'; + const title = ':memo: Blog post'; const idea = 'This PR appears to add a new blog post, ' + 'and may require further review from the React Native team.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); } // Flags edits to blog posts const editsBlogPost = danger.git.modified_files.filter(isBlogFile).length > 0; if (editsBlogPost) { - const message = ':memo: Blog post'; + const title = ':memo: Blog post'; const idea = 'This PR appears to edit an existing blog post, ' + 'and may require further review from the React Native team.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); } // Fails if the description is too short. if (danger.github.pr.body.length < 10) { fail(':grey_question: This pull request needs a description.'); + markdown('@facebook-github-bot label Needs more information'); } // Warns if the PR title contains [WIP] const isWIP = includes(danger.github.pr.title, '[WIP]'); if (isWIP) { - const message = ':construction_worker: Work In Progress'; + const title = ':construction_worker: Work In Progress'; const idea = 'This PR appears to be a work in progress, and may not be ready to be merged yet.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); } // Warns if there are changes to package.json, and tags the team. const packageChanged = includes(danger.git.modified_files, 'package.json'); if (packageChanged) { - const message = ':lock: package.json'; + const title = ':lock: package.json'; const idea = 'Changes were made to package.json. ' + 'This will require a manual import by a Facebook employee.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); } // Warns if a test plan is missing. @@ -74,31 +80,51 @@ const includesTestPlan = danger.github.pr.body.toLowerCase().includes('test plan // Warns if a test plan is missing, when editing the Getting Started guide. This page needs to be // tested in all its permutations. if (!includesTestPlan && gettingStartedChanged) { - const message = ':clipboard: Test Plan'; + const title = ':clipboard: Test Plan'; const idea = 'This PR appears to be missing a Test Plan.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); + markdown('@facebook-github-bot label Needs more information'); } // Doc edits rarely require a test plan. We'll trust the reviewer to push back if one is needed. if (!includesTestPlan && !editsDocs) { - const message = ':clipboard: Test Plan'; + const title = ':clipboard: Test Plan'; const idea = 'This PR appears to be missing a Test Plan.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); + markdown('@facebook-github-bot label Needs more information'); } // Tags PRs that have been submitted by a core contributor. +// TODO: Switch to using an actual MAINTAINERS file. const taskforce = fs.readFileSync('../bots/IssueCommands.txt', 'utf8').split('\n')[0].split(':')[1]; const isSubmittedByTaskforce = includes(taskforce, danger.github.pr.user.login); if (isSubmittedByTaskforce) { - markdown('This PR has been submitted by a core contributor.'); + markdown('@facebook-github-bot label Core Team'); +} + +// Tags big PRs +var bigPRThreshold = 600; +if (danger.github.pr.additions + danger.github.pr.deletions > bigPRThreshold) { + const title = ':exclamation: Big PR'; + const idea = `This PR is extremely unlikely to get reviewed because it touches ${danger.github.pr.additions + danger.github.pr.deletions} lines.`; + warn(`${title} - ${idea}`); + + markdown('@facebook-github-bot large-pr'); +} +if (danger.git.modified_files + danger.git.added_files + danger.git.deleted_files > bigPRThreshold) { + const title = ':exclamation: Big PR'; + const idea = `This PR is extremely unlikely to get reviewed because it touches ${danger.git.modified_files + danger.git.added_files + danger.git.deleted_files} files.`; + warn(`${title} - ${idea}`); + + markdown('@facebook-github-bot large-pr'); } // Warns if the bots whitelist file is updated. const issueCommandsFileModified = includes(danger.git.modified_files, 'bots/IssueCommands.txt'); if (issueCommandsFileModified) { - const message = ':exclamation: Bots'; - const idea = 'This PR appears to modify the list of people that may issue commands to the ' + - 'GitHub bot.'; - warn(`${message} - ${idea}`); + const title = ':exclamation: Bots'; + const idea = 'This PR appears to modify the list of people that may issue ' + + 'commands to the GitHub bot.'; + warn(`${title} - ${idea}`); } // Warns if the PR is opened against stable, as commits need to be cherry picked and tagged by a release maintainer. @@ -106,13 +132,13 @@ if (issueCommandsFileModified) { const isMergeRefMaster = danger.github.pr.base.ref === 'master'; const isMergeRefStable = danger.github.pr.base.ref.indexOf(`-stable`) !== -1; if (!isMergeRefMaster && isMergeRefStable) { - const message = ':grey_question: Base Branch'; + const title = ':grey_question: Base Branch'; const idea = 'The base branch for this PR is something other than `master`. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on `master` first and then cherry-pick the commits into the branch for that release. The [Releases Guide](https://github.com/facebook/react-native/blob/master/Releases.md) has more information.'; - warn(`${message} - ${idea}`); + warn(`${title} - ${idea}`); } else if (!isMergeRefMaster && !isMergeRefStable) { - const message = ':exclamation: Base Branch'; + const title = ':exclamation: Base Branch'; const idea = 'The base branch for this PR is something other than `master`. [Are you sure you want to target something other than the `master` branch?](http://facebook.github.io/react-native/docs/contributing.html#pull-requests)'; - fail(`${message} - ${idea}`); + fail(`${title} - ${idea}`); } // People can add themselves to CODEOWNERS in order to be automatically added as reviewers when a file matching a glob pattern is modified. The following will have the bot add a mention in that case. diff --git a/docs/Maintainers.md b/docs/Maintainers.md index 2e56b322b..55847e896 100644 --- a/docs/Maintainers.md +++ b/docs/Maintainers.md @@ -312,4 +312,12 @@ Additionally, the following commands can be used on a pull request: Flag the PR for merging. If used by a core contributor, the bot will attempt to import the pull request. In general, core contributors are those who have consistently submitted high quality contributions to the project. Access control for this command is configured internally in Facebook, outside of the IssueCommands.txt file mentioned above.

+
+

+ @facebook-github-bot large-pr +

+

+ Flag PRs that change too many files at once. These PRs are extremely unlikely to be reviewed. The bot will leave a helpful message indicating next steps such as splitting the PR. The bot will close the PR after adding the "Large PR" label. +

+