Skip to content

Commit 6967a4f

Browse files
hramosfacebook-github-bot
hramos
authored and
facebook-github-bot
committed
Flag code analysis issues once per converter used (facebook#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 facebook#21492 for an example of such a case, where `analysis-bot` left a spammy review at facebook#21492 (review). Pull Request resolved: facebook#21503 Differential Revision: D10219439 Pulled By: hramos fbshipit-source-id: 75d32ef3bfeaa91ab614763a19494659ad1be0dd
1 parent f1d6e22 commit 6967a4f

File tree

4 files changed

+96
-73
lines changed

4 files changed

+96
-73
lines changed

‎.circleci/config.yml‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -591,38 +591,38 @@ jobs:
591591
- run:
592592
name: Analyze Shell Scripts
593593
command: |
594-
if [ -n "$CIRCLE_PR_NUMBER" ]; then
595-
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"
596-
sudo apt-get install -y shellcheck
597-
yarn add @octokit/rest@15.10.0
598-
echo -e "\\x1B[36mAnalyzing shell scripts\\x1B[0m"
599-
GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" ./scripts/circleci/analyze_scripts.sh
600-
else
601-
echo "Skipping shell script analysis."
602-
fi
594+
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"
595+
sudo apt-get install -y shellcheck
596+
yarn add @octokit/rest@15.10.0
597+
echo -e "\\x1B[36mAnalyzing shell scripts\\x1B[0m"; \
598+
GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" \
599+
GITHUB_OWNER="$CIRCLE_PROJECT_USERNAME" \
600+
GITHUB_REPO="$CIRCLE_PROJECT_REPONAME" \
601+
GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" \
602+
./scripts/circleci/analyze_scripts.sh
603603
when: always
604604

605605
- run:
606606
name: Analyze Code
607607
command: |
608-
if [ -n "$CIRCLE_PR_NUMBER" ]; then
609-
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"; yarn add @octokit/rest@15.10.0
610-
echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" ./scripts/circleci/analyze_code.sh
611-
else
612-
echo "Skipping code analysis."
613-
fi
608+
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"; yarn add @octokit/rest@15.10.0
609+
echo -e "\\x1B[36mAnalyzing code\\x1B[0m"; \
610+
GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" \
611+
GITHUB_OWNER="$CIRCLE_PROJECT_USERNAME" \
612+
GITHUB_REPO="$CIRCLE_PROJECT_REPONAME" \
613+
GITHUB_PR_NUMBER="$CIRCLE_PR_NUMBER" \
614+
./scripts/circleci/analyze_code.sh
614615
when: always
615616

616617
- run:
617618
name: Analyze Pull Request
618619
command: |
619-
if [ -n "$CIRCLE_PR_NUMBER" ]; then
620-
cd bots
621-
yarn install --non-interactive --cache-folder ~/.cache/yarn
622-
DANGER_GITHUB_API_TOKEN="$PUBLIC_PULLBOT_GITHUB_TOKEN_A""$PUBLIC_PULLBOT_GITHUB_TOKEN_B" yarn danger
623-
else
624-
echo "Skipping pull request analysis."
625-
fi
620+
echo -e "\\x1B[36mInstalling additional dependencies\\x1B[0m"
621+
cd bots
622+
yarn install --non-interactive --cache-folder ~/.cache/yarn
623+
echo -e "\\x1B[36mAnalyzing pull request\\x1B[0m"; \
624+
DANGER_GITHUB_API_TOKEN="$PUBLIC_PULLBOT_GITHUB_TOKEN_A""$PUBLIC_PULLBOT_GITHUB_TOKEN_B" \
625+
yarn danger
626626
when: always
627627
- save-cache: *save-cache-analysis
628628

@@ -730,6 +730,8 @@ workflows:
730730
tags:
731731
only: /v[0-9]+(\.[0-9]+)*(\-rc(\.[0-9]+)?)?/
732732

733-
# Run code checks
733+
# Run code checks on PRs from forks
734734
- analyze_pr:
735-
filters: *filter-ignore-master-stable
735+
filters:
736+
branches:
737+
only: /^pull\/.*$/

‎scripts/circleci/analyze_code.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ cat <(echo eslint; npm run lint --silent -- --format=json; echo flow; npm run fl
99
# check status
1010
STATUS=$?
1111
if [ $STATUS == 0 ]; then
12-
echo "Code analyzed successfully"
12+
echo "Code analyzed successfully."
1313
else
14-
echo "Code analysis failed, error status $STATUS"
14+
echo "Code analysis failed, error status $STATUS."
1515
fi

‎scripts/circleci/analyze_scripts.sh‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ cat <(echo shellcheck; printf '%s\n' "${results[@]}" | jq .,[] | jq -s . | jq --
1313
# check status
1414
STATUS=$?
1515
if [ $STATUS == 0 ]; then
16-
echo "Shell scripts analyzed successfully"
16+
echo "Shell scripts analyzed successfully."
1717
else
18-
echo "Shell script analysis failed, error status $STATUS"
18+
echo "Shell script analysis failed, error status $STATUS."
1919
fi

‎scripts/circleci/code-analysis-bot.js‎

Lines changed: 66 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,12 @@
99

1010
'use strict';
1111

12-
if (!process.env.CIRCLE_PROJECT_USERNAME) {
13-
console.error('Missing CIRCLE_PROJECT_USERNAME. Example: facebook');
12+
if (!process.env.GITHUB_OWNER) {
13+
console.error('Missing GITHUB_OWNER. Example: facebook');
1414
process.exit(1);
1515
}
16-
if (!process.env.CIRCLE_PROJECT_REPONAME) {
17-
console.error('Missing CIRCLE_PROJECT_REPONAME. Example: react-native');
16+
if (!process.env.GITHUB_REPO) {
17+
console.error('Missing GITHUB_REPO. Example: react-native');
1818
process.exit(1);
1919
}
2020

@@ -162,35 +162,51 @@ function getLineMapFromPatch(patchString) {
162162
return lineMap;
163163
}
164164

165-
function sendReview(owner, repo, number, commit_id, comments, convertersUsed) {
166-
if (comments.length === 0) {
167-
// Do not leave an empty review.
168-
return;
169-
}
170-
171-
let body = '**Code analysis results:**\n\n';
172-
convertersUsed.forEach(converter => {
173-
body += '* `' + converter + '` found some issues.\n';
174-
});
175-
176-
const event = 'REQUEST_CHANGES';
177-
178-
const opts = {
179-
owner,
180-
repo,
181-
number,
182-
commit_id,
183-
body,
184-
event,
185-
comments,
186-
};
165+
function sendReview(owner, repo, number, commit_id, body, comments) {
166+
if (process.env.GITHUB_TOKEN) {
167+
if (comments.length === 0) {
168+
// Do not leave an empty review.
169+
return;
170+
}
187171

188-
octokit.pullRequests.createReview(opts, function(error, res) {
189-
if (error) {
190-
console.error(error);
172+
const event = 'REQUEST_CHANGES';
173+
174+
const opts = {
175+
owner,
176+
repo,
177+
number,
178+
commit_id,
179+
body,
180+
event,
181+
comments,
182+
};
183+
184+
octokit.pullRequests.createReview(opts, function(error, res) {
185+
if (error) {
186+
console.error(error);
187+
return;
188+
}
189+
});
190+
} else {
191+
if (comments.length === 0) {
192+
console.log('No issues found.');
191193
return;
192194
}
193-
});
195+
196+
if (process.env.CIRCLE_CI) {
197+
console.error(
198+
'Code analysis found issues, but the review cannot be posted to GitHub without an access token.',
199+
);
200+
process.exit(1);
201+
}
202+
203+
let results = body + '\n';
204+
comments.forEach(comment => {
205+
results +=
206+
comment.path + ':' + comment.position + ': ' + comment.body + '\n';
207+
});
208+
console.log(results);
209+
}
194210
}
195211

196212
function main(messages, owner, repo, number) {
@@ -199,18 +215,17 @@ function main(messages, owner, repo, number) {
199215
return;
200216
}
201217

202-
if (!process.env.GITHUB_TOKEN) {
203-
console.error(
204-
'Missing GITHUB_TOKEN. Example: 5fd88b964fa214c4be2b144dc5af5d486a2f8c1e',
218+
if (process.env.GITHUB_TOKEN) {
219+
octokit.authenticate({
220+
type: 'oauth',
221+
token: process.env.GITHUB_TOKEN,
222+
});
223+
} else {
224+
console.log(
225+
'Missing GITHUB_TOKEN. Example: 5fd88b964fa214c4be2b144dc5af5d486a2f8c1e. Review feedback with code analysis results will not be provided on GitHub.',
205226
);
206-
process.exit(1);
207227
}
208228

209-
octokit.authenticate({
210-
type: 'oauth',
211-
token: process.env.GITHUB_TOKEN,
212-
});
213-
214229
getShaFromPullRequest(owner, repo, number, sha => {
215230
getFilesFromCommit(owner, repo, sha, files => {
216231
let comments = [];
@@ -234,7 +249,13 @@ function main(messages, owner, repo, number) {
234249
}); // forEach
235250
}); // filter
236251

237-
sendReview(owner, repo, number, sha, comments, convertersUsed);
252+
let body = '**Code analysis results:**\n\n';
253+
const uniqueconvertersUsed = [...new Set(convertersUsed)];
254+
uniqueconvertersUsed.forEach(converter => {
255+
body += '* `' + converter + '` found some issues.\n';
256+
});
257+
258+
sendReview(owner, repo, number, sha, body, comments);
238259
}); // getFilesFromCommit
239260
}); // getShaFromPullRequest
240261
}
@@ -287,16 +308,16 @@ process.stdin.on('end', function() {
287308
delete messages[absolutePath];
288309
}
289310

290-
const owner = process.env.CIRCLE_PROJECT_USERNAME;
291-
const repo = process.env.CIRCLE_PROJECT_REPONAME;
311+
const owner = process.env.GITHUB_OWNER;
312+
const repo = process.env.GITHUB_REPO;
292313

293-
if (!process.env.CIRCLE_PR_NUMBER) {
294-
console.error('Missing CIRCLE_PR_NUMBER. Example: 4687');
314+
if (!process.env.GITHUB_PR_NUMBER) {
315+
console.error('Missing GITHUB_PR_NUMBER. Example: 4687');
295316
// for master branch, don't throw an error
296317
process.exit(0);
297318
}
298319

299-
const number = process.env.CIRCLE_PR_NUMBER;
320+
const number = process.env.GITHUB_PR_NUMBER;
300321

301322
// intentional lint warning to make sure that the bot is working :)
302323
main(messages, owner, repo, number);

0 commit comments

Comments
 (0)