From 62481abcaa15166f21a75b375310912080922564 Mon Sep 17 00:00:00 2001 From: Jan Hesse Date: Fri, 21 Jun 2024 02:16:09 +0200 Subject: [PATCH] feat: Add options 'one_of' and 'none_of' for filters and validators (#758) --- __fixtures__/unit/helper.js | 3 +- __tests__/unit/filters/author.test.js | 143 ++++++++++- __tests__/unit/validators/author.test.js | 147 ++++++++++++ __tests__/unit/validators/lastComment.test.js | 223 ++++++++++++++++-- docs/changelog.rst | 1 + docs/deployment.rst | 2 +- docs/filters/author.rst | 7 +- docs/filters/payload.rst | 12 + docs/recipes.rst | 32 +++ docs/validators/author.rst | 7 +- docs/validators/lastComment.rst | 47 +++- lib/filters/author.js | 4 +- .../options_processor/options/none_of.js | 11 + .../options_processor/options/one_of.js | 11 + lib/filters/payload.js | 10 +- lib/validators/author.js | 13 +- lib/validators/lastComment.js | 63 ++++- .../options_processor/listProcessor.js | 45 ++++ .../options_processor/options/none_of.js | 26 ++ .../options_processor/options/one_of.js | 26 ++ lib/validators/validator.js | 4 +- 21 files changed, 791 insertions(+), 46 deletions(-) create mode 100644 lib/filters/options_processor/options/none_of.js create mode 100644 lib/filters/options_processor/options/one_of.js create mode 100644 lib/validators/options_processor/listProcessor.js create mode 100644 lib/validators/options_processor/options/none_of.js create mode 100644 lib/validators/options_processor/options/one_of.js diff --git a/__fixtures__/unit/helper.js b/__fixtures__/unit/helper.js index b92b14d6..1ede4119 100644 --- a/__fixtures__/unit/helper.js +++ b/__fixtures__/unit/helper.js @@ -74,7 +74,8 @@ module.exports = { updated_at: (options.updatedAt) ? options.updatedAt : new Date().toISOString(), assignees: (options.assignees) ? options.assignees : [], pull_request: {} - } + }, + comment: options.issueComment }, log: { child: (s) => { diff --git a/__tests__/unit/filters/author.test.js b/__tests__/unit/filters/author.test.js index a58184d3..63ef561d 100644 --- a/__tests__/unit/filters/author.test.js +++ b/__tests__/unit/filters/author.test.js @@ -17,6 +17,16 @@ test('should fail with unexpected author', async () => { expect(filter.status).toBe('fail') }) +test('should fail with unexpected author one_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: [otherAuthorName] + } + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + test('should pass with expected author', async () => { const author = new Author() const settings = { @@ -29,6 +39,16 @@ test('should pass with expected author', async () => { expect(filter.status).toBe('pass') }) +test('should pass with one_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: [authorName] + } + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + test('should fail with excluded author', async () => { const author = new Author() const settings = { @@ -41,13 +61,21 @@ test('should fail with excluded author', async () => { expect(filter.status).toBe('fail') }) -test('should pass with excluded author', async () => { +test('should fail with none_of author', async () => { const author = new Author() const settings = { do: 'author', - must_exclude: { - regex: otherAuthorName - } + none_of: [authorName] + } + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + +test('should pass with none_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: [otherAuthorName] } const filter = await author.processFilter(createMockContext(authorName), settings) expect(filter.status).toBe('pass') @@ -67,6 +95,20 @@ test('should pass with expected author from correct team', async () => { expect(filter.status).toBe('pass') }) +test('should pass with one_of author from correct team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: authorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + test('should fail with expected author from incorrect team', async () => { const author = new Author() const settings = { @@ -81,6 +123,20 @@ test('should fail with expected author from incorrect team', async () => { expect(filter.status).toBe('fail') }) +test('should fail with one_of author from incorrect team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: authorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + test('should fail with unexpected author from correct team', async () => { const author = new Author() const settings = { @@ -95,6 +151,20 @@ test('should fail with unexpected author from correct team', async () => { expect(filter.status).toBe('fail') }) +test('should fail with one_of author from correct team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: otherAuthorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + test('should pass when the author is a member of the team', async () => { const author = new Author() const settings = { @@ -106,6 +176,17 @@ test('should pass when the author is a member of the team', async () => { expect(filter.status).toBe('pass') }) +test('should pass when the author is one_of the members of the team', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + test('should fail when the author is not a member of the team', async () => { const author = new Author() const authorName = 'mergeable' @@ -118,6 +199,60 @@ test('should fail when the author is not a member of the team', async () => { expect(filter.status).toBe('fail') }) +test('should fail when the author is not one_of the members of the team', async () => { + const author = new Author() + const authorName = 'mergeable' + const settings = { + do: 'author', + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([otherAuthorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + +test('should pass when the author is not member of the none_of team', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([otherAuthorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + +test('should fail when the author is member of the none_of team', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + +test('should pass when the author is one_of @author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: ['@author'] + } + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + +test('should fail when the author is none_of @author', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@author'] + } + const filter = await author.processFilter(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + const createMockContext = (author) => { return Helper.mockContext({ author }) } diff --git a/__tests__/unit/validators/author.test.js b/__tests__/unit/validators/author.test.js index d7da0ae3..206e5554 100644 --- a/__tests__/unit/validators/author.test.js +++ b/__tests__/unit/validators/author.test.js @@ -17,6 +17,16 @@ test('should fail with unexpected author', async () => { expect(validation.status).toBe('fail') }) +test('should fail with unexpected one_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: [otherAuthorName] + } + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('fail') +}) + test('should pass with expected author', async () => { const author = new Author() const settings = { @@ -29,6 +39,16 @@ test('should pass with expected author', async () => { expect(validation.status).toBe('pass') }) +test('should pass with one_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: [authorName] + } + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('pass') +}) + test('should fail with excluded author', async () => { const author = new Author() const settings = { @@ -41,6 +61,16 @@ test('should fail with excluded author', async () => { expect(validation.status).toBe('fail') }) +test('should fail with none_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: [authorName] + } + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('fail') +}) + test('should pass with excluded author', async () => { const author = new Author() const settings = { @@ -53,6 +83,16 @@ test('should pass with excluded author', async () => { expect(validation.status).toBe('pass') }) +test('should pass with none_of author', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: [otherAuthorName] + } + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('pass') +}) + test('should pass with expected author from correct team', async () => { const author = new Author() const settings = { @@ -67,6 +107,20 @@ test('should pass with expected author from correct team', async () => { expect(validation.status).toBe('pass') }) +test('should pass with one_of author from correct team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: authorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('pass') +}) + test('should fail with expected author from incorrect team', async () => { const author = new Author() const settings = { @@ -81,6 +135,20 @@ test('should fail with expected author from incorrect team', async () => { expect(validation.status).toBe('fail') }) +test('should fail with one_of author from incorrect team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: authorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([]) + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('fail') +}) + test('should fail with unexpected author from correct team', async () => { const author = new Author() const settings = { @@ -95,6 +163,20 @@ test('should fail with unexpected author from correct team', async () => { expect(validation.status).toBe('fail') }) +test('should fail with one_of author from correct team', async () => { + const author = new Author() + const settings = { + do: 'author', + must_include: { + regex: otherAuthorName + }, + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('fail') +}) + test('should pass when the author is a member of the team', async () => { const author = new Author() const settings = { @@ -106,6 +188,17 @@ test('should pass when the author is a member of the team', async () => { expect(validation.status).toBe('pass') }) +test('should pass when the author is one_of the members of the team', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('pass') +}) + test('should fail when the author is not a member of the team', async () => { const author = new Author() const authorName = 'mergeable' @@ -118,6 +211,60 @@ test('should fail when the author is not a member of the team', async () => { expect(validation.status).toBe('fail') }) +test('should fail when the author is not one_of the members of the team', async () => { + const author = new Author() + const authorName = 'mergeable' + const settings = { + do: 'author', + one_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([otherAuthorName]) + const validation = await author.processValidate(createMockContext(authorName), settings) + expect(validation.status).toBe('fail') +}) + +test('should pass when the author is not member of the none_of team', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([otherAuthorName]) + const filter = await author.processValidate(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + +test('should fail when the author is member of the none_of team', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@org/team-slug'] + } + Teams.extractTeamMembers = jest.fn().mockReturnValue([authorName]) + const filter = await author.processValidate(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + +test('should pass when the author is one_of @author', async () => { + const author = new Author() + const settings = { + do: 'author', + one_of: ['@author'] + } + const filter = await author.processValidate(createMockContext(authorName), settings) + expect(filter.status).toBe('pass') +}) + +test('should fail when the author is none_of @author', async () => { + const author = new Author() + const settings = { + do: 'author', + none_of: ['@author'] + } + const filter = await author.processValidate(createMockContext(authorName), settings) + expect(filter.status).toBe('fail') +}) + const createMockContext = (author) => { return Helper.mockContext({ author }) } diff --git a/__tests__/unit/validators/lastComment.test.js b/__tests__/unit/validators/lastComment.test.js index f2178299..ab73edd0 100644 --- a/__tests__/unit/validators/lastComment.test.js +++ b/__tests__/unit/validators/lastComment.test.js @@ -18,6 +18,137 @@ test('validate returns correctly', async () => { expect(results.status).toBe('pass') }) +test('validate applies user exclusions correctly', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_exclude: { + regex: 'exclude this' + }, + comment_author: { + none_of: ['exclude_author'] + } + } + + let results = await lastComment.processValidate(createMockContext(['exclude this'], ['exclude_author']), settings) + // must_exclude is not applied since the commenter is excluded + expect(results.status).toBe('pass') + + results = await lastComment.processValidate(createMockContext(['exclude this'], ['some[bot]']), settings) + // must_exclude is not applied since the commenting bot is excluded + expect(results.status).toBe('pass') + + // must_exclude has no match + results = await lastComment.processValidate(createMockContext(['a', 'b'], ['user_a', 'user_b']), settings) + expect(results.status).toBe('pass') + + // must_exclude is applied to one comment since the commenter is not in the excluded list + results = await lastComment.processValidate(createMockContext(['a', 'exclude this'], ['user_a', 'user_b']), settings) + expect(results.status).toBe('fail') +}) + +test('validate applies bot exclusions correctly', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_exclude: { + regex: 'a bot comment' + }, + comment_author: { + no_bots: true + } + } + + // must_exclude is not applied since the commenter is excluded + let results = await lastComment.processValidate(createMockContext(['a bot comment'], ['some[bot]']), settings) + expect(results.status).toBe('pass') + + // must_exclude has no match + results = await lastComment.processValidate(createMockContext(['a', 'b'], ['some[bot]', 'user_b']), settings) + expect(results.status).toBe('pass') + + // must_exclude is applied to one comment since the commenter is not in the excluded list + results = await lastComment.processValidate(createMockContext(['a', 'a bot comment'], ['some[bot]', 'user_b']), settings) + expect(results.status).toBe('fail') +}) + +test('validate applies bot exclusions correctly', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_exclude: { + regex: 'a bot comment' + }, + comment_author: { + none_of: ['angry[bot]'], + no_bots: false + } + } + + // must_exclude is applied since the commenting bot is not excluded + let results = await lastComment.processValidate(createMockContext(['a bot comment'], ['some[bot]']), settings) + expect(results.status).toBe('fail') + + // must_exclude is not applied since the commenting bot is explicitly excluded + results = await lastComment.processValidate(createMockContext(['a bot comment'], ['angry[bot]']), settings) + expect(results.status).toBe('pass') + + // must_exclude has no match + results = await lastComment.processValidate(createMockContext(['a', 'b'], ['user_a', 'user_b']), settings) + expect(results.status).toBe('pass') + + // must_exclude is applied to one comment since the commenter is not in the excluded list + results = await lastComment.processValidate(createMockContext(['a', 'a bot comment'], ['user_a', 'user_b']), settings) + expect(results.status).toBe('fail') +}) + +test('validate applies @author exclusions correctly', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_exclude: { + regex: 'exclude this' + }, + comment_author: { + none_of: ['@author'] + } + } + + let results = await lastComment.processValidate(createMockContext(['exclude this'], ['creator']), settings) + // must_exclude is not applied since the commenter is excluded + expect(results.status).toBe('pass') + + // must_exclude is applied to one comment since the commenter is not in the excluded list + results = await lastComment.processValidate(createMockContext(['a', 'exclude this'], ['creator', 'user_b']), settings) + expect(results.status).toBe('fail') +}) + +test('validate applies @author inclusions correctly', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_exclude: { + regex: 'exclude this' + }, + comment_author: { + one_of: ['@author'] + } + } + + let results = await lastComment.processValidate(createMockContext(['exclude this'], ['creator']), settings) + // must_exclude is applied since the commenter is included + expect(results.status).toBe('fail') + + // must_exclude is applied to one comment since the commenter is in the included list + results = await lastComment.processValidate(createMockContext(['a', 'exclude this'], ['creator', 'user_b']), settings) + expect(results.status).toBe('fail') +}) + test('fail gracefully if invalid regex', async () => { const lastComment = new LastComment() @@ -85,42 +216,94 @@ test('mergeable is false if must_exclude is one of the lastComment', async () => expect(validation.status).toBe('pass') }) +test('mergeable is true if single comment got edited, disregarding whether its the last comment', async () => { + const lastComment = new LastComment() + + const settings = { + do: 'lastComment', + must_include: { + regex: 'xyz' + }, + must_exclude: { + regex: 'ignore' + } + } + + let validation = await lastComment.processValidate(createMockContext(['abc', 'experimental', 'xyz'], undefined, 'issue_comment'), settings) + // must_include is applied for the last comment, when triggered by an edit operation + expect(validation.status).toBe('pass') + + validation = await lastComment.processValidate(createMockContext(['abc', 'xyz', '456'], undefined, 'issue_comment'), settings) + // must_include is applied although its not the last comment, when triggered by an edit operation + expect(validation.status).toBe('pass') +}) + test('complex Logic test', async () => { const lastComment = new LastComment() const settings = { do: 'lastComment', - or: [{ - and: [{ + comment_author: { + none_of: ['e1'], + no_bots: false + }, + or: [ + { + and: [ + { + must_include: { + regex: 'release note: yes', + message: 'Please include release note: yes' + } + }, + { + must_include: { + regex: 'lang\\/core|lang\\/c\\+\\+|lang\\/c#', + message: 'Please include a language comment' + } + } + ] + }, + { must_include: { - regex: 'release note: yes', - message: 'Please include release note: yes' + regex: 'release note: no', + message: 'Please include release note: no' } - }, { - must_include: { - regex: 'lang\\/core|lang\\/c\\+\\+|lang\\/c#', - message: 'Please include a language comment' - } - }] - }, { - must_include: { - regex: 'release note: no', - message: 'Please include release note: no' } - }] + ] } - let validation = await lastComment.processValidate(createMockContext(['experimental', 'xyz', 'release note: no']), settings) + let validation = await lastComment.processValidate(createMockContext(['experimental', 'xyz', 'release note: no'], ['u1', 'u1', 'u1']), settings) expect(validation.status).toBe('pass') - validation = await lastComment.processValidate(createMockContext(['123', '456', 'release note: yes']), settings) + validation = await lastComment.processValidate(createMockContext(['123', '456', 'release note: yes'], ['u1', 'u1', 'u1']), settings) expect(validation.status).toBe('fail') expect(validation.validations[0].description).toBe('((Please include a language comment) ***OR*** Please include release note: no)') - validation = await lastComment.processValidate(createMockContext(['456', 'lang/core']), settings) + validation = await lastComment.processValidate(createMockContext(['456', 'lang/core'], ['u1', 'u1']), settings) + expect(validation.status).toBe('fail') expect(validation.validations[0].description).toBe('((Please include release note: yes) ***OR*** Please include release note: no)') + + // correct comments are ignored since the commenter is excluded + validation = await lastComment.processValidate(createMockContext(['lang/core', 'release note: yes'], ['e1', 'e1']), settings) + expect(validation.status).toBe('fail') + expect(validation.validations[0].description).toBe('((Please include a language comment) ***OR*** Please include release note: no)') }) -function createMockContext (comments) { - return Helper.mockContext({ comments: Array.isArray(comments) ? comments.map(comment => ({ body: comment })) : [{ body: comments }] }) +function createMockContext (comments, commenters, eventname = 'pull_request') { + const constructComment = (comment, commenter) => { + const dataComment = { + body: comment, + user: { login: commenter || 'creator' } + } + return dataComment + } + + return Helper.mockContext({ + eventName: eventname, + issueComment: constructComment(comments, commenters), + comments: Array.isArray(comments) + ? comments.map((comment, idx) => constructComment(comment, commenters ? commenters[idx] : undefined)) + : [constructComment(comments, commenters)] + }) } diff --git a/docs/changelog.rst b/docs/changelog.rst index d8f5fd4a..9142ade0 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -1,5 +1,6 @@ CHANGELOG ===================================== +| June 20, 2024: feat: Add options 'one_of' and 'none_of'. Support in filters `payload`, `author`, and in action `lastComment` to filter comments authors `#757 `_ | June 20, 2024: feat: Add annotations @sender @bot @repository @action and template helper {{formatDate}} `#756 `_ | June 20, 2024: fix: Comments on Issues should not trigger `checks` action `#759 `_ | June 20, 2024: fix: Respect all comments in lastComment validator and comment action `#755 `_ diff --git a/docs/deployment.rst b/docs/deployment.rst index 7d222403..1a93ede9 100644 --- a/docs/deployment.rst +++ b/docs/deployment.rst @@ -23,7 +23,7 @@ The `Probot deployment guide `_ descr * Commit statuses - **Read & Write** * Single file - **Read-only** * Path: ``.github/mergeable.yml`` -* Contents - **Read-Only** +* Contents - **Read-Only** (Note: the ``merge`` action requires Read & Write) * Projects - **Read-Only** **Organization Permissions:** diff --git a/docs/filters/author.rst b/docs/filters/author.rst index 37e25514..c80fad3e 100644 --- a/docs/filters/author.rst +++ b/docs/filters/author.rst @@ -6,12 +6,13 @@ Author - do: author must_include: regex: 'user-1' - message: 'Custom include message...' + message: 'Custom include message...' # optional must_exclude: regex: 'user-2' - message: 'Custom exclude message...' + message: 'Custom exclude message...' # optional team: 'org/team-slug' # verify that the author is in the team - # all of the message sub-option is optional + one_of: ['user-1', '@org/team-slug'] # verify author for being one of the users or a team member + none_of: ['user-2', '@bot'] # verify author for not being one of the users or the mergeable bot you can use ``and`` and ``or`` options to create more complex filters diff --git a/docs/filters/payload.rst b/docs/filters/payload.rst index baa7d18d..21a8d7c2 100644 --- a/docs/filters/payload.rst +++ b/docs/filters/payload.rst @@ -34,6 +34,16 @@ An example to check if a `pull_request` event has a ``label`` named `foo` regex: 'foo' key: 'name' +An example to check whether the initiator of the event is part of a team but not excluded. + +:: + + - do: payload + sender: + login: + one_of: ['@org/team-slug'] + none_of: ['banned_user', '@author', '@bot'] + Each field must be checked using one of the following options @@ -49,6 +59,8 @@ Each field must be checked using one of the following options regex: 'Text to exclude' regex_flag: 'none' # Optional. Specify the flag for Regex. default is 'i', to disable default use 'none' key: 'name' # Optional. If checking an array of objects, this specifies the key to check. + one_of: ['user-1', 'user-2'] # Compares the field value for occurance in the list of strings, case-insensitive, annotations supported + none_of: ['@author', '@bot'] # Compares the field value for absence in the list of strings, case-insensitive, annotations supported Supported Events: diff --git a/docs/recipes.rst b/docs/recipes.rst index 89686b7a..88a83a61 100644 --- a/docs/recipes.rst +++ b/docs/recipes.rst @@ -148,6 +148,38 @@ Add a comment on a pull request when it is created Thanks for creating a pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. +React on a comment to merge pull requests +""""""""""""""""""""""""""""""""""""""""" +When a human creates a new comment or edits existing comment, ``mergeable`` finds a special command in it, to then execute a merge. +The comment writer must be different from the PR author and be member of a specified team. + +:: + + version: 2 + mergeable: + - when: issue_comment.* + name: "Merge pull requests when requested via comment" + filter: + - do: payload + sender: + login: + must_exclude: + regex: '\[bot\]$' + validate: + - do: lastComment + must_include: + regex: 'merge$' + message: 'Comment "merge" detected, checking eligibility to merge the PR.' + must_exclude: + regex: '^\[ \]' + message: 'Comment contains unchecked items, can't merge yet.' + comment_author: + one_of: ['@org/product-owners-team'] + none_of: ['@author'] + pass: + - do: merge + merge_method: "squash" + Auto-merge pull requests once all checks pass """"""""""""""""""""""""""""""""""""""""""""" This recipe relies on the fact that the main branch has been protected and only allows merges diff --git a/docs/validators/author.rst b/docs/validators/author.rst index 37e25514..c80fad3e 100644 --- a/docs/validators/author.rst +++ b/docs/validators/author.rst @@ -6,12 +6,13 @@ Author - do: author must_include: regex: 'user-1' - message: 'Custom include message...' + message: 'Custom include message...' # optional must_exclude: regex: 'user-2' - message: 'Custom exclude message...' + message: 'Custom exclude message...' # optional team: 'org/team-slug' # verify that the author is in the team - # all of the message sub-option is optional + one_of: ['user-1', '@org/team-slug'] # verify author for being one of the users or a team member + none_of: ['user-2', '@bot'] # verify author for not being one of the users or the mergeable bot you can use ``and`` and ``or`` options to create more complex filters diff --git a/docs/validators/lastComment.rst b/docs/validators/lastComment.rst index da4e1821..9a69e757 100644 --- a/docs/validators/lastComment.rst +++ b/docs/validators/lastComment.rst @@ -1,14 +1,53 @@ -Last Comment -^^^^^^^^^^ +LastComment +^^^^^^^^^^^ +Validates that the newly created comment contains or excludes given text. When an existing comment is edited, then exactly this one is validated instead. :: - - do: lastComment // check if the last comment contains only the word 'merge' + - do: lastComment + must_include: + regex: '/sign' + regex_flag: 'none' # Optional. Specify the flag for Regex. default is 'i', to disable default use 'none' + message: 'Contributor Agreement signed...' + must_exclude: + regex: 'incompliant' + regex_flag: 'none' # Optional. Specify the flag for Regex. default is 'i', to disable default use 'none' + message: 'Violates compliance...' + comment_author: + one_of: ['user-1', '@author'] # when the option is present, ONLY comments from users in this list will be considered, use @author for PR/Issue author + none_of: ['user-2', '@author'] # when the option is present, comments from users in this list will NOT be considered, use @author for PR/Issue author + no_bots: true # by default comments from any bots will NOT be considered, set to false to exclude only specific bots explicitly in 'comment_author' option + +Simple example: +:: + + # check if the last comment contains only the word 'merge' + - do: lastComment must_include: regex: '^merge$' +Complex example: +:: + + # check if the last comment, not posted by PR/Issue author, meets one of these conditions + # it might have been posted by a bot, except Mergeble itself + - do: lastComment + comment_author: + none_of: ['Mergeable[bot]', '@author'] + no_bots: false + or: + - and: + - must_exclude: + regex: 'block|wip|stale' + message: 'pre-requisites are not fulfilled...' + - must_include: + regex: 'agreed|confirmed|compliant' + message: 'pre-requisites are fulfilled...' + - must_include: + regex: '^/override$' + message: 'skip pre-requisite check...' + Supported Events: :: 'pull_request.*', 'pull_request_review.*', 'issues.*', 'issue_comment.*' - diff --git a/lib/filters/author.js b/lib/filters/author.js index 642e5c43..cf319534 100644 --- a/lib/filters/author.js +++ b/lib/filters/author.js @@ -18,7 +18,9 @@ class Author extends Filter { regex_flag: 'string', message: 'string' }, - team: 'string' + team: 'string', + one_of: 'array', + none_of: 'array' } } diff --git a/lib/filters/options_processor/options/none_of.js b/lib/filters/options_processor/options/none_of.js new file mode 100644 index 00000000..d2401f91 --- /dev/null +++ b/lib/filters/options_processor/options/none_of.js @@ -0,0 +1,11 @@ +const listProcessor = require('../../../validators/options_processor/listProcessor') +const noneOf = require('../../../validators/options_processor/options/none_of') + +class NoneOf { + static async process (context, filter, input, rule) { + const candidates = await listProcessor.process(rule.none_of, context) + return noneOf.process(filter, input, { none_of: candidates }) + } +} + +module.exports = NoneOf diff --git a/lib/filters/options_processor/options/one_of.js b/lib/filters/options_processor/options/one_of.js new file mode 100644 index 00000000..9e746b81 --- /dev/null +++ b/lib/filters/options_processor/options/one_of.js @@ -0,0 +1,11 @@ +const listProcessor = require('../../../validators/options_processor/listProcessor') +const oneOf = require('../../../validators/options_processor/options/one_of') + +class OneOf { + static async process (context, filter, input, rule) { + const candidates = await listProcessor.process(rule.one_of, context) + return oneOf.process(filter, input, { one_of: candidates }) + } +} + +module.exports = OneOf diff --git a/lib/filters/payload.js b/lib/filters/payload.js index affbc998..138519bb 100644 --- a/lib/filters/payload.js +++ b/lib/filters/payload.js @@ -4,18 +4,18 @@ const constructError = require('./options_processor/options/lib/constructError') const _ = require('lodash') const { forEach } = require('p-iteration') -const options = ['boolean', 'must_include', 'must_exclude'] +const options = ['boolean', 'must_include', 'must_exclude', 'one_of', 'none_of'] -async function recursveThruFields (filterObj, currentPath, output, payload, field) { +async function recursveThruFields (filterObj, context, currentPath, output, payload, field) { await forEach(Object.keys(field), async key => { if (key === 'do') return if (options.includes(key)) { - output.push(await filterObj.processOptions(filterObj, payload, Object.assign(field, { do: currentPath }))) + output.push(await filterObj.processOptions(context, payload, Object.assign(field, { do: currentPath }))) } else if (_.isUndefined(payload[key])) { output.push(constructError(filterObj, '', field, `${currentPath + '.' + key} does NOT exist`)) } else { - await recursveThruFields(filterObj, `${currentPath + '.' + key}`, output, payload[key], field[key]) + await recursveThruFields(filterObj, context, `${currentPath + '.' + key}`, output, payload[key], field[key]) } }) } @@ -36,7 +36,7 @@ class Payload extends Filter { async filter (context, settings) { const output = [] - await recursveThruFields(this, 'payload', output, this.getPayload(context, true), settings) + await recursveThruFields(this, context, 'payload', output, this.getPayload(context, true), settings) const filter = { name: settings.do, diff --git a/lib/validators/author.js b/lib/validators/author.js index 3fb555f8..88f66ed2 100644 --- a/lib/validators/author.js +++ b/lib/validators/author.js @@ -1,5 +1,7 @@ const { Validator } = require('./validator') const Teams = require('./options_processor/teams') +const ListProcessor = require('./options_processor/listProcessor') + class Author extends Validator { constructor () { super('author') @@ -18,7 +20,9 @@ class Author extends Validator { regex_flag: 'string', message: 'string' }, - team: 'string' + team: 'string', + one_of: 'array', + none_of: 'array' } } @@ -33,6 +37,13 @@ class Author extends Validator { delete settings.team } + if (settings.one_of) { + settings.one_of = await ListProcessor.process(settings.one_of, context) + } + if (settings.none_of) { + settings.none_of = await ListProcessor.process(settings.none_of, context) + } + return this.processOptions(settings, payload.user.login) } } diff --git a/lib/validators/lastComment.js b/lib/validators/lastComment.js index 5cbd6e46..40fda9c9 100644 --- a/lib/validators/lastComment.js +++ b/lib/validators/lastComment.js @@ -1,13 +1,17 @@ const { Validator } = require('./validator') +const ListProcessor = require('./options_processor/listProcessor') +const _ = require('lodash') class LastComment extends Validator { constructor () { super('lastComment') + // Ignore 'issue_comment.deleted' to not validate what obviously isn't wanted anymore this.supportedEvents = [ 'pull_request.*', 'pull_request_review.*', 'issues.*', - 'issue_comment.*' + 'issue_comment.created', + 'issue_comment.edited' ] this.supportedSettings = { must_include: { @@ -19,18 +23,73 @@ class LastComment extends Validator { regex: ['string', 'array'], regex_flag: 'string', message: 'string' + }, + comment_author: { + one_of: 'array', + none_of: 'array', + no_bots: 'boolean' } } } async validate (context, validationSettings) { - const comments = await this.githubAPI.listComments(context, this.getPayload(context).number) + let excludeBots = true + const commentAuthorOption = { do: validationSettings.do } + if (validationSettings.comment_author) { + if (validationSettings.comment_author.one_of) { + commentAuthorOption.one_of = await ListProcessor.process(validationSettings.comment_author.one_of, context) + } + if (validationSettings.comment_author.none_of) { + commentAuthorOption.none_of = await ListProcessor.process(validationSettings.comment_author.none_of, context) + } + if (validationSettings.comment_author.no_bots === false) { + excludeBots = false + } + } + delete validationSettings.comment_author + + // payload is the issue or pull_request that the comment was posted in + const payload = this.getPayload(context) + const issueNumber = payload.number + let comments = [] + if (context.eventName === 'issue_comment') { + // the single comment that got created/edited + comments = [this.getPayload(context, true).comment] + } else { + // all the comments of the issue or pr + comments = await this.githubAPI.listComments(context, issueNumber) + } + + comments = await this.filterByCommentAuthor(comments, excludeBots, commentAuthorOption) return this.processOptions( validationSettings, comments.length ? comments[comments.length - 1].body : '' ) } + + async filterByCommentAuthor (comments, excludeBots, commentAuthorOption) { + let filteredComments = Array.from(comments) + + // exclude all GitHub bots by default + if (excludeBots) { + filteredComments = _.reject(filteredComments, c => c.user.login.toLowerCase().endsWith('[bot]')) + } + + // for each comment, process comment_author option and drop those which don't pass + if (commentAuthorOption) { + const filtered = [] + for (const c of filteredComments) { + const result = await this.processOptions(commentAuthorOption, c.user.login) + if (result.status === 'fail') { + filtered.push(c) + } + } + filteredComments = _.difference(filteredComments, filtered) + } + + return filteredComments + } } module.exports = LastComment diff --git a/lib/validators/options_processor/listProcessor.js b/lib/validators/options_processor/listProcessor.js new file mode 100644 index 00000000..b21f911d --- /dev/null +++ b/lib/validators/options_processor/listProcessor.js @@ -0,0 +1,45 @@ +const Teams = require('./teams') +const TeamNotFoundError = require('../../errors/teamNotFoundError') +const EventAware = require('../../eventAware') +const searchAndReplaceSpecialAnnotations = require('../../actions/lib/searchAndReplaceSpecialAnnotation') +const { forEach } = require('p-iteration') + +/** + * ListProcessor replaces annotations in an array of strings. + * Team slugs are exploded to the members they contain. + * All elements are lowercased to be used for comparison with the one_of or none_of option processor. + * @returns a new array containing the replacements + */ +class ListProcessor { + static async process (list, context) { + if (!Array.isArray(list)) { + list = [list] + } + + const candidates = [] + const helper = new EventAware() + const payload = helper.getPayload(context) + const evt = helper.getEventAttributes(context) + await forEach(list, async (element) => { + if (element.match(/^@.+\/[^/]+$/)) { + try { + const teamMembers = await Teams.extractTeamMembers(context, [element]) + candidates.push(...teamMembers.map((m) => m.toLowerCase())) + } catch (err) { + if (err instanceof TeamNotFoundError) { + // uncritical, is just no candidate + } else { + throw err + } + } + } else { + const replacement = searchAndReplaceSpecialAnnotations(element, payload, evt) + candidates.push(replacement.toLowerCase()) + } + }) + + return candidates + } +} + +module.exports = ListProcessor diff --git a/lib/validators/options_processor/options/none_of.js b/lib/validators/options_processor/options/none_of.js new file mode 100644 index 00000000..1a8d9113 --- /dev/null +++ b/lib/validators/options_processor/options/none_of.js @@ -0,0 +1,26 @@ +const UNKNOWN_INPUT_TYPE_ERROR = 'Input type invalid, expected string as input' +const LIST_NOT_FOUND_ERROR = 'Failed to run the test because \'none_of\' option is not present. Please check README for more information about configuration' + +class NoneOf { + static process (validatorContext, input, rule) { + const filter = rule.none_of + if (typeof input !== 'string') { + throw new Error(UNKNOWN_INPUT_TYPE_ERROR) + } + if (!Array.isArray(filter)) { + throw new Error(LIST_NOT_FOUND_ERROR) + } + + const isExcluded = !filter.includes(input.toLowerCase()) + + const successMessage = `'${input}' is not in the none_of list'` + const failureMessage = `'${input}' is in the none_of list'` + + return { + status: isExcluded ? 'pass' : 'fail', + description: isExcluded ? successMessage : failureMessage + } + } +} + +module.exports = NoneOf diff --git a/lib/validators/options_processor/options/one_of.js b/lib/validators/options_processor/options/one_of.js new file mode 100644 index 00000000..23ad814a --- /dev/null +++ b/lib/validators/options_processor/options/one_of.js @@ -0,0 +1,26 @@ +const UNKNOWN_INPUT_TYPE_ERROR = 'Input type invalid, expected string as input' +const LIST_NOT_FOUND_ERROR = 'Failed to run the test because \'one_of\' option is not present. Please check README for more information about configuration' + +class OneOf { + static process (validatorContext, input, rule) { + const filter = rule.one_of + if (typeof input !== 'string') { + throw new Error(UNKNOWN_INPUT_TYPE_ERROR) + } + if (!Array.isArray(filter)) { + throw new Error(LIST_NOT_FOUND_ERROR) + } + + const isIncluded = filter.includes(input.toLowerCase()) + + const successMessage = `'${input}' is in the one_of list'` + const failureMessage = `'${input}' is not in the one_of list'` + + return { + status: isIncluded ? 'pass' : 'fail', + description: isIncluded ? successMessage : failureMessage + } + } +} + +module.exports = OneOf diff --git a/lib/validators/validator.js b/lib/validators/validator.js index 8fe7c38c..fbf4d2d0 100644 --- a/lib/validators/validator.js +++ b/lib/validators/validator.js @@ -19,7 +19,9 @@ const DEFAULT_SUPPORTED_OPTIONS = [ 'no_empty', 'required', 'jira', - 'team' + 'team', + 'one_of', + 'none_of' ] class Validator extends EventAware {