Skip to content

Commit c9b7069

Browse files
csilverscsilvers
authored
Add labels in a more principled way. (#90)
## Summary: Before, I was just munging the label to the name, so I wouldn't have to change a lot of code. This didn't work because the name was used in places where the code needs it to actually be a name (e.g. when telling github what reviewers to add to the PR). So now I store names and labels separately, and just print the label explicitly when emitting the comment text. This also gives me more control over where the label goes; all in all a more maintainable arrangement. Issue: https://khanacademy.atlassian.net/browse/FEI-5970 ## Test plan: yarn jest Author: csilvers Reviewers: lillialexis, csilvers, MiguelCastillo Required Reviewers: Approved By: lillialexis Checks: ✅ gerald, ✅ lint_and_unit, ✅ autofix, ✅ build_index Pull Request URL: #90
1 parent 4f836bc commit c9b7069

File tree

8 files changed

+175
-133
lines changed

8 files changed

+175
-133
lines changed

Diff for: ‎dist/index.js

+3-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Diff for: ‎setup-files/NOTIFIED

+7
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ Examples:
77
# This rule will notify @owner1 on changes to all files
88
# **/* @owner1
99

10+
# This rule will notify @owner1 on changes to all files, and the rule
11+
# has a label "allfiles", which will be included in the github PR info.
12+
# allfiles: **/* @owner1
13+
1014
# This rule will notify @owner1 and @Org/team1 on changes to all .js files
1115
# **/*.js @owner1 @Org/team1
1216

@@ -37,6 +41,9 @@ Regex Examples:
3741
# This rule will notify @owner1 on changes that include the word "gerald"
3842
# "/gerald/ig" @owner1
3943

44+
# This rule will notify @owner1 on changes that include the word "gerald", and has a label "used_gerald", which will be included in the github PR info.
45+
# used_gerald: "/gerald/ig" @owner1
46+
4047
# This rule will notify @owner1 on changes that *add* the word "gerald"
4148
# "/^\+.*gerald/igm" @owner1
4249

Diff for: ‎setup-files/REVIEWERS

+6
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ Examples:
77
# This rule will request @owner1 for review on changes to all files. This rule will also request @owner2 for a blocking review.
88
# **/* @owner1 @owner2!
99

10+
# This rule will request @owner1 for review on changes to all files, and the rule has a label "allfiles", which will be included in the github PR info.
11+
# allfiles: **/* @owner1
12+
1013
# This rule will request @owner1 and @Org/team1 for review on changes to all .js files
1114
# **/*.js @owner1 @Org/team1
1215

@@ -37,6 +40,9 @@ Regex Examples:
3740
# This rule will request @owner1 for review on changes that include the word "gerald"
3841
# "/gerald/ig" @owner1
3942

43+
# This rule will request @owner1 for review on changes that include the word "gerald", and has a label "used_gerald", which will be included in the github PR info.
44+
# used_gerald: "/gerald/ig" @owner1
45+
4046
# This rule will request @owner1 for review on changes that *add* the word "gerald"
4147
# "/^\+.*gerald/igm" @owner1
4248

Diff for: ‎src/__test__/main.test.js

+10-4
Original file line numberDiff line numberDiff line change
@@ -314,17 +314,23 @@ describe('test that changes to verified commits dont notify people', () => {
314314

315315
describe('test that makeCommitComment makes well formatted strings', () => {
316316
it('should format the commit comment nicely', async () => {
317-
const peopleToFiles = {
318-
'@yipstanley': ['src/runOnPush.js', '.github/workflows/build.yml'],
319-
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
317+
const peopleToLabelToFiles = {
318+
'@yipstanley': {
319+
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
320+
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
321+
},
322+
'@Khan/frontend-infra': {
323+
'': ['src/runOnPush.js', '.geraldignore'],
324+
},
320325
};
321326

322-
await __makeCommitComment(peopleToFiles, 'suite5-commit1');
327+
await __makeCommitComment(peopleToLabelToFiles, 'suite5-commit1');
323328

324329
expect(await getComment('suite5-commit1')).toMatchInlineSnapshot(`
325330
"Notify of Push Without Pull Request
326331
327332
@yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
333+
@yipstanley for changes to \`flow-typed/npm/@octokit/rest_vx.x.x.js\` (typechanges)
328334
@Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
329335
"
330336
`);

Diff for: ‎src/__test__/utils.test.js

+74-52
Original file line numberDiff line numberDiff line change
@@ -63,30 +63,30 @@ describe('maybe add', () => {
6363
let pattern = /test/;
6464
let diffs = {testFile: 'a diff with the word test'};
6565
const name = 'testName';
66-
const bin = {testName: []};
66+
const bin = {testName: {'': []}};
6767
const filesChanged = ['testFile'];
6868

69-
__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
69+
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);
7070

71-
expect(bin).toEqual({testName: ['testFile']});
71+
expect(bin).toEqual({testName: {'': ['testFile']}});
7272

7373
// it shouldn't re-add 'testFile' or another testName key
74-
__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
74+
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);
7575

76-
expect(bin).toEqual({testName: ['testFile']});
76+
expect(bin).toEqual({testName: {'': ['testFile']}});
7777

7878
pattern = /nonexistent/;
7979

80-
__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
80+
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);
8181

82-
expect(bin).toEqual({testName: ['testFile']});
82+
expect(bin).toEqual({testName: {'': ['testFile']}});
8383

8484
pattern = /existent/;
8585
diffs = {otherFile: 'the word existent exists in this diff'};
8686

87-
__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
87+
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);
8888

89-
expect(bin).toEqual({testName: ['testFile']});
89+
expect(bin).toEqual({testName: {'': ['testFile']}});
9090
});
9191
});
9292

@@ -150,12 +150,20 @@ describe('push or set to bin', () => {
150150
const bin = {};
151151
const username = 'testName';
152152
let files = ['file1', 'file2'];
153-
__pushOrSetToBin(bin, username, files);
154-
expect(bin).toEqual({testName: files});
153+
__pushOrSetToBin(bin, username, '', files);
154+
expect(bin).toEqual({testName: {'': files}});
155155

156156
files = ['file2', 'file3', 'file4'];
157-
__pushOrSetToBin(bin, username, files);
158-
expect(bin).toEqual({testName: ['file1', 'file2', 'file3', 'file4']});
157+
__pushOrSetToBin(bin, username, '', files);
158+
expect(bin).toEqual({testName: {'': ['file1', 'file2', 'file3', 'file4']}});
159+
160+
__pushOrSetToBin(bin, username, 'mylabel', files);
161+
expect(bin).toEqual({
162+
testName: {
163+
'': ['file1', 'file2', 'file3', 'file4'],
164+
mylabel: ['file2', 'file3', 'file4'],
165+
},
166+
});
159167
});
160168
});
161169

@@ -191,14 +199,18 @@ mylabel: src/*Push.js @owner`,
191199
expect(
192200
await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'pull_request'),
193201
).toEqual({
194-
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
195-
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
196-
'@testPerson': ['.github/workflows/build.yml'],
202+
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
203+
'@githubUser': {
204+
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
205+
},
206+
'@testPerson': {'': ['.github/workflows/build.yml']},
197207
});
198208

199209
expect(await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'push')).toEqual({
200-
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
201-
'@owner (mylabel)': ['src/runOnPush.js'],
210+
'@owner': {
211+
'': ['src/execCmd.js', 'src/runOnPush.js'],
212+
mylabel: ['src/runOnPush.js'],
213+
},
202214
});
203215
});
204216

@@ -234,15 +246,17 @@ mylabel: src/*Push.js @owner`,
234246
notifiedFile,
235247
),
236248
).toEqual({
237-
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
238-
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
239-
'@testPerson': ['.github/workflows/build.yml'],
249+
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
250+
'@githubUser': {
251+
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
252+
},
253+
'@testPerson': {'': ['.github/workflows/build.yml']},
240254
});
241255

242256
expect(
243257
await getNotified(filesChanged, fileDiffs, {}, '__testUser', 'push', notifiedFile),
244258
).toEqual({
245-
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
259+
'@owner': {'': ['src/execCmd.js', 'src/runOnPush.js']},
246260
});
247261
});
248262
});
@@ -272,11 +286,11 @@ describe('get reviewers', () => {
272286
'yipstanley',
273287
);
274288
expect(reviewers).toEqual({
275-
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
276-
'@testPerson': ['.github/workflows/build.yml'],
289+
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
290+
'@testPerson': {'': ['.github/workflows/build.yml']},
277291
});
278292
expect(requiredReviewers).toEqual({
279-
'@githubUser': ['.github/workflows/build.yml'],
293+
'@githubUser': {'': ['.github/workflows/build.yml']},
280294
});
281295
});
282296

@@ -304,10 +318,10 @@ describe('get reviewers', () => {
304318
'yipstanley',
305319
);
306320
expect(reviewers).toEqual({
307-
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
321+
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
308322
});
309323
expect(requiredReviewers).toEqual({
310-
'@githubUser': ['.github/workflows/build.yml'],
324+
'@githubUser': {'': ['.github/workflows/build.yml']},
311325
});
312326
});
313327

@@ -335,11 +349,11 @@ describe('get reviewers', () => {
335349
'yipstanley',
336350
);
337351
expect(reviewers).toEqual({
338-
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
339-
'@testPerson': ['.github/workflows/build.yml'],
352+
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
353+
'@testPerson': {'': ['.github/workflows/build.yml']},
340354
});
341355
expect(requiredReviewers).toEqual({
342-
'@githubUser': ['.github/workflows/build.yml'],
356+
'@githubUser': {'': ['.github/workflows/build.yml']},
343357
});
344358
});
345359

@@ -365,10 +379,10 @@ describe('get reviewers', () => {
365379
'yipstanley',
366380
);
367381
expect(reviewers).toEqual({
368-
'@testPerson': ['.github/workflows/build.yml'],
382+
'@testPerson': {'': ['.github/workflows/build.yml']},
369383
});
370384
expect(requiredReviewers).toEqual({
371-
'@githubUser': ['.github/workflows/build.yml'],
385+
'@githubUser': {'': ['.github/workflows/build.yml']},
372386
});
373387
});
374388

@@ -394,10 +408,10 @@ describe('get reviewers', () => {
394408
'yipstanley',
395409
);
396410
expect(reviewers).toEqual({
397-
'@testPerson': ['.github/workflows/build.yml'],
411+
'@testPerson': {'': ['.github/workflows/build.yml']},
398412
});
399413
expect(requiredReviewers).toEqual({
400-
'@githubUser': ['.github/workflows/build.yml'],
414+
'@githubUser': {'': ['.github/workflows/build.yml']},
401415
});
402416
});
403417
});
@@ -586,22 +600,28 @@ describe('test that ignore files are parsed correctly', () => {
586600

587601
describe('test that makeCommentBody makes a nicely-formatted string', () => {
588602
it('should format the Gerald pull request comment correctly', async () => {
589-
const peopleToFiles = {
590-
'@yipstanley': [
591-
'src/runOnPush.js',
592-
'.github/workflows/build.yml',
593-
'flow-typed/npm/@octokit/rest_vx.x.x.js',
594-
],
595-
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
603+
const peopleToLabelToFiles = {
604+
'@yipstanley': {
605+
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
606+
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
607+
},
608+
'@Khan/frontend-infra': {
609+
'': ['src/runOnPush.js', '.geraldignore'],
610+
},
596611
};
597612

598-
const result = await makeCommentBody({peopleToFiles, header: 'Reviewers', tagPerson: true});
613+
const result = await makeCommentBody({
614+
peopleToLabelToFiles,
615+
header: 'Reviewers',
616+
tagPerson: true,
617+
});
599618

600619
expect(result).toMatchInlineSnapshot(`
601620
"<details>
602621
<summary><b>Reviewers</b></summary>
603622
604-
* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
623+
* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
624+
* @yipstanley for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
605625
* @Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
606626
</details>
607627
@@ -610,17 +630,18 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
610630
});
611631

612632
it('should comment out reviewers', async () => {
613-
const peopleToFiles = {
614-
'@yipstanley': [
615-
'src/runOnPush.js',
616-
'.github/workflows/build.yml',
617-
'flow-typed/npm/@octokit/rest_vx.x.x.js',
618-
],
619-
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
633+
const peopleToLabelToFiles = {
634+
'@yipstanley': {
635+
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
636+
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
637+
},
638+
'@Khan/frontend-infra': {
639+
'': ['src/runOnPush.js', '.geraldignore'],
640+
},
620641
};
621642

622643
const result = await makeCommentBody({
623-
peopleToFiles,
644+
peopleToLabelToFiles,
624645
header: 'Reviewers',
625646
tagPerson: false,
626647
});
@@ -629,7 +650,8 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
629650
"<details>
630651
<summary><b>Reviewers</b></summary>
631652
632-
* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
653+
* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
654+
* \`@yipstanley\` for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
633655
* \`@Khan/frontend-infra\` for changes to \`src/runOnPush.js\`, \`.geraldignore\`
634656
</details>
635657

Diff for: ‎src/runOnPullRequest.js

+7-6
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
GERALD_COMMENT_REVIEWERS_HEADER,
2222
MATCH_COMMENT_HEADER_REGEX,
2323
} from './constants';
24+
import type {NameToLabelToFiles} from './utils';
2425

2526
/**
2627
* @desc Helper function to update, delete, or create a comment
@@ -30,23 +31,23 @@ import {
3031
*/
3132
const updatePullRequestComment = async (
3233
comment: ?Octokit$IssuesListCommentsResponseItem,
33-
notifyees: {[string]: Array<string>, ...},
34-
reviewers: {[string]: Array<string>, ...},
35-
requiredReviewers: {[string]: Array<string>, ...},
34+
notifyees: NameToLabelToFiles,
35+
reviewers: NameToLabelToFiles,
36+
requiredReviewers: NameToLabelToFiles,
3637
) => {
3738
let body: string = GERALD_COMMENT_HEADER;
3839
body += makeCommentBody({
39-
peopleToFiles: notifyees,
40+
peopleToLabelToFiles: notifyees,
4041
header: GERALD_COMMENT_NOTIFIED_HEADER,
4142
tagPerson: true,
4243
});
4344
body += makeCommentBody({
44-
peopleToFiles: reviewers,
45+
peopleToLabelToFiles: reviewers,
4546
header: GERALD_COMMENT_REVIEWERS_HEADER,
4647
tagPerson: false,
4748
});
4849
body += makeCommentBody({
49-
peopleToFiles: requiredReviewers,
50+
peopleToLabelToFiles: requiredReviewers,
5051
header: GERALD_COMMENT_REQ_REVIEWERS_HEADER,
5152
tagPerson: false,
5253
});

Diff for: ‎src/runOnPush.js

+10-8
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,19 @@ import {getNotified, getFileDiffs, getFileContents} from './utils';
44
import {execCmd} from './execCmd';
55
import {ownerAndRepo, extraPermGithub, type Context} from './setup';
66
import {PUSH, GERALD_COMMIT_COMMENT_HEADER} from './constants';
7+
import type {NameToLabelToFiles} from './utils';
78

8-
const makeCommitComment = async (
9-
peopleToFiles: {[string]: Array<string>, ...},
10-
commitSHA: string,
11-
) => {
12-
const names: string[] = Object.keys(peopleToFiles);
13-
if (peopleToFiles && names.length) {
9+
const makeCommitComment = async (peopleToLabelToFiles: NameToLabelToFiles, commitSHA: string) => {
10+
const names: string[] = Object.keys(peopleToLabelToFiles);
11+
if (peopleToLabelToFiles && names.length) {
1412
let body: string = GERALD_COMMIT_COMMENT_HEADER;
1513
names.forEach((person: string) => {
16-
const files = peopleToFiles[person];
17-
body += `${person} for changes to \`${files.join('`, `')}\`\n`;
14+
const labels: string[] = Object.keys(peopleToLabelToFiles[person]);
15+
labels.forEach((label: string) => {
16+
const files = peopleToLabelToFiles[person][label];
17+
const labelText = label ? ` (${label})` : '';
18+
body += `${person} for changes to \`${files.join('`, `')}\`${labelText}\n`;
19+
});
1820
});
1921

2022
await extraPermGithub.repos.createCommitComment({

0 commit comments

Comments
 (0)