From c45db8a36650dce9e0876ec1e2477fd1617b4f2a Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Fri, 18 Oct 2024 11:01:33 -0700 Subject: [PATCH 1/3] feat: add configuration to exclude author from mentions --- __tests__/comment-upserter.test.ts | 65 ++++++++-------- __tests__/fixtures/codemention.yml | 2 +- __tests__/runner.test.ts | 115 ++++++++++++++++++++++------- src/configuration.ts | 2 + src/runner.ts | 13 +++- 5 files changed, 136 insertions(+), 61 deletions(-) diff --git a/__tests__/comment-upserter.test.ts b/__tests__/comment-upserter.test.ts index 2c18215..7fe71d1 100644 --- a/__tests__/comment-upserter.test.ts +++ b/__tests__/comment-upserter.test.ts @@ -3,7 +3,11 @@ import {RestEndpointMethodTypes} from '@octokit/plugin-rest-endpoint-methods' import {RestEndpointMethods} from '@octokit/plugin-rest-endpoint-methods/dist-types/generated/method-types.d' import {EqualMatchingInjectorConfig, It, Mock, Times} from 'moq.ts' -import {CommentUpserterImpl, DEFAULT_COMMENT_PREAMBLE, FOOTER} from '../src/comment-upserter' +import { + CommentUpserterImpl, + DEFAULT_COMMENT_PREAMBLE, + FOOTER +} from '../src/comment-upserter' import {Repo} from '../src/github-types' describe('CommentUpserterImpl', () => { @@ -67,16 +71,15 @@ describe('CommentUpserterImpl', () => { }) it('creates a comment', async () => { - const expectedCommentBody = - [ - DEFAULT_COMMENT_PREAMBLE, - '| File Patterns | Mentions |', - '| - | - |', - '| db/migrate/\\*\\* | @cto, @dba |', - '| .github/\\*\\*
spec/\\*.rb | @ci |', - '', - FOOTER - ].join('\n') + const expectedCommentBody = [ + DEFAULT_COMMENT_PREAMBLE, + '| File Patterns | Mentions |', + '| - | - |', + '| db/migrate/\\*\\* | @cto, @dba |', + '| .github/\\*\\*
spec/\\*.rb | @ci |', + '', + FOOTER + ].join('\n') issuesMock .setup(instance => instance.createComment(It.IsAny())) @@ -102,17 +105,16 @@ describe('CommentUpserterImpl', () => { preamble: 'Added you as a subscriber.', epilogue: '> [CodeMention](https://github.com/tobyhs/codemention)' } - const expectedCommentBody = - [ - customContent.preamble, - '| File Patterns | Mentions |', - '| - | - |', - '| db/migrate/\\*\\* | @cto, @dba |', - '| .github/\\*\\*
spec/\\*.rb | @ci |', - '', - customContent.epilogue, - FOOTER - ].join('\n') + const expectedCommentBody = [ + customContent.preamble, + '| File Patterns | Mentions |', + '| - | - |', + '| db/migrate/\\*\\* | @cto, @dba |', + '| .github/\\*\\*
spec/\\*.rb | @ci |', + '', + customContent.epilogue, + FOOTER + ].join('\n') issuesMock .setup(instance => instance.createComment(It.IsAny())) @@ -138,19 +140,19 @@ describe('CommentUpserterImpl', () => { describe('and the comment is different', () => { describe('and the comment has the sentinel at the start', () => { it('updates the comment', async () => { - const expectedCommentBody = - [ + const expectedCommentBody = [ DEFAULT_COMMENT_PREAMBLE, '| File Patterns | Mentions |', '| - | - |', '| db/migrate/\\*\\* | @cto, @dba |', '| .github/\\*\\*
spec/\\*.rb | @ci |', '', - FOOTER, + FOOTER ].join('\n') // previous version of the action put the sentinel comment at the start - const existingComment = FOOTER + '| config/brakeman.yml | @security |' + const existingComment = + FOOTER + '| config/brakeman.yml | @security |' stubListComments(['First', existingComment]) issuesMock @@ -175,18 +177,18 @@ describe('CommentUpserterImpl', () => { describe('and the comment has the sentinel at the end', () => { it('updates the comment', async () => { - const expectedCommentBody = - [ + const expectedCommentBody = [ DEFAULT_COMMENT_PREAMBLE, '| File Patterns | Mentions |', '| - | - |', '| db/migrate/\\*\\* | @cto, @dba |', '| .github/\\*\\*
spec/\\*.rb | @ci |', '', - FOOTER, + FOOTER ].join('\n') - const existingComment = '| config/brakeman.yml | @security |' + FOOTER + const existingComment = + '| config/brakeman.yml | @security |' + FOOTER stubListComments(['First', existingComment]) issuesMock @@ -212,8 +214,7 @@ describe('CommentUpserterImpl', () => { describe('and the comment is the same', () => { it('does not update the comment', async () => { - const commentBody = - [ + const commentBody = [ DEFAULT_COMMENT_PREAMBLE, '| File Patterns | Mentions |', '| - | - |', diff --git a/__tests__/fixtures/codemention.yml b/__tests__/fixtures/codemention.yml index 5d8f069..1027443 100644 --- a/__tests__/fixtures/codemention.yml +++ b/__tests__/fixtures/codemention.yml @@ -3,7 +3,7 @@ commentConfiguration: epilogue: 'testing epilogue' rules: - patterns: ['config/**'] - mentions: ['sysadmin'] + mentions: ['sysadmin', 'testlogin'] - patterns: ['db/migrate/**'] mentions: ['cto', 'dba'] - patterns: ['.github/**', 'spec/*.rb'] diff --git a/__tests__/runner.test.ts b/__tests__/runner.test.ts index c1aa8ea..02e4840 100644 --- a/__tests__/runner.test.ts +++ b/__tests__/runner.test.ts @@ -14,33 +14,43 @@ import {Repo} from '../src/github-types' import Runner from '../src/runner' describe('Runner', () => { - let configurationReader: ConfigurationReader - let filesChangedReader: FilesChangedReader - let commentUpserterMock: Mock - let runner: Runner - - let repo: Repo - let context: Context - const prNumber = 123 - const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' - let pullRequest: PullRequest - const configuration: Configuration = yaml.load( - fs.readFileSync(path.join(__dirname, 'fixtures', 'codemention.yml'), 'utf8') - ) as Configuration - - beforeEach(() => { - repo = {owner: 'tobyhs', repo: 'codemention'} - pullRequest = { + function setUpDependencies({ + draft, + prNumber, + baseSha, + excludeAuthor + }: { + draft: boolean + prNumber: number + baseSha: string + excludeAuthor?: boolean + }) { + const repo = {owner: 'tobyhs', repo: 'codemention'} + const pullRequest = { number: prNumber, base: {sha: baseSha}, - draft: false + draft, + user: { + login: 'testlogin' + } } as PullRequest - context = { + const context = { repo, payload: {pull_request: pullRequest} } as unknown as Context - configurationReader = new Mock({ + const configuration: Configuration = yaml.load( + fs.readFileSync( + path.join(__dirname, 'fixtures', 'codemention.yml'), + 'utf8' + ) + ) as Configuration + + if (excludeAuthor) { + configuration.excludeAuthor = true + } + + const configurationReader = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) .setup(async instance => instance.read(repo, baseSha)) @@ -51,31 +61,40 @@ describe('Runner', () => { 'config/application.rb', '.github/workflows/codemention.yml' ] - filesChangedReader = new Mock({ + const filesChangedReader = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) .setup(async instance => instance.read(repo, prNumber)) .returnsAsync(filesChanged) .object() - commentUpserterMock = new Mock({ + const commentUpserterMock = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) commentUpserterMock .setup(instance => instance.upsert(It.IsAny(), It.IsAny(), It.IsAny())) .returns(Promise.resolve()) - runner = new Runner( + const runner = new Runner( configurationReader, filesChangedReader, commentUpserterMock.object() ) - }) + + return {repo, runner, context, commentUpserterMock} + } describe('.run', () => { describe('when the pull request is a draft', () => { it('does not upsert a comment', async () => { - pullRequest.draft = true + const prNumber = 123 + const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' + const {runner, context, commentUpserterMock} = setUpDependencies({ + draft: true, + prNumber, + baseSha + }) + await runner.run(context) commentUpserterMock.verify( instance => instance.upsert(It.IsAny(), It.IsAny(), It.IsAny()), @@ -85,11 +104,19 @@ describe('Runner', () => { }) it('runs main logic of the GitHub action', async () => { + const prNumber = 123 + const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' + const {repo, runner, context, commentUpserterMock} = setUpDependencies({ + draft: false, + prNumber, + baseSha + }) + await runner.run(context) const matchingRules = [ { patterns: ['config/**'], - mentions: ['sysadmin'] + mentions: ['sysadmin', 'testlogin'] }, { patterns: ['.github/**', 'spec/*.rb'], @@ -97,8 +124,42 @@ describe('Runner', () => { } ] commentUpserterMock.verify(instance => - instance.upsert(repo, prNumber, matchingRules, {preamble: 'testing preamble', epilogue: 'testing epilogue'}) + instance.upsert(repo, prNumber, matchingRules, { + preamble: 'testing preamble', + epilogue: 'testing epilogue' + }) ) }) + + describe('when excludeAuthor configuration is specified', () => { + it('does not include the author in the comment', async () => { + const prNumber = 123 + const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' + const {repo, runner, context, commentUpserterMock} = setUpDependencies({ + draft: false, + prNumber, + baseSha, + excludeAuthor: true + }) + + await runner.run(context) + const matchingRules = [ + { + patterns: ['config/**'], + mentions: ['sysadmin'] + }, + { + patterns: ['.github/**', 'spec/*.rb'], + mentions: ['ci'] + } + ] + commentUpserterMock.verify(instance => + instance.upsert(repo, prNumber, matchingRules, { + preamble: 'testing preamble', + epilogue: 'testing epilogue' + }) + ) + }) + }) }) }) diff --git a/src/configuration.ts b/src/configuration.ts index f6f77b0..106ef74 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -27,4 +27,6 @@ export interface Configuration { rules: MentionRule[] /** Configuration for comment */ commentConfiguration?: CommentConfiguration + /** Whether to exclude PR author from mentions */ + excludeAuthor?: boolean } diff --git a/src/runner.ts b/src/runner.ts index b8a1165..5489c2d 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -6,6 +6,7 @@ import micromatch from 'micromatch' import {CommentUpserter} from './comment-upserter' import {ConfigurationReader} from './configuration-reader' import {FilesChangedReader} from './files-changed-reader' +import {MentionRule} from './configuration' /** * @see {@link run} @@ -43,10 +44,20 @@ export default class Runner { const matchingRules = configuration.rules.filter( rule => micromatch(filesChanged, rule.patterns).length > 0 ) + const filteredRules = configuration.excludeAuthor + ? matchingRules + .map((rule: MentionRule) => ({ + ...rule, + mentions: rule.mentions.filter( + mention => mention !== pullRequest.user.login + ) + })) + .filter(rule => rule.mentions.length > 0) + : matchingRules await this.commentUpserter.upsert( repo, pullRequest.number, - matchingRules, + filteredRules, configuration.commentConfiguration ) } From a6e2491e74e7b68b8d3becb6eec86ed6ea86cc40 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Mon, 21 Oct 2024 16:32:33 -0700 Subject: [PATCH 2/3] make default instead of configurable --- __tests__/runner.test.ts | 112 ++++++++++----------------------------- src/configuration.ts | 2 - src/runner.ts | 27 +++++----- 3 files changed, 40 insertions(+), 101 deletions(-) diff --git a/__tests__/runner.test.ts b/__tests__/runner.test.ts index 02e4840..2ed2799 100644 --- a/__tests__/runner.test.ts +++ b/__tests__/runner.test.ts @@ -14,43 +14,36 @@ import {Repo} from '../src/github-types' import Runner from '../src/runner' describe('Runner', () => { - function setUpDependencies({ - draft, - prNumber, - baseSha, - excludeAuthor - }: { - draft: boolean - prNumber: number - baseSha: string - excludeAuthor?: boolean - }) { - const repo = {owner: 'tobyhs', repo: 'codemention'} - const pullRequest = { + let configurationReader: ConfigurationReader + let filesChangedReader: FilesChangedReader + let commentUpserterMock: Mock + let runner: Runner + + let repo: Repo + let context: Context + const prNumber = 123 + const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' + let pullRequest: PullRequest + const configuration: Configuration = yaml.load( + fs.readFileSync(path.join(__dirname, 'fixtures', 'codemention.yml'), 'utf8') + ) as Configuration + + beforeEach(() => { + repo = {owner: 'tobyhs', repo: 'codemention'} + pullRequest = { number: prNumber, base: {sha: baseSha}, - draft, + draft: false, user: { login: 'testlogin' } } as PullRequest - const context = { + context = { repo, payload: {pull_request: pullRequest} } as unknown as Context - const configuration: Configuration = yaml.load( - fs.readFileSync( - path.join(__dirname, 'fixtures', 'codemention.yml'), - 'utf8' - ) - ) as Configuration - - if (excludeAuthor) { - configuration.excludeAuthor = true - } - - const configurationReader = new Mock({ + configurationReader = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) .setup(async instance => instance.read(repo, baseSha)) @@ -61,40 +54,31 @@ describe('Runner', () => { 'config/application.rb', '.github/workflows/codemention.yml' ] - const filesChangedReader = new Mock({ + filesChangedReader = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) .setup(async instance => instance.read(repo, prNumber)) .returnsAsync(filesChanged) .object() - const commentUpserterMock = new Mock({ + commentUpserterMock = new Mock({ injectorConfig: new EqualMatchingInjectorConfig() }) commentUpserterMock .setup(instance => instance.upsert(It.IsAny(), It.IsAny(), It.IsAny())) .returns(Promise.resolve()) - const runner = new Runner( + runner = new Runner( configurationReader, filesChangedReader, commentUpserterMock.object() ) - - return {repo, runner, context, commentUpserterMock} - } + }) describe('.run', () => { describe('when the pull request is a draft', () => { it('does not upsert a comment', async () => { - const prNumber = 123 - const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' - const {runner, context, commentUpserterMock} = setUpDependencies({ - draft: true, - prNumber, - baseSha - }) - + pullRequest.draft = true await runner.run(context) commentUpserterMock.verify( instance => instance.upsert(It.IsAny(), It.IsAny(), It.IsAny()), @@ -104,19 +88,11 @@ describe('Runner', () => { }) it('runs main logic of the GitHub action', async () => { - const prNumber = 123 - const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' - const {repo, runner, context, commentUpserterMock} = setUpDependencies({ - draft: false, - prNumber, - baseSha - }) - await runner.run(context) const matchingRules = [ { patterns: ['config/**'], - mentions: ['sysadmin', 'testlogin'] + mentions: ['sysadmin'] }, { patterns: ['.github/**', 'spec/*.rb'], @@ -124,42 +100,8 @@ describe('Runner', () => { } ] commentUpserterMock.verify(instance => - instance.upsert(repo, prNumber, matchingRules, { - preamble: 'testing preamble', - epilogue: 'testing epilogue' - }) + instance.upsert(repo, prNumber, matchingRules, {preamble: 'testing preamble', epilogue: 'testing epilogue'}) ) }) - - describe('when excludeAuthor configuration is specified', () => { - it('does not include the author in the comment', async () => { - const prNumber = 123 - const baseSha = 'bfc5b2d29cfa2db8ce40f6c60bc9629490fe1225' - const {repo, runner, context, commentUpserterMock} = setUpDependencies({ - draft: false, - prNumber, - baseSha, - excludeAuthor: true - }) - - await runner.run(context) - const matchingRules = [ - { - patterns: ['config/**'], - mentions: ['sysadmin'] - }, - { - patterns: ['.github/**', 'spec/*.rb'], - mentions: ['ci'] - } - ] - commentUpserterMock.verify(instance => - instance.upsert(repo, prNumber, matchingRules, { - preamble: 'testing preamble', - epilogue: 'testing epilogue' - }) - ) - }) - }) }) }) diff --git a/src/configuration.ts b/src/configuration.ts index 106ef74..f6f77b0 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -27,6 +27,4 @@ export interface Configuration { rules: MentionRule[] /** Configuration for comment */ commentConfiguration?: CommentConfiguration - /** Whether to exclude PR author from mentions */ - excludeAuthor?: boolean } diff --git a/src/runner.ts b/src/runner.ts index 5489c2d..1f7acdc 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -41,23 +41,22 @@ export default class Runner { this.configurationReader.read(repo, pullRequest.base.sha), this.filesChangedReader.read(repo, pullRequest.number) ]) - const matchingRules = configuration.rules.filter( - rule => micromatch(filesChanged, rule.patterns).length > 0 - ) - const filteredRules = configuration.excludeAuthor - ? matchingRules - .map((rule: MentionRule) => ({ - ...rule, - mentions: rule.mentions.filter( - mention => mention !== pullRequest.user.login - ) - })) - .filter(rule => rule.mentions.length > 0) - : matchingRules + + // filter out the PR author so that they don't get double-notified + const matchingRules = configuration.rules + .filter(rule => micromatch(filesChanged, rule.patterns).length > 0) + .map((rule: MentionRule) => ({ + ...rule, + mentions: rule.mentions.filter( + mention => mention !== pullRequest.user.login + ) + })) + .filter(rule => rule.mentions.length > 0) + await this.commentUpserter.upsert( repo, pullRequest.number, - filteredRules, + matchingRules, configuration.commentConfiguration ) } From e8c5a8a2b9c1d5dba5aa372fa9d6e815345b02c2 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Mon, 21 Oct 2024 16:34:33 -0700 Subject: [PATCH 3/3] clean up comments --- src/runner.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/runner.ts b/src/runner.ts index 1f7acdc..3a75094 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -42,15 +42,17 @@ export default class Runner { this.filesChangedReader.read(repo, pullRequest.number) ]) - // filter out the PR author so that they don't get double-notified const matchingRules = configuration.rules + // filter to rules that match .filter(rule => micromatch(filesChanged, rule.patterns).length > 0) + // filter out the PR author from mentions so that they don't get double-notified .map((rule: MentionRule) => ({ ...rule, mentions: rule.mentions.filter( mention => mention !== pullRequest.user.login ) })) + // filter out the rules that no longer have mentions due to author filtering .filter(rule => rule.mentions.length > 0) await this.commentUpserter.upsert(