From 6bdce91c2e84d4255ba4141ee32d6ba669c576ce Mon Sep 17 00:00:00 2001 From: Hector Ramos Date: Thu, 4 Oct 2018 21:17:00 -0700 Subject: [PATCH 1/3] Flag code analysis issues once per converter used 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. --- scripts/circleci/code-analysis-bot.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/circleci/code-analysis-bot.js b/scripts/circleci/code-analysis-bot.js index 0e5a3d5be70a..abddb2133609 100644 --- a/scripts/circleci/code-analysis-bot.js +++ b/scripts/circleci/code-analysis-bot.js @@ -169,7 +169,8 @@ function sendReview(owner, repo, number, commit_id, comments, convertersUsed) { } let body = '**Code analysis results:**\n\n'; - convertersUsed.forEach(converter => { + const uniqueconvertersUsed = [...new Set(convertersUsed)]; + uniqueconvertersUsed.forEach(converter => { body += '* `' + converter + '` found some issues.\n'; }); From 9fdbead5d5fcbfbafe2ec77d91681d7962273c5c Mon Sep 17 00:00:00 2001 From: Hector Ramos Date: Fri, 5 Oct 2018 09:41:55 -0700 Subject: [PATCH 2/3] Ensure analyze_pr runs only on PRs from forks --- .circleci/config.yml | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6a19d4d7ab01..17baf9a2624c 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -591,38 +591,26 @@ 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" ./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" ./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 + 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 when: always - save-cache: *save-cache-analysis @@ -730,6 +718,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\/.*$/ From 7a7f72c86ce1919737a348a521f29770365d0ef0 Mon Sep 17 00:00:00 2001 From: Hector Ramos Date: Fri, 5 Oct 2018 10:50:02 -0700 Subject: [PATCH 3/3] Print out review feedback locally for test purposes --- .circleci/config.yml | 20 ++++- scripts/circleci/analyze_code.sh | 4 +- scripts/circleci/analyze_scripts.sh | 4 +- scripts/circleci/code-analysis-bot.js | 112 +++++++++++++++----------- 4 files changed, 86 insertions(+), 54 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 17baf9a2624c..682a6732414f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -594,23 +594,35 @@ jobs: 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 + 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: | 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 + 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: | + echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m" 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 + 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 diff --git a/scripts/circleci/analyze_code.sh b/scripts/circleci/analyze_code.sh index a2953e45c574..ef55d484e91a 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 730f00f5a30c..052688cab9ba 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 abddb2133609..86f21ab69f52 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,36 +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'; - const uniqueconvertersUsed = [...new Set(convertersUsed)]; - uniqueconvertersUsed.forEach(converter => { - body += '* `' + converter + '` found some issues.\n'; - }); - - const event = 'REQUEST_CHANGES'; - - const opts = { - owner, - repo, - number, - commit_id, - body, - event, - comments, - }; +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; + } - octokit.pullRequests.createReview(opts, function(error, res) { - if (error) { - console.error(error); + 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) { @@ -200,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 = []; @@ -235,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 } @@ -288,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);