From fe2e80910e3e096361415c1bcd9690394fbdb3ab Mon Sep 17 00:00:00 2001 From: Galen Rice Date: Fri, 26 Aug 2022 21:46:09 -0400 Subject: [PATCH 1/7] [FEATURE] Skip merge commits in pull requests Merge commits can be identified by having two parent commits, so we add to the GraphQL query selecting PR commits to also return `parents.totalCount`. This shows any standard commit having 1 parent, and merge commits having 2. We can then `.filter()` out those instances before mapping commit messages. --- src/input-helper.ts | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/input-helper.ts b/src/input-helper.ts index eff4be3..af40452 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -241,7 +241,10 @@ async function getCommitMessagesFromPullRequest( edges { node { commit { - message + message, + parents(last: 1) { + totalCount + } } } } @@ -267,6 +270,9 @@ async function getCommitMessagesFromPullRequest( node: { commit: { message: string + parents: { + totalCount: number + } } } } @@ -289,11 +295,17 @@ async function getCommitMessagesFromPullRequest( let messages: string[] = [] if (repository.pullRequest) { - messages = repository.pullRequest.commits.edges.map(function ( - edge: CommitEdgeItem - ): string { - return edge.node.commit.message - }) + messages = repository.pullRequest.commits.edges + .filter(function (edge: CommitEdgeItem): boolean { + if (edge.node.commit.parents.totalCount > 1) { + // Skip merge commits (which have more than 1 parent commit) + return false + } + return true + }) + .map(function (edge: CommitEdgeItem): string { + return edge.node.commit.message + }) } return messages From 403524f18b044be9cf93a2b2cdbc2c629d3cd257 Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Thu, 20 Oct 2022 08:47:03 +0200 Subject: [PATCH 2/7] [TASK] Extend test --- __tests__/input-helper.test.ts | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/__tests__/input-helper.test.ts b/__tests__/input-helper.test.ts index bb385c0..3326d59 100644 --- a/__tests__/input-helper.test.ts +++ b/__tests__/input-helper.test.ts @@ -445,7 +445,10 @@ describe('input-helper tests', () => { node: { commit: { message: - 'input: make input-helper functions async\n\nIn order to work with asynchronous call like an async http request\nin an easier way, the functions getInput and getMessages were\nconverted to async.' + 'input: make input-helper functions async\n\nIn order to work with asynchronous call like an async http request\nin an easier way, the functions getInput and getMessages were\nconverted to async.', + parents: { + totalCount: 1 + } } } }, @@ -453,7 +456,10 @@ describe('input-helper tests', () => { node: { commit: { message: - "input: PR options ignore title and check PR commits\n\nthis make it possible to igore partially or completely the PR payload.\nThe commits associated with the pull request can be checked instead of\nchecking the pull request payload. The parameter are:\n\n- excludeTitle: 'true | false'\n- excludeDescription: 'true | false'\n- checkAllCommitMessages: 'true | false'\n\nby default, all options comes false." + "input: PR options ignore title and check PR commits\n\nthis make it possible to igore partially or completely the PR payload.\nThe commits associated with the pull request can be checked instead of\nchecking the pull request payload. The parameter are:\n\n- excludeTitle: 'true | false'\n- excludeDescription: 'true | false'\n- checkAllCommitMessages: 'true | false'\n\nby default, all options comes false.", + parents: { + totalCount: 1 + } } } }, @@ -461,7 +467,20 @@ describe('input-helper tests', () => { node: { commit: { message: - 'docs: include parameters excludeTitle, checkAllCommitMessages and accessToken\n\nCo-authored-by: Gilbertsoft <25326036+gilbertsoft@users.noreply.github.com>' + 'docs: include parameters excludeTitle, checkAllCommitMessages and accessToken\n\nCo-authored-by: Gilbertsoft <25326036+gilbertsoft@users.noreply.github.com>', + parents: { + totalCount: 1 + } + } + } + }, + { + node: { + commit: { + message: 'merge: merge commit to be ignored', + parents: { + totalCount: 2 + } } } } From 8c355712406786450999d561f3b85572aebb96b8 Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Thu, 20 Oct 2022 08:55:46 +0200 Subject: [PATCH 3/7] [TASK] Update dist --- dist/index.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/dist/index.js b/dist/index.js index e59aa2b..2b4a19d 100644 --- a/dist/index.js +++ b/dist/index.js @@ -344,7 +344,10 @@ function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, reposito edges { node { commit { - message + message, + parents(last: 1) { + totalCount + } } } } @@ -369,7 +372,15 @@ function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, reposito core.debug(` - response: ${JSON.stringify(repository, null, 2)}`); let messages = []; if (repository.pullRequest) { - messages = repository.pullRequest.commits.edges.map(function (edge) { + messages = repository.pullRequest.commits.edges + .filter(function (edge) { + if (edge.node.commit.parents.totalCount > 1) { + // Skip merge commits (which have more than 1 parent commit) + return false; + } + return true; + }) + .map(function (edge) { return edge.node.commit.message; }); } From 2f931d9d4ad06175dd0510837f2827f858135f2f Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Thu, 20 Oct 2022 09:10:34 +0200 Subject: [PATCH 4/7] [TASK] Ignore merge commits for push event --- __tests__/input-helper.test.ts | 8 ++++++++ dist/index.js | 6 ++++-- src/input-helper.ts | 6 +++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/__tests__/input-helper.test.ts b/__tests__/input-helper.test.ts index 3326d59..e626932 100644 --- a/__tests__/input-helper.test.ts +++ b/__tests__/input-helper.test.ts @@ -553,6 +553,7 @@ describe('input-helper tests', () => { expect(checkerArguments.pattern).toBe('some-pattern') expect(checkerArguments.error).toBe('some-error') expect(checkerArguments.messages).toBeTruthy() + expect(checkerArguments.messages.length).toBe(1) expect(checkerArguments.messages[0]).toBe('some-message') }) @@ -566,6 +567,12 @@ describe('input-helper tests', () => { }, { message: 'other-message' + }, + { + message: 'ignored-message', + parents: { + totalCount: 2 + } } ] } @@ -577,6 +584,7 @@ describe('input-helper tests', () => { expect(checkerArguments.pattern).toBe('some-pattern') expect(checkerArguments.error).toBe('some-error') expect(checkerArguments.messages).toBeTruthy() + expect(checkerArguments.messages.length).toBe(2) expect(checkerArguments.messages[0]).toBe('some-message') expect(checkerArguments.messages[1]).toBe('other-message') }) diff --git a/dist/index.js b/dist/index.js index 2b4a19d..7959185 100644 --- a/dist/index.js +++ b/dist/index.js @@ -236,7 +236,7 @@ exports.getInputs = getInputs; * @returns string[] */ function getMessages(pullRequestOptions) { - var _a; + var _a, _b, _c; return __awaiter(this, void 0, void 0, function* () { core.debug('Get messages...'); core.debug(` - pullRequestOptions: ${JSON.stringify(pullRequestOptions, null, 2)}`); @@ -311,7 +311,9 @@ function getMessages(pullRequestOptions) { break; } for (const i in github.context.payload.commits) { - if (github.context.payload.commits[i].message) { + if (github.context.payload.commits[i].message && + // ignore merge commits + ((_c = (_b = github.context.payload.commits[i].parents) === null || _b === void 0 ? void 0 : _b.totalCount) !== null && _c !== void 0 ? _c : 1) === 1) { messages.push(github.context.payload.commits[i].message); } } diff --git a/src/input-helper.ts b/src/input-helper.ts index af40452..4393913 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -201,7 +201,11 @@ async function getMessages( } for (const i in github.context.payload.commits) { - if (github.context.payload.commits[i].message) { + if ( + github.context.payload.commits[i].message && + // ignore merge commits + (github.context.payload.commits[i].parents?.totalCount ?? 1) === 1 + ) { messages.push(github.context.payload.commits[i].message) } } From 60cf4b896d533348230263a9b32d018a6aaf1a3a Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Thu, 20 Oct 2022 09:30:44 +0200 Subject: [PATCH 5/7] [TASK] Shorten filter function --- dist/index.js | 7 ++----- src/input-helper.ts | 7 ++----- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/dist/index.js b/dist/index.js index 7959185..0985ef1 100644 --- a/dist/index.js +++ b/dist/index.js @@ -376,11 +376,8 @@ function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, reposito if (repository.pullRequest) { messages = repository.pullRequest.commits.edges .filter(function (edge) { - if (edge.node.commit.parents.totalCount > 1) { - // Skip merge commits (which have more than 1 parent commit) - return false; - } - return true; + // Skip merge commits (which have more than 1 parent commit) + return edge.node.commit.parents.totalCount === 1; }) .map(function (edge) { return edge.node.commit.message; diff --git a/src/input-helper.ts b/src/input-helper.ts index 4393913..d5e8259 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -301,11 +301,8 @@ async function getCommitMessagesFromPullRequest( if (repository.pullRequest) { messages = repository.pullRequest.commits.edges .filter(function (edge: CommitEdgeItem): boolean { - if (edge.node.commit.parents.totalCount > 1) { - // Skip merge commits (which have more than 1 parent commit) - return false - } - return true + // Skip merge commits (which have more than 1 parent commit) + return edge.node.commit.parents.totalCount === 1 }) .map(function (edge: CommitEdgeItem): string { return edge.node.commit.message From 42ac0162620fbe60280dce60e20fa50ed939e204 Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:15:37 +0200 Subject: [PATCH 6/7] [BUGFIX] Ignore merge commits for push events --- __tests__/commit-message-checker.test.ts | 2 +- __tests__/input-helper.test.ts | 81 +++++++++++++++++++--- dist/index.js | 56 +++++++++++++++- src/input-helper.ts | 85 +++++++++++++++++++++++- 4 files changed, 212 insertions(+), 12 deletions(-) diff --git a/__tests__/commit-message-checker.test.ts b/__tests__/commit-message-checker.test.ts index b37d64a..cb1c53f 100644 --- a/__tests__/commit-message-checker.test.ts +++ b/__tests__/commit-message-checker.test.ts @@ -1,7 +1,7 @@ /* * This file is part of the "GS Commit Message Checker" Action for Github. * - * Copyright (C) 2019 by Gilbertsoft LLC (gilbertsoft.org) + * Copyright (C) 2019-2022 by Gilbertsoft LLC (gilbertsoft.org) * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, diff --git a/__tests__/input-helper.test.ts b/__tests__/input-helper.test.ts index e626932..90fd671 100644 --- a/__tests__/input-helper.test.ts +++ b/__tests__/input-helper.test.ts @@ -1,7 +1,7 @@ /* * This file is part of the "GS Commit Message Checker" Action for Github. * - * Copyright (C) 2019 by Gilbertsoft LLC (gilbertsoft.org) + * Copyright (C) 2019-2022 by Gilbertsoft LLC (gilbertsoft.org) * * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, @@ -40,10 +40,19 @@ const mockGitHub = jest.genMockFromModule('@actions/github') as any mockGitHub.context = {} // Mock @octokit/graphql +interface IGraphqlResponseGetter { + (parameters?: any): any +} let graphqlResponse = {} as any + +function defaultGraphqlResponseGetter(parameters?: any): any { + return graphqlResponse +} + +let getGraphqlResponse: IGraphqlResponseGetter = defaultGraphqlResponseGetter const mockGraphql = jest.genMockFromModule('@octokit/graphql') as any mockGraphql.graphql = (query: string, parameters?: any): any => { - return graphqlResponse + return getGraphqlResponse(parameters) } describe('input-helper tests', () => { @@ -62,6 +71,7 @@ describe('input-helper tests', () => { inputs = {} mockGitHub.context = {} graphqlResponse = {} + getGraphqlResponse = defaultGraphqlResponseGetter }) afterAll(() => { @@ -541,13 +551,35 @@ describe('input-helper tests', () => { payload: { commits: [ { + id: '1', message: 'some-message' } - ] + ], + repository: { + owner: { + name: 'some-owner' + }, + name: 'some-repo' + } } } + inputs.pattern = 'some-pattern' inputs.error = 'some-error' + + const response = { + repository: { + object: { + message: 'some-message', + parents: { + totalCount: 1 + } + } + } + } + + graphqlResponse = response + const checkerArguments: ICheckerArguments = await inputHelper.getInputs() expect(checkerArguments).toBeTruthy() expect(checkerArguments.pattern).toBe('some-pattern') @@ -563,22 +595,55 @@ describe('input-helper tests', () => { payload: { commits: [ { + id: '1', message: 'some-message' }, { + id: '2', message: 'other-message' }, { - message: 'ignored-message', - parents: { - totalCount: 2 - } + id: '3', + message: 'ignored-message' } - ] + ], + repository: { + owner: { + name: 'some-owner' + }, + name: 'some-repo' + } } } + inputs.pattern = 'some-pattern' inputs.error = 'some-error' + + getGraphqlResponse = (parameters?: any): any => { + if (parameters.commitSha === '3') { + return { + repository: { + object: { + message: 'ignored-message', + parents: { + totalCount: 2 + } + } + } + } + } + + return { + repository: { + object: { + parents: { + totalCount: 1 + } + } + } + } + } + const checkerArguments: ICheckerArguments = await inputHelper.getInputs() expect(checkerArguments).toBeTruthy() expect(checkerArguments.pattern).toBe('some-pattern') diff --git a/dist/index.js b/dist/index.js index 0985ef1..7736b3d 100644 --- a/dist/index.js +++ b/dist/index.js @@ -236,7 +236,7 @@ exports.getInputs = getInputs; * @returns string[] */ function getMessages(pullRequestOptions) { - var _a, _b, _c; + var _a, _b; return __awaiter(this, void 0, void 0, function* () { core.debug('Get messages...'); core.debug(` - pullRequestOptions: ${JSON.stringify(pullRequestOptions, null, 2)}`); @@ -310,10 +310,21 @@ function getMessages(pullRequestOptions) { core.debug(' - skipping commits'); break; } + if (!github.context.payload.repository) { + throw new Error('No repository found in the payload.'); + } + if (!github.context.payload.repository.name) { + throw new Error('No name found in the repository.'); + } + if (!github.context.payload.repository.owner || + (!github.context.payload.repository.owner.login && + !github.context.payload.repository.owner.name)) { + throw new Error('No owner found in the repository.'); + } for (const i in github.context.payload.commits) { if (github.context.payload.commits[i].message && // ignore merge commits - ((_c = (_b = github.context.payload.commits[i].parents) === null || _b === void 0 ? void 0 : _b.totalCount) !== null && _c !== void 0 ? _c : 1) === 1) { + !(yield isMergeCommit(pullRequestOptions.accessToken, (_b = github.context.payload.repository.owner.name) !== null && _b !== void 0 ? _b : github.context.payload.repository.owner.login, github.context.payload.repository.name, github.context.payload.commits[i].id))) { messages.push(github.context.payload.commits[i].message); } } @@ -386,6 +397,47 @@ function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, reposito return messages; }); } +function isMergeCommit(accessToken, repositoryOwner, repositoryName, commitSha) { + return __awaiter(this, void 0, void 0, function* () { + core.debug('Get messages from pull request...'); + core.debug(` - accessToken: ${accessToken}`); + core.debug(` - repositoryOwner: ${repositoryOwner}`); + core.debug(` - repositoryName: ${repositoryName}`); + core.debug(` - commitSha: ${commitSha}`); + const query = ` + query commit( + $repositoryOwner: String!, + $repositoryName: String!, + $commitSha: GitObjectID! + ) { + repository(owner: $repositoryOwner, name: $repositoryName) { + object(oid: $commitSha) { + ... on Commit { + message + parents(last: 1) { + totalCount + } + } + } + } + } +`; + const variables = { + baseUrl: process.env['GITHUB_API_URL'] || 'https://api.github.com', + repositoryOwner, + repositoryName, + commitSha, + headers: { + authorization: `token ${accessToken}` + } + }; + core.debug(` - query: ${query}`); + core.debug(` - variables: ${JSON.stringify(variables, null, 2)}`); + const response = yield (0, graphql_1.graphql)(query, variables); + core.debug(` - response: ${JSON.stringify(response, null, 2)}`); + return response.repository.object.parents.totalCount > 1; + }); +} /***/ }), diff --git a/src/input-helper.ts b/src/input-helper.ts index d5e8259..8384ce6 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -200,11 +200,33 @@ async function getMessages( break } + if (!github.context.payload.repository) { + throw new Error('No repository found in the payload.') + } + + if (!github.context.payload.repository.name) { + throw new Error('No name found in the repository.') + } + + if ( + !github.context.payload.repository.owner || + (!github.context.payload.repository.owner.login && + !github.context.payload.repository.owner.name) + ) { + throw new Error('No owner found in the repository.') + } + for (const i in github.context.payload.commits) { if ( github.context.payload.commits[i].message && // ignore merge commits - (github.context.payload.commits[i].parents?.totalCount ?? 1) === 1 + !(await isMergeCommit( + pullRequestOptions.accessToken, + github.context.payload.repository.owner.name ?? + github.context.payload.repository.owner.login, + github.context.payload.repository.name, + github.context.payload.commits[i].id + )) ) { messages.push(github.context.payload.commits[i].message) } @@ -311,3 +333,64 @@ async function getCommitMessagesFromPullRequest( return messages } + +async function isMergeCommit( + accessToken: string, + repositoryOwner: string, + repositoryName: string, + commitSha: string +): Promise { + core.debug('Get messages from pull request...') + core.debug(` - accessToken: ${accessToken}`) + core.debug(` - repositoryOwner: ${repositoryOwner}`) + core.debug(` - repositoryName: ${repositoryName}`) + core.debug(` - commitSha: ${commitSha}`) + + const query = ` + query commit( + $repositoryOwner: String!, + $repositoryName: String!, + $commitSha: GitObjectID! + ) { + repository(owner: $repositoryOwner, name: $repositoryName) { + object(oid: $commitSha) { + ... on Commit { + message + parents(last: 1) { + totalCount + } + } + } + } + } +` + const variables = { + baseUrl: process.env['GITHUB_API_URL'] || 'https://api.github.com', + repositoryOwner, + repositoryName, + commitSha, + headers: { + authorization: `token ${accessToken}` + } + } + + core.debug(` - query: ${query}`) + core.debug(` - variables: ${JSON.stringify(variables, null, 2)}`) + + interface CommitResponseData { + repository: { + object: { + message: string + parents: { + totalCount: number + } + } + } + } + + const response = await graphql(query, variables) + + core.debug(` - response: ${JSON.stringify(response, null, 2)}`) + + return response.repository.object.parents.totalCount > 1 +} From e7447893580e0d99f50e51350fab044b6cac488b Mon Sep 17 00:00:00 2001 From: Simon Gilli <25326036+gilbertsoft@users.noreply.github.com> Date: Fri, 21 Oct 2022 14:03:11 +0200 Subject: [PATCH 7/7] [TASK] Add excludeMergeCommits option --- README.md | 9 +- __tests__/input-helper.test.ts | 365 ++++++++++++++++++++++++++++----- action.yml | 4 + dist/index.js | 27 ++- package.json | 2 +- src/input-helper.ts | 53 +++-- 6 files changed, 388 insertions(+), 72 deletions(-) diff --git a/README.md b/README.md index e167fab..89af571 100644 --- a/README.md +++ b/README.md @@ -23,9 +23,9 @@ More information about `pattern` and `flags` can be found in the `flags` is optional and defaults to `gm`. -`excludeDescription`, `excludeTitle` and `checkAllCommitMessages` are optional. -Default behavior is to include the description and title and not check pull -request commit messages. +`excludeDescription`, `excludeTitle`, `excludeMergeCommits`, and +`checkAllCommitMessages` are optional. Default behavior is to include the +description, title, and merge commits, and not check pull request commit messages. ### Example Workflow @@ -67,8 +67,9 @@ jobs: error: 'The maximum line length of 74 characters is exceeded.' excludeDescription: 'true' # optional: this excludes the description body of a pull request excludeTitle: 'true' # optional: this excludes the title of a pull request + excludeMergeCommits: 'true' # optional: this excludes merge commits checkAllCommitMessages: 'true' # optional: this checks all commits associated with a pull request - accessToken: ${{ secrets.GITHUB_TOKEN }} # github access token is only required if checkAllCommitMessages is true + accessToken: ${{ secrets.GITHUB_TOKEN }} # github access token is only required if checkAllCommitMessages or excludeMergeCommits is true - name: Check for Resolves / Fixes uses: gsactions/commit-message-checker@v2 with: diff --git a/__tests__/input-helper.test.ts b/__tests__/input-helper.test.ts index 90fd671..90c62b5 100644 --- a/__tests__/input-helper.test.ts +++ b/__tests__/input-helper.test.ts @@ -79,39 +79,20 @@ describe('input-helper tests', () => { jest.resetModules() }) - it('requires pattern', async () => { + it('inputs: requires pattern', async () => { await expect(inputHelper.getInputs()).rejects.toThrow( 'Input required and not supplied: pattern' ) }) - it('requires error message', async () => { + it('inputs: requires error message', async () => { inputs.pattern = 'some-pattern' await expect(inputHelper.getInputs()).rejects.toThrow( 'Input required and not supplied: error' ) }) - it('requires event', async () => { - inputs.pattern = 'some-pattern' - inputs.error = 'some-error' - await expect(inputHelper.getInputs()).rejects.toThrow( - 'Event "undefined" is not supported.' - ) - }) - - it('requires valid event', async () => { - mockGitHub.context = { - eventName: 'some-event' - } - inputs.pattern = 'some-pattern' - inputs.error = 'some-error' - await expect(inputHelper.getInputs()).rejects.toThrow( - 'Event "some-event" is not supported.' - ) - }) - - it('sets pattern', async () => { + it('inputs: sets pattern', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -128,7 +109,7 @@ describe('input-helper tests', () => { expect(checkerArguments.pattern).toBe('some-pattern') }) - it('sets flags', async () => { + it('inputs: sets flags', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -145,7 +126,7 @@ describe('input-helper tests', () => { expect(checkerArguments.flags).toBe('abcdefgh') }) - it('sets error', async () => { + it('inputs: sets error', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -162,7 +143,26 @@ describe('input-helper tests', () => { expect(checkerArguments.error).toBe('some-error') }) - it('requires pull_request payload', async () => { + it('event: requires event', async () => { + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'Event "undefined" is not supported.' + ) + }) + + it('event: requires valid event', async () => { + mockGitHub.context = { + eventName: 'some-event' + } + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'Event "some-event" is not supported.' + ) + }) + + it('pull_request: requires payload', async () => { mockGitHub.context = { eventName: 'pull_request' } @@ -173,7 +173,7 @@ describe('input-helper tests', () => { ) }) - it('requires pull_request', async () => { + it('pull_request: requires pull_request payload', async () => { mockGitHub.context = { eventName: 'pull_request', payload: {} @@ -185,7 +185,7 @@ describe('input-helper tests', () => { ) }) - it('requires pull_request title', async () => { + it('pull_request: requires title', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -202,7 +202,7 @@ describe('input-helper tests', () => { ) }) - it('sets pull_request title', async () => { + it('pull_request: sets title', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -220,7 +220,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages[0]).toBe('some-title') }) - it('sets pull_request title and body', async () => { + it('pull_request: sets title and body', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -238,7 +238,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages[0]).toBe('some-title\n\nsome-body') }) - it('excludes pull_request body', async () => { + it('pull_request: excludes body', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -257,7 +257,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages[0]).toBe('some-title') }) - it('excludes pull_request title', async () => { + it('pull_request: excludes title', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -276,7 +276,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages[0]).toBe('some-body') }) - it('excludes pull_request title and body', async () => { + it('pull_request: excludes title and body', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -296,7 +296,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages.length).toBe(0) }) - it('requires accessToken', async () => { + it('pull_request: requires accessToken', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -314,7 +314,7 @@ describe('input-helper tests', () => { ) }) - it('requires pull_request number', async () => { + it('pull_request: requires number', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -333,7 +333,7 @@ describe('input-helper tests', () => { ) }) - it('requires repository', async () => { + it('pull_request: requires repository', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -353,7 +353,7 @@ describe('input-helper tests', () => { ) }) - it('requires repository name', async () => { + it('pull_request: requires repository name', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -374,7 +374,7 @@ describe('input-helper tests', () => { ) }) - it('requires repository owner (1)', async () => { + it('pull_request: requires repository owner (1)', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -397,7 +397,7 @@ describe('input-helper tests', () => { ) }) - it('requires repository owner (2)', async () => { + it('pull_request: requires repository owner (2)', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -421,7 +421,7 @@ describe('input-helper tests', () => { ) }) - it('sets pull_request commits', async () => { + it('pull_request: sets commits', async () => { mockGitHub.context = { eventName: 'pull_request', payload: { @@ -507,10 +507,98 @@ describe('input-helper tests', () => { expect(checkerArguments.pattern).toBe('some-pattern') expect(checkerArguments.error).toBe('some-error') expect(checkerArguments.messages).toBeTruthy() + expect(checkerArguments.messages.length).toBe(4) + }) + + it('pull_request: excludes merge commits', async () => { + mockGitHub.context = { + eventName: 'pull_request', + payload: { + pull_request: { + title: 'some-title', + body: 'some-body', + number: 1 + }, + repository: { + owner: { + name: 'some-owner' + }, + name: 'some-repo' + } + } + } + + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.excludeDescription = 'true' + inputs.excludeTitle = 'true' + inputs.excludeMergeCommits = 'true' + inputs.checkAllCommitMessages = 'true' + inputs.accessToken = 'some-token' + + const response = { + repository: { + pullRequest: { + commits: { + edges: [ + { + node: { + commit: { + message: + 'input: make input-helper functions async\n\nIn order to work with asynchronous call like an async http request\nin an easier way, the functions getInput and getMessages were\nconverted to async.', + parents: { + totalCount: 1 + } + } + } + }, + { + node: { + commit: { + message: + "input: PR options ignore title and check PR commits\n\nthis make it possible to igore partially or completely the PR payload.\nThe commits associated with the pull request can be checked instead of\nchecking the pull request payload. The parameter are:\n\n- excludeTitle: 'true | false'\n- excludeDescription: 'true | false'\n- checkAllCommitMessages: 'true | false'\n\nby default, all options comes false.", + parents: { + totalCount: 1 + } + } + } + }, + { + node: { + commit: { + message: + 'docs: include parameters excludeTitle, checkAllCommitMessages and accessToken\n\nCo-authored-by: Gilbertsoft <25326036+gilbertsoft@users.noreply.github.com>', + parents: { + totalCount: 1 + } + } + } + }, + { + node: { + commit: { + message: 'merge: merge commit to be ignored', + parents: { + totalCount: 2 + } + } + } + } + ] + } + } + } + } + + graphqlResponse = response + + const checkerArguments: ICheckerArguments = await inputHelper.getInputs() + expect(checkerArguments).toBeTruthy() + expect(checkerArguments.messages).toBeTruthy() expect(checkerArguments.messages.length).toBe(3) }) - it('require push payload', async () => { + it('push: requires payload property', async () => { mockGitHub.context = { eventName: 'push' } @@ -521,7 +609,7 @@ describe('input-helper tests', () => { ) }) - it('push payload is optional', async () => { + it('push: payload content is optional', async () => { mockGitHub.context = { eventName: 'push', payload: {} @@ -532,7 +620,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages).toHaveLength(0) }) - it('push payload commits is optional', async () => { + it('push: payload commits is optional', async () => { mockGitHub.context = { eventName: 'push', payload: { @@ -545,7 +633,97 @@ describe('input-helper tests', () => { expect(checkerArguments.messages).toHaveLength(0) }) - it('sets correct single push payload', async () => { + it('push: requires repository', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'some-message' + } + ] + } + } + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'No repository found in the payload.' + ) + }) + + it('push: requires repository name', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'some-message' + } + ], + repository: {} + } + } + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.checkAllCommitMessages = 'true' + inputs.accessToken = 'dummy-token' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'No name found in the repository.' + ) + }) + + it('push: requires repository owner (1)', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'some-message' + } + ], + repository: { + name: 'repository-name' + } + } + } + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.checkAllCommitMessages = 'true' + inputs.accessToken = 'dummy-token' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'No owner found in the repository.' + ) + }) + + it('push: requires repository owner (2)', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'some-message' + } + ], + repository: { + name: 'repository-name', + owner: {} + } + } + } + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.checkAllCommitMessages = 'true' + inputs.accessToken = 'dummy-token' + await expect(inputHelper.getInputs()).rejects.toThrow( + 'No owner found in the repository.' + ) + }) + + it('push: sets single commit correctly', async () => { mockGitHub.context = { eventName: 'push', payload: { @@ -589,7 +767,7 @@ describe('input-helper tests', () => { expect(checkerArguments.messages[0]).toBe('some-message') }) - it('sets correct multiple push payload', async () => { + it('push: sets multiple commits correctly', async () => { mockGitHub.context = { eventName: 'push', payload: { @@ -604,7 +782,7 @@ describe('input-helper tests', () => { }, { id: '3', - message: 'ignored-message' + message: 'merge-commit-message' } ], repository: { @@ -624,7 +802,7 @@ describe('input-helper tests', () => { return { repository: { object: { - message: 'ignored-message', + message: 'merge-commit-message', parents: { totalCount: 2 } @@ -649,8 +827,101 @@ describe('input-helper tests', () => { expect(checkerArguments.pattern).toBe('some-pattern') expect(checkerArguments.error).toBe('some-error') expect(checkerArguments.messages).toBeTruthy() - expect(checkerArguments.messages.length).toBe(2) + expect(checkerArguments.messages.length).toBe(3) expect(checkerArguments.messages[0]).toBe('some-message') expect(checkerArguments.messages[1]).toBe('other-message') }) + + it('push: requires accessToken to exclude merge commits', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'some-message' + } + ], + repository: { + owner: { + name: 'some-owner' + }, + name: 'some-repo' + } + } + } + + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.excludeMergeCommits = 'true' + + await expect(inputHelper.getInputs()).rejects.toThrow( + 'The `excludeMergeCommits` option requires a github access token.' + ) + }) + + it('push: excludes merge commits', async () => { + mockGitHub.context = { + eventName: 'push', + payload: { + commits: [ + { + id: '1', + message: 'merge-commit-message-1' + }, + { + id: '2', + message: 'some-message' + }, + { + id: '3', + message: 'merge-commit-message-2' + } + ], + repository: { + owner: { + name: 'some-owner' + }, + name: 'some-repo' + } + } + } + + inputs.pattern = 'some-pattern' + inputs.error = 'some-error' + inputs.excludeMergeCommits = 'true' + inputs.accessToken = 'some-token' + + getGraphqlResponse = (parameters?: any): any => { + if (parameters.commitSha === '2') { + return { + repository: { + object: { + message: 'some-message', + parents: { + totalCount: 1 + } + } + } + } + } + + return { + repository: { + object: { + parents: { + totalCount: 2 + } + } + } + } + } + + const checkerArguments: ICheckerArguments = await inputHelper.getInputs() + expect(checkerArguments).toBeTruthy() + expect(checkerArguments.pattern).toBe('some-pattern') + expect(checkerArguments.error).toBe('some-error') + expect(checkerArguments.messages).toBeTruthy() + expect(checkerArguments.messages.length).toBe(1) + }) }) diff --git a/action.yml b/action.yml index cbce124..9b7f5a7 100644 --- a/action.yml +++ b/action.yml @@ -20,6 +20,10 @@ inputs: description: 'Setting this input to true will exclude the Pull Request description from the check.' required: false default: 'false' + excludeMergeCommits: + description: 'Setting this input to true will exclude merge commits from the check.' + required: false + default: 'false' checkAllCommitMessages: description: 'Setting this input to true will check all Pull Request commits' required: false diff --git a/dist/index.js b/dist/index.js index 7736b3d..0ce76f4 100644 --- a/dist/index.js +++ b/dist/index.js @@ -206,6 +206,9 @@ function getInputs() { // Get excludeDescription const excludeDescriptionStr = core.getInput('excludeDescription'); core.debug(`excludeDescription: ${excludeDescriptionStr}`); + // Get excludeMergeCommits + const excludeMergeCommitsStr = core.getInput('excludeMergeCommits'); + core.debug(`excludeDescription: ${excludeMergeCommitsStr}`); // Get checkAllCommitMessages const checkAllCommitMessagesStr = core.getInput('checkAllCommitMessages'); core.debug(`checkAllCommitMessages: ${checkAllCommitMessagesStr}`); @@ -217,6 +220,9 @@ function getInputs() { ignoreDescription: excludeDescriptionStr ? excludeDescriptionStr === 'true' : /* default */ false, + ignoreMergeCommits: excludeMergeCommitsStr + ? excludeMergeCommitsStr === 'true' + : /* default */ false, checkAllCommitMessages: checkAllCommitMessagesStr ? checkAllCommitMessagesStr === 'true' : /* default */ false, @@ -292,7 +298,7 @@ function getMessages(pullRequestOptions) { !github.context.payload.repository.owner.name)) { throw new Error('No owner found in the repository.'); } - const commitMessages = yield getCommitMessagesFromPullRequest(pullRequestOptions.accessToken, (_a = github.context.payload.repository.owner.name) !== null && _a !== void 0 ? _a : github.context.payload.repository.owner.login, github.context.payload.repository.name, github.context.payload.pull_request.number); + const commitMessages = yield getCommitMessagesFromPullRequest(pullRequestOptions.accessToken, (_a = github.context.payload.repository.owner.name) !== null && _a !== void 0 ? _a : github.context.payload.repository.owner.login, github.context.payload.repository.name, github.context.payload.pull_request.number, pullRequestOptions.ignoreMergeCommits); for (message of commitMessages) { if (message) { messages.push(message); @@ -321,10 +327,19 @@ function getMessages(pullRequestOptions) { !github.context.payload.repository.owner.name)) { throw new Error('No owner found in the repository.'); } + if (pullRequestOptions.ignoreMergeCommits) { + if (!pullRequestOptions.accessToken) { + throw new Error('The `excludeMergeCommits` option requires a github access token.'); + } + } for (const i in github.context.payload.commits) { - if (github.context.payload.commits[i].message && - // ignore merge commits - !(yield isMergeCommit(pullRequestOptions.accessToken, (_b = github.context.payload.repository.owner.name) !== null && _b !== void 0 ? _b : github.context.payload.repository.owner.login, github.context.payload.repository.name, github.context.payload.commits[i].id))) { + if (github.context.payload.commits[i].message) { + // ignore merge commits if requested + if (pullRequestOptions.ignoreMergeCommits && + (yield isMergeCommit(pullRequestOptions.accessToken, (_b = github.context.payload.repository.owner.name) !== null && _b !== void 0 ? _b : github.context.payload.repository.owner.login, github.context.payload.repository.name, github.context.payload.commits[i].id))) { + core.debug(` - skipping merge commit ${github.context.payload.commits[i].id}`); + continue; + } messages.push(github.context.payload.commits[i].message); } } @@ -337,7 +352,7 @@ function getMessages(pullRequestOptions) { return messages; }); } -function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, repositoryName, pullRequestNumber) { +function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, repositoryName, pullRequestNumber, ignoreMergeCommits) { return __awaiter(this, void 0, void 0, function* () { core.debug('Get messages from pull request...'); core.debug(` - accessToken: ${accessToken}`); @@ -388,7 +403,7 @@ function getCommitMessagesFromPullRequest(accessToken, repositoryOwner, reposito messages = repository.pullRequest.commits.edges .filter(function (edge) { // Skip merge commits (which have more than 1 parent commit) - return edge.node.commit.parents.totalCount === 1; + return !ignoreMergeCommits || edge.node.commit.parents.totalCount === 1; }) .map(function (edge) { return edge.node.commit.message; diff --git a/package.json b/package.json index 68967c8..b6f2562 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "format-check": "prettier --check '**/*.ts'", "lint": "eslint src/**/*.ts", "package": "ncc build", - "test": "jest", + "test": "jest --coverage", "all": "npm run build && npm run format && npm run lint && npm run package && npm test" }, "dependencies": { diff --git a/src/input-helper.ts b/src/input-helper.ts index 8384ce6..158ed4a 100644 --- a/src/input-helper.ts +++ b/src/input-helper.ts @@ -26,6 +26,7 @@ import {ICheckerArguments} from './commit-message-checker' export interface PullRequestOptions { ignoreTitle: boolean ignoreDescription: boolean + ignoreMergeCommits: boolean // requires github token checkAllCommitMessages: boolean // requires github token accessToken: string } @@ -60,6 +61,10 @@ export async function getInputs(): Promise { const excludeDescriptionStr = core.getInput('excludeDescription') core.debug(`excludeDescription: ${excludeDescriptionStr}`) + // Get excludeMergeCommits + const excludeMergeCommitsStr = core.getInput('excludeMergeCommits') + core.debug(`excludeDescription: ${excludeMergeCommitsStr}`) + // Get checkAllCommitMessages const checkAllCommitMessagesStr = core.getInput('checkAllCommitMessages') core.debug(`checkAllCommitMessages: ${checkAllCommitMessagesStr}`) @@ -72,6 +77,9 @@ export async function getInputs(): Promise { ignoreDescription: excludeDescriptionStr ? excludeDescriptionStr === 'true' : /* default */ false, + ignoreMergeCommits: excludeMergeCommitsStr + ? excludeMergeCommitsStr === 'true' + : /* default */ false, checkAllCommitMessages: checkAllCommitMessagesStr ? checkAllCommitMessagesStr === 'true' : /* default */ false, @@ -175,7 +183,8 @@ async function getMessages( github.context.payload.repository.owner.name ?? github.context.payload.repository.owner.login, github.context.payload.repository.name, - github.context.payload.pull_request.number + github.context.payload.pull_request.number, + pullRequestOptions.ignoreMergeCommits ) for (message of commitMessages) { @@ -216,18 +225,33 @@ async function getMessages( throw new Error('No owner found in the repository.') } + if (pullRequestOptions.ignoreMergeCommits) { + if (!pullRequestOptions.accessToken) { + throw new Error( + 'The `excludeMergeCommits` option requires a github access token.' + ) + } + } + for (const i in github.context.payload.commits) { - if ( - github.context.payload.commits[i].message && - // ignore merge commits - !(await isMergeCommit( - pullRequestOptions.accessToken, - github.context.payload.repository.owner.name ?? - github.context.payload.repository.owner.login, - github.context.payload.repository.name, - github.context.payload.commits[i].id - )) - ) { + if (github.context.payload.commits[i].message) { + // ignore merge commits if requested + if ( + pullRequestOptions.ignoreMergeCommits && + (await isMergeCommit( + pullRequestOptions.accessToken, + github.context.payload.repository.owner.name ?? + github.context.payload.repository.owner.login, + github.context.payload.repository.name, + github.context.payload.commits[i].id + )) + ) { + core.debug( + ` - skipping merge commit ${github.context.payload.commits[i].id}` + ) + continue + } + messages.push(github.context.payload.commits[i].message) } } @@ -246,7 +270,8 @@ async function getCommitMessagesFromPullRequest( accessToken: string, repositoryOwner: string, repositoryName: string, - pullRequestNumber: number + pullRequestNumber: number, + ignoreMergeCommits: boolean ): Promise { core.debug('Get messages from pull request...') core.debug(` - accessToken: ${accessToken}`) @@ -324,7 +349,7 @@ async function getCommitMessagesFromPullRequest( messages = repository.pullRequest.commits.edges .filter(function (edge: CommitEdgeItem): boolean { // Skip merge commits (which have more than 1 parent commit) - return edge.node.commit.parents.totalCount === 1 + return !ignoreMergeCommits || edge.node.commit.parents.totalCount === 1 }) .map(function (edge: CommitEdgeItem): string { return edge.node.commit.message