Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add labels in a more principled way. #90

Merged
merged 4 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions dist/index.js

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions setup-files/NOTIFIED
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ Examples:
# This rule will notify @owner1 on changes to all files
# **/* @owner1

# This rule will notify @owner1 on changes to all files, and the rule
# has a label "allfiles", which will be included in the github PR info.
# allfiles: **/* @owner1

# This rule will notify @owner1 and @Org/team1 on changes to all .js files
# **/*.js @owner1 @Org/team1

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

# 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.
# used_gerald: "/gerald/ig" @owner1

# This rule will notify @owner1 on changes that *add* the word "gerald"
# "/^\+.*gerald/igm" @owner1

Expand Down
6 changes: 6 additions & 0 deletions setup-files/REVIEWERS
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ Examples:
# This rule will request @owner1 for review on changes to all files. This rule will also request @owner2 for a blocking review.
# **/* @owner1 @owner2!

# 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.
# allfiles: **/* @owner1

# This rule will request @owner1 and @Org/team1 for review on changes to all .js files
# **/*.js @owner1 @Org/team1

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

# 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.
# used_gerald: "/gerald/ig" @owner1

# This rule will request @owner1 for review on changes that *add* the word "gerald"
# "/^\+.*gerald/igm" @owner1

Expand Down
14 changes: 10 additions & 4 deletions src/__test__/main.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,23 @@ describe('test that changes to verified commits dont notify people', () => {

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

await __makeCommitComment(peopleToFiles, 'suite5-commit1');
await __makeCommitComment(peopleToLabelToFiles, 'suite5-commit1');

expect(await getComment('suite5-commit1')).toMatchInlineSnapshot(`
"Notify of Push Without Pull Request

@yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
@yipstanley for changes to \`flow-typed/npm/@octokit/rest_vx.x.x.js\` (typechanges)
@Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
"
`);
Expand Down
126 changes: 74 additions & 52 deletions src/__test__/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,30 +63,30 @@ describe('maybe add', () => {
let pattern = /test/;
let diffs = {testFile: 'a diff with the word test'};
const name = 'testName';
const bin = {testName: []};
const bin = {testName: {'': []}};
const filesChanged = ['testFile'];

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

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

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

pattern = /nonexistent/;

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

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

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});
});
});

Expand Down Expand Up @@ -150,12 +150,20 @@ describe('push or set to bin', () => {
const bin = {};
const username = 'testName';
let files = ['file1', 'file2'];
__pushOrSetToBin(bin, username, files);
expect(bin).toEqual({testName: files});
__pushOrSetToBin(bin, username, '', files);
expect(bin).toEqual({testName: {'': files}});

files = ['file2', 'file3', 'file4'];
__pushOrSetToBin(bin, username, files);
expect(bin).toEqual({testName: ['file1', 'file2', 'file3', 'file4']});
__pushOrSetToBin(bin, username, '', files);
expect(bin).toEqual({testName: {'': ['file1', 'file2', 'file3', 'file4']}});

__pushOrSetToBin(bin, username, 'mylabel', files);
expect(bin).toEqual({
testName: {
'': ['file1', 'file2', 'file3', 'file4'],
mylabel: ['file2', 'file3', 'file4'],
},
});
});
});

Expand Down Expand Up @@ -191,14 +199,18 @@ mylabel: src/*Push.js @owner`,
expect(
await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'pull_request'),
).toEqual({
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@githubUser': {
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
},
'@testPerson': {'': ['.github/workflows/build.yml']},
});

expect(await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'push')).toEqual({
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
'@owner (mylabel)': ['src/runOnPush.js'],
'@owner': {
'': ['src/execCmd.js', 'src/runOnPush.js'],
mylabel: ['src/runOnPush.js'],
},
});
});

Expand Down Expand Up @@ -234,15 +246,17 @@ mylabel: src/*Push.js @owner`,
notifiedFile,
),
).toEqual({
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@githubUser': {
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
},
'@testPerson': {'': ['.github/workflows/build.yml']},
});

expect(
await getNotified(filesChanged, fileDiffs, {}, '__testUser', 'push', notifiedFile),
).toEqual({
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
'@owner': {'': ['src/execCmd.js', 'src/runOnPush.js']},
});
});
});
Expand Down Expand Up @@ -272,11 +286,11 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand Down Expand Up @@ -304,10 +318,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand Down Expand Up @@ -335,11 +349,11 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand All @@ -365,10 +379,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@testPerson': ['.github/workflows/build.yml'],
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand All @@ -394,10 +408,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@testPerson': ['.github/workflows/build.yml'],
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});
});
Expand Down Expand Up @@ -586,22 +600,28 @@ describe('test that ignore files are parsed correctly', () => {

describe('test that makeCommentBody makes a nicely-formatted string', () => {
it('should format the Gerald pull request comment correctly', async () => {
const peopleToFiles = {
'@yipstanley': [
'src/runOnPush.js',
'.github/workflows/build.yml',
'flow-typed/npm/@octokit/rest_vx.x.x.js',
],
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
const peopleToLabelToFiles = {
'@yipstanley': {
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
},
'@Khan/frontend-infra': {
'': ['src/runOnPush.js', '.geraldignore'],
},
};

const result = await makeCommentBody({peopleToFiles, header: 'Reviewers', tagPerson: true});
const result = await makeCommentBody({
peopleToLabelToFiles,
header: 'Reviewers',
tagPerson: true,
});

expect(result).toMatchInlineSnapshot(`
"<details>
<summary><b>Reviewers</b></summary>

* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
* @yipstanley for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
* @Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
</details>

Expand All @@ -610,17 +630,18 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
});

it('should comment out reviewers', async () => {
const peopleToFiles = {
'@yipstanley': [
'src/runOnPush.js',
'.github/workflows/build.yml',
'flow-typed/npm/@octokit/rest_vx.x.x.js',
],
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
const peopleToLabelToFiles = {
'@yipstanley': {
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
},
'@Khan/frontend-infra': {
'': ['src/runOnPush.js', '.geraldignore'],
},
};

const result = await makeCommentBody({
peopleToFiles,
peopleToLabelToFiles,
header: 'Reviewers',
tagPerson: false,
});
Expand All @@ -629,7 +650,8 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
"<details>
<summary><b>Reviewers</b></summary>

* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
* \`@yipstanley\` for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
* \`@Khan/frontend-infra\` for changes to \`src/runOnPush.js\`, \`.geraldignore\`
</details>

Expand Down
13 changes: 7 additions & 6 deletions src/runOnPullRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
GERALD_COMMENT_REVIEWERS_HEADER,
MATCH_COMMENT_HEADER_REGEX,
} from './constants';
import type {NameToLabelToFiles} from './utils';

/**
* @desc Helper function to update, delete, or create a comment
Expand All @@ -30,23 +31,23 @@ import {
*/
const updatePullRequestComment = async (
comment: ?Octokit$IssuesListCommentsResponseItem,
notifyees: {[string]: Array<string>, ...},
reviewers: {[string]: Array<string>, ...},
requiredReviewers: {[string]: Array<string>, ...},
notifyees: NameToLabelToFiles,
reviewers: NameToLabelToFiles,
requiredReviewers: NameToLabelToFiles,
) => {
let body: string = GERALD_COMMENT_HEADER;
body += makeCommentBody({
peopleToFiles: notifyees,
peopleToLabelToFiles: notifyees,
header: GERALD_COMMENT_NOTIFIED_HEADER,
tagPerson: true,
});
body += makeCommentBody({
peopleToFiles: reviewers,
peopleToLabelToFiles: reviewers,
header: GERALD_COMMENT_REVIEWERS_HEADER,
tagPerson: false,
});
body += makeCommentBody({
peopleToFiles: requiredReviewers,
peopleToLabelToFiles: requiredReviewers,
header: GERALD_COMMENT_REQ_REVIEWERS_HEADER,
tagPerson: false,
});
Expand Down
18 changes: 10 additions & 8 deletions src/runOnPush.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,19 @@ import {getNotified, getFileDiffs, getFileContents} from './utils';
import {execCmd} from './execCmd';
import {ownerAndRepo, extraPermGithub, type Context} from './setup';
import {PUSH, GERALD_COMMIT_COMMENT_HEADER} from './constants';
import type {NameToLabelToFiles} from './utils';

const makeCommitComment = async (
peopleToFiles: {[string]: Array<string>, ...},
commitSHA: string,
) => {
const names: string[] = Object.keys(peopleToFiles);
if (peopleToFiles && names.length) {
const makeCommitComment = async (peopleToLabelToFiles: NameToLabelToFiles, commitSHA: string) => {
const names: string[] = Object.keys(peopleToLabelToFiles);
if (peopleToLabelToFiles && names.length) {
let body: string = GERALD_COMMIT_COMMENT_HEADER;
names.forEach((person: string) => {
const files = peopleToFiles[person];
body += `${person} for changes to \`${files.join('`, `')}\`\n`;
const labels: string[] = Object.keys(peopleToLabelToFiles[person]);
labels.forEach((label: string) => {
const files = peopleToLabelToFiles[person][label];
const labelText = label ? ` (${label})` : '';
body += `${person} for changes to \`${files.join('`, `')}\`${labelText}\n`;
});
});

await extraPermGithub.repos.createCommitComment({
Expand Down
Loading