From 6967a4fa567f55459e8b43acffbaa6404856da03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A9ctor=20Ramos?= Date: Fri, 5 Oct 2018 11:26:14 -0700 Subject: [PATCH] Flag code analysis issues once per converter used (#21503) Summary: Fixes issue where the bot would leave multiple lines in the top review comment if, say, eslint found 30 issues, there would be 30 mentions of eslint having found issues. See https://github.com/facebook/react-native/pull/21492 for an example of such a case, where `analysis-bot` left a spammy review at https://github.com/facebook/react-native/pull/21492#pullrequestreview-161837975. Pull Request resolved: https://github.com/facebook/react-native/pull/21503 Differential Revision: D10219439 Pulled By: hramos fbshipit-source-id: 75d32ef3bfeaa91ab614763a19494659ad1be0dd --- .circleci/config.yml | 50 ++++++------ scripts/circleci/analyze_code.sh | 4 +- scripts/circleci/analyze_scripts.sh | 4 +- scripts/circleci/code-analysis-bot.js | 113 +++++++++++++++----------- 4 files changed, 97 insertions(+), 74 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6a19d4d7ab0..682a6732414 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -591,38 +591,38 @@ jobs: - run: name: Analyze Shell Scripts command: | - if [ -n "$CIRCLE_PR_NUMBER" ]; then - echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m" - sudo apt-get install -y shellcheck - yarn add @octokit/rest@15.10.0 - echo -e "\\x1B[36mAnalyzing shell scripts\\x1B[0m" - GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" ./scripts/circleci/analyze_scripts.sh - else - echo "Skipping shell script analysis." - fi + echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m" + sudo apt-get install -y shellcheck + yarn add @octokit/rest@15.10.0 + echo -e "\\x1B[36mAnalyzing shell scripts\\x1B[0m"; \ + GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" \ + GITHUB_OWNER="$CIRCLE_PROJECT_USERNAME" \ + GITHUB_REPO="$CIRCLE_PROJECT_REPONAME" \ + GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" \ + ./scripts/circleci/analyze_scripts.sh when: always - run: name: Analyze Code command: | - if [ -n "$CIRCLE_PR_NUMBER" ]; then - echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"; yarn add @octokit/rest@15.10.0 - echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" ./scripts/circleci/analyze_code.sh - else - echo "Skipping code analysis." - fi + echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"; yarn add @octokit/rest@15.10.0 + echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; \ + GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" \ + GITHUB_OWNER="$CIRCLE_PROJECT_USERNAME" \ + GITHUB_REPO="$CIRCLE_PROJECT_REPONAME" \ + GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" \ + ./scripts/circleci/analyze_code.sh when: always - run: name: Analyze Pull Request command: | - if [ -n "$CIRCLE_PR_NUMBER" ]; then - cd bots - yarn install --non-interactive --cache-folder ~/.cache/yarn - DANGER_GITHUB_API_TOKEN="$PUBLIC_PULLBOT_GITHUB_TOKEN_A""$PUBLIC_PULLBOT_GITHUB_TOKEN_B" yarn danger - else - echo "Skipping pull request analysis." - fi + echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m" + cd bots + yarn install --non-interactive --cache-folder ~/.cache/yarn + echo -e "\\x1B[36mAnalyzing pull request\\x1B[0m"; \ + DANGER_GITHUB_API_TOKEN="$PUBLIC_PULLBOT_GITHUB_TOKEN_A""$PUBLIC_PULLBOT_GITHUB_TOKEN_B" \ + yarn danger when: always - save-cache: *save-cache-analysis @@ -730,6 +730,8 @@ workflows: tags: only: /v[0-9]+(\.[0-9]+)*(\-rc(\.[0-9]+)?)?/ - # Run code checks + # Run code checks on PRs from forks - analyze_pr: - filters: *filter-ignore-master-stable + filters: + branches: + only: /^pull\/.*$/ diff --git a/scripts/circleci/analyze_code.sh b/scripts/circleci/analyze_code.sh index a2953e45c57..ef55d484e91 100755 --- a/scripts/circleci/analyze_code.sh +++ b/scripts/circleci/analyze_code.sh @@ -9,7 +9,7 @@ cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run fl # check status STATUS=$? if [ $STATUS == 0 ]; then - echo "Code analyzed successfully" + echo "Code analyzed successfully." else - echo "Code analysis failed, error status $STATUS" + echo "Code analysis failed, error status $STATUS." fi diff --git a/scripts/circleci/analyze_scripts.sh b/scripts/circleci/analyze_scripts.sh index 730f00f5a30..052688cab9b 100755 --- a/scripts/circleci/analyze_scripts.sh +++ b/scripts/circleci/analyze_scripts.sh @@ -13,7 +13,7 @@ cat <(echo shellcheck; printf '%s\n' "${results[@]}" | jq .,[] | jq -s . | jq -- # check status STATUS=$? if [ $STATUS == 0 ]; then - echo "Shell scripts analyzed successfully" + echo "Shell scripts analyzed successfully." else - echo "Shell script analysis failed, error status $STATUS" + echo "Shell script analysis failed, error status $STATUS." fi diff --git a/scripts/circleci/code-analysis-bot.js b/scripts/circleci/code-analysis-bot.js index 0e5a3d5be70..86f21ab69f5 100644 --- a/scripts/circleci/code-analysis-bot.js +++ b/scripts/circleci/code-analysis-bot.js @@ -9,12 +9,12 @@ 'use strict'; -if (!process.env.CIRCLE_PROJECT_USERNAME) { - console.error('Missing CIRCLE_PROJECT_USERNAME. Example: facebook'); +if (!process.env.GITHUB_OWNER) { + console.error('Missing GITHUB_OWNER. Example: facebook'); process.exit(1); } -if (!process.env.CIRCLE_PROJECT_REPONAME) { - console.error('Missing CIRCLE_PROJECT_REPONAME. Example: react-native'); +if (!process.env.GITHUB_REPO) { + console.error('Missing GITHUB_REPO. Example: react-native'); process.exit(1); } @@ -162,35 +162,51 @@ function getLineMapFromPatch(patchString) { return lineMap; } -function sendReview(owner, repo, number, commit_id, comments, convertersUsed) { - if (comments.length === 0) { - // Do not leave an empty review. - return; - } - - let body = '**Code analysis results:**\n\n'; - convertersUsed.forEach(converter => { - body += '* `' + converter + '` found some issues.\n'; - }); - - const event = 'REQUEST_CHANGES'; - - const opts = { - owner, - repo, - number, - commit_id, - body, - event, - comments, - }; - - octokit.pullRequests.createReview(opts, function(error, res) { - if (error) { - console.error(error); +function sendReview(owner, repo, number, commit_id, body, comments) { + if (process.env.GITHUB_TOKEN) { + if (comments.length === 0) { + // Do not leave an empty review. return; } - }); + + const event = 'REQUEST_CHANGES'; + + const opts = { + owner, + repo, + number, + commit_id, + body, + event, + comments, + }; + + octokit.pullRequests.createReview(opts, function(error, res) { + if (error) { + console.error(error); + return; + } + }); + } else { + if (comments.length === 0) { + console.log('No issues found.'); + return; + } + + if (process.env.CIRCLE_CI) { + console.error( + 'Code analysis found issues, but the review cannot be posted to GitHub without an access token.', + ); + process.exit(1); + } + + let results = body + '\n'; + comments.forEach(comment => { + results += + comment.path + ':' + comment.position + ': ' + comment.body + '\n'; + }); + console.log(results); + } } function main(messages, owner, repo, number) { @@ -199,18 +215,17 @@ function main(messages, owner, repo, number) { return; } - if (!process.env.GITHUB_TOKEN) { - console.error( - 'Missing GITHUB_TOKEN. Example: 5fd88b964fa214c4be2b144dc5af5d486a2f8c1e', + if (process.env.GITHUB_TOKEN) { + octokit.authenticate({ + type: 'oauth', + token: process.env.GITHUB_TOKEN, + }); + } else { + console.log( + 'Missing GITHUB_TOKEN. Example: 5fd88b964fa214c4be2b144dc5af5d486a2f8c1e. Review feedback with code analysis results will not be provided on GitHub.', ); - process.exit(1); } - octokit.authenticate({ - type: 'oauth', - token: process.env.GITHUB_TOKEN, - }); - getShaFromPullRequest(owner, repo, number, sha => { getFilesFromCommit(owner, repo, sha, files => { let comments = []; @@ -234,7 +249,13 @@ function main(messages, owner, repo, number) { }); // forEach }); // filter - sendReview(owner, repo, number, sha, comments, convertersUsed); + let body = '**Code analysis results:**\n\n'; + const uniqueconvertersUsed = [...new Set(convertersUsed)]; + uniqueconvertersUsed.forEach(converter => { + body += '* `' + converter + '` found some issues.\n'; + }); + + sendReview(owner, repo, number, sha, body, comments); }); // getFilesFromCommit }); // getShaFromPullRequest } @@ -287,16 +308,16 @@ process.stdin.on('end', function() { delete messages[absolutePath]; } - const owner = process.env.CIRCLE_PROJECT_USERNAME; - const repo = process.env.CIRCLE_PROJECT_REPONAME; + const owner = process.env.GITHUB_OWNER; + const repo = process.env.GITHUB_REPO; - if (!process.env.CIRCLE_PR_NUMBER) { - console.error('Missing CIRCLE_PR_NUMBER. Example: 4687'); + if (!process.env.GITHUB_PR_NUMBER) { + console.error('Missing GITHUB_PR_NUMBER. Example: 4687'); // for master branch, don't throw an error process.exit(0); } - const number = process.env.CIRCLE_PR_NUMBER; + const number = process.env.GITHUB_PR_NUMBER; // intentional lint warning to make sure that the bot is working :) main(messages, owner, repo, number);