diff --git a/__snapshots__/github.js b/__snapshots__/github.js index 4b6d9d883..c43b3ff46 100644 --- a/__snapshots__/github.js +++ b/__snapshots__/github.js @@ -2,6 +2,24 @@ exports['GitHub commentOnIssue can create a comment 1'] = { "body": "This is a comment" } +exports['GitHub commitsSince backfills commit files without pull requests 1'] = [ + { + "sha": "e6daec403626c9987c7af0d97b34f324cd84320a", + "message": "Merge pull request #7 from chingor13/feature-branch-plain-merge\n\nfeat: feature that will be plain merged", + "files": [], + "pullRequest": { + "sha": "e6daec403626c9987c7af0d97b34f324cd84320a", + "number": 7, + "baseBranchName": "main", + "headBranchName": "feature-branch-plain-merge", + "title": "feat: feature that will be plain merged", + "body": "", + "labels": [], + "files": [] + } + } +] + exports['GitHub commitsSince finds commits up until a condition 1'] = [ { "sha": "e6daec403626c9987c7af0d97b34f324cd84320a", diff --git a/src/github.ts b/src/github.ts index e27862fd4..a310a053c 100644 --- a/src/github.ts +++ b/src/github.ts @@ -365,44 +365,64 @@ export class GitHub { } const history = response.repository.ref.target.history; const commits = (history.nodes || []) as GraphQLCommit[]; + const commitData: Commit[] = []; + for (const graphCommit of commits) { + const commit: Commit = { + sha: graphCommit.sha, + message: graphCommit.message, + files: [], + }; + const pullRequest = graphCommit.associatedPullRequests.nodes.find(pr => { + return pr.mergeCommit && pr.mergeCommit.oid === graphCommit.sha; + }); + if (pullRequest) { + const files = pullRequest.files.nodes.map(node => node.path); + commit.pullRequest = { + sha: commit.sha, + number: pullRequest.number, + baseBranchName: pullRequest.baseRefName, + headBranchName: pullRequest.headRefName, + title: pullRequest.title, + body: pullRequest.body, + labels: pullRequest.labels.nodes.map(node => node.name), + files, + }; + // We cannot directly fetch files on commits via graphql, only provide file + // information for commits with associated pull requests + commit.files = files; + } else { + // In this case, there is no squashed merge commit. This could be a simple + // merge commit, a rebase merge commit, or a direct commit to the branch. + // Fallback to fetching the list of commits from the REST API. In the future + // we can perhaps lazy load these. + commit.files = await this.getCommitFiles(graphCommit.sha); + } + commitData.push(commit); + } return { pageInfo: history.pageInfo, - data: commits.map(graphCommit => { - const commit: Commit = { - sha: graphCommit.sha, - message: graphCommit.message, - files: [], - }; - const pullRequest = graphCommit.associatedPullRequests.nodes.find( - pr => { - return pr.mergeCommit && pr.mergeCommit.oid === graphCommit.sha; - } - ); - if (pullRequest) { - const files = pullRequest.files.nodes.map(node => node.path); - commit.pullRequest = { - sha: commit.sha, - number: pullRequest.number, - baseBranchName: pullRequest.baseRefName, - headBranchName: pullRequest.headRefName, - title: pullRequest.title, - body: pullRequest.body, - labels: pullRequest.labels.nodes.map(node => node.name), - files, - }; - // We cannot directly fetch files on commits via graphql, only provide file - // information for commits with associated pull requests - commit.files = files; - } else { - logger.warn( - `No merged pull request for commit: ${graphCommit.sha} - files unavailable` - ); - } - return commit; - }), + data: commitData, }; } + /** + * Get the list of file paths modified in a given commit. + * + * @param {string} sha The commit SHA + * @returns {string[]} File paths + * @throws {GitHubAPIError} on an API error + */ + getCommitFiles = wrapAsync(async (sha: string): Promise => { + logger.debug(`Backfilling file list for commit: ${sha}`); + const resp = await this.octokit.repos.getCommit({ + owner: this.repository.owner, + repo: this.repository.repo, + ref: sha, + }); + const files = resp.data.files || []; + return files.map(file => file.filename!).filter(filename => !!filename); + }); + private graphqlRequest = wrapAsync( async ( opts: { diff --git a/src/manifest.ts b/src/manifest.ts index d4258d062..eb7f90a57 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -768,7 +768,6 @@ export class Manifest { ); } this._pathsByComponent[component] = path; - logger.info(this._pathsByComponent); } } return this._pathsByComponent; diff --git a/test/github.ts b/test/github.ts index 4407eae47..4a3007fc9 100644 --- a/test/github.ts +++ b/test/github.ts @@ -14,12 +14,13 @@ import * as nock from 'nock'; import {expect} from 'chai'; -import {beforeEach, describe, it} from 'mocha'; +import {afterEach, beforeEach, describe, it} from 'mocha'; nock.disableNetConnect(); import {readFileSync} from 'fs'; import {resolve} from 'path'; import * as snapshot from 'snap-shot-it'; +import * as sinon from 'sinon'; import {GitHub, GitHubRelease} from '../src/github'; import {PullRequest} from '../src/pull-request'; @@ -30,6 +31,7 @@ import {DuplicateReleaseError, GitHubAPIError} from '../src/errors'; import {fail} from 'assert'; const fixturesPath = './test/fixtures'; +const sandbox = sinon.createSandbox(); describe('GitHub', () => { let github: GitHub; @@ -56,6 +58,9 @@ describe('GitHub', () => { // This shared nock will take care of some common requests. req = getNock(); }); + afterEach(() => { + sandbox.restore(); + }); describe('create', () => { it('allows configuring the default branch explicitly', async () => { @@ -287,6 +292,7 @@ describe('GitHub', () => { describe('commitsSince', () => { it('finds commits up until a condition', async () => { + sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql = JSON.parse( readFileSync(resolve(fixturesPath, 'commits-since.json'), 'utf8') ); @@ -307,6 +313,7 @@ describe('GitHub', () => { }); it('paginates through commits', async () => { + sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql1 = JSON.parse( readFileSync(resolve(fixturesPath, 'commits-since-page-1.json'), 'utf8') ); @@ -336,6 +343,7 @@ describe('GitHub', () => { }); it('finds first commit of a multi-commit merge pull request', async () => { + sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql = JSON.parse( readFileSync(resolve(fixturesPath, 'commits-since.json'), 'utf8') ); @@ -356,6 +364,7 @@ describe('GitHub', () => { }); it('limits pagination', async () => { + sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql1 = JSON.parse( readFileSync(resolve(fixturesPath, 'commits-since-page-1.json'), 'utf8') ); @@ -377,6 +386,7 @@ describe('GitHub', () => { }); it('returns empty commits if branch does not exist', async () => { + sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql = JSON.parse( readFileSync( resolve(fixturesPath, 'commits-since-missing-branch.json'), @@ -396,6 +406,51 @@ describe('GitHub', () => { expect(commitsSinceSha.length).to.eql(0); req.done(); }); + + it('backfills commit files without pull requests', async () => { + const graphql = JSON.parse( + readFileSync(resolve(fixturesPath, 'commits-since.json'), 'utf8') + ); + req + .post('/graphql') + .reply(200, { + data: graphql, + }) + .get( + '/repos/fake/fake/commits/0cda26c2e7776748072ba5a24302474947b3ebbd' + ) + .reply(200, {files: [{filename: 'abc'}]}) + .get( + '/repos/fake/fake/commits/c6d9dfb03aa2dbe1abc329592af60713fe28586d' + ) + .reply(200, {files: [{filename: 'def'}]}) + .get( + '/repos/fake/fake/commits/c8f1498c92c323bfa8f5ffe84e0ade1c37e4ea6e' + ) + .reply(200, {files: [{filename: 'ghi'}]}); + const targetBranch = 'main'; + const commitsSinceSha = await github.commitsSince( + targetBranch, + commit => { + // this commit is the 2nd most recent + return commit.sha === 'b29149f890e6f76ee31ed128585744d4c598924c'; + } + ); + expect(commitsSinceSha.length).to.eql(1); + snapshot(commitsSinceSha); + req.done(); + }); + }); + + describe('getCommitFiles', () => { + it('fetches the list of files', async () => { + req + .get('/repos/fake/fake/commits/abc123') + .reply(200, {files: [{filename: 'abc'}]}); + const files = await github.getCommitFiles('abc123'); + expect(files).to.eql(['abc']); + req.done(); + }); }); describe('releaseIterator', () => {