From 60f0b73c2fcbef0148794e010d9c68ecfc4d1586 Mon Sep 17 00:00:00 2001 From: Jeff Ching Date: Mon, 20 Dec 2021 16:27:42 -0800 Subject: [PATCH] fix: only backfill files if requested --- __snapshots__/github.js | 102 +++++++++++++++++------------------ src/commit.ts | 2 +- src/github.ts | 25 ++++++--- src/manifest.ts | 3 +- src/strategies/ruby-yoshi.ts | 5 ++ src/util/commit-split.ts | 5 ++ test/github.ts | 9 ++-- 7 files changed, 84 insertions(+), 67 deletions(-) diff --git a/__snapshots__/github.js b/__snapshots__/github.js index 784822f39..2fa8464f0 100644 --- a/__snapshots__/github.js +++ b/__snapshots__/github.js @@ -6,7 +6,6 @@ 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, @@ -16,7 +15,8 @@ exports['GitHub commitsSince backfills commit files without pull requests 1'] = "body": "", "labels": [], "files": [] - } + }, + "files": [] } ] @@ -24,7 +24,6 @@ exports['GitHub commitsSince finds commits up until a condition 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, @@ -34,7 +33,8 @@ exports['GitHub commitsSince finds commits up until a condition 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] } ] @@ -42,7 +42,6 @@ exports['GitHub commitsSince finds first commit of a multi-commit merge pull req { "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, @@ -52,12 +51,12 @@ exports['GitHub commitsSince finds first commit of a multi-commit merge pull req "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "message": "feat: feature-branch-plain-merge commit 2", - "files": [], "pullRequest": { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "number": 7, @@ -67,12 +66,12 @@ exports['GitHub commitsSince finds first commit of a multi-commit merge pull req "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "message": "feat: feature-branch-plain-merge commit 1", - "files": [], "pullRequest": { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "number": 7, @@ -82,7 +81,8 @@ exports['GitHub commitsSince finds first commit of a multi-commit merge pull req "body": "", "labels": [], "files": [] - } + }, + "files": [] } ] @@ -90,7 +90,6 @@ exports['GitHub commitsSince limits pagination 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, @@ -100,12 +99,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "message": "feat: feature-branch-plain-merge commit 2", - "files": [], "pullRequest": { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "number": 7, @@ -115,12 +114,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "message": "feat: feature-branch-plain-merge commit 1", - "files": [], "pullRequest": { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "number": 7, @@ -130,12 +129,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5", "message": "fix: feature-branch-merge fix 1", - "files": [], "pullRequest": { "sha": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5", "number": 6, @@ -145,12 +144,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "a257514a541d483425118d973674b1ce006a5489", "message": "chore: feature-branch-merge lint", - "files": [], "pullRequest": { "sha": "a257514a541d483425118d973674b1ce006a5489", "number": 6, @@ -160,12 +159,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "b6a8ab1a50106cfb03f22c2cdaf7abfdcccce088", "message": "feat: feature-branch-merge commit 2", - "files": [], "pullRequest": { "sha": "b6a8ab1a50106cfb03f22c2cdaf7abfdcccce088", "number": 6, @@ -175,12 +174,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "520b6f42551c86002197d033564a76a3f99b0019", "message": "feat: feature-branch-merge commit 1", - "files": [], "pullRequest": { "sha": "520b6f42551c86002197d033564a76a3f99b0019", "number": 6, @@ -190,12 +189,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "9dda1a331d311d0a7643015cc9e6802548c8d943", "message": "chore(main): release 0.1.1-SNAPSHOT (#3)", - "files": [], "pullRequest": { "sha": "9dda1a331d311d0a7643015cc9e6802548c8d943", "number": 3, @@ -207,12 +206,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "type: process" ], "files": [] - } + }, + "files": [] }, { "sha": "e86984fb22ccc5eafb6c3d815851ade3463193da", "message": "feat: feature-branch that will be squash merged (#2)\n\n* feat: feature-branch commit 1\r\n\r\n* feat: feature-branch commit 2\r\n\r\n* chore: fix lint\r\n\r\n* fix: feature-branch fix 1", - "files": [], "pullRequest": { "sha": "e86984fb22ccc5eafb6c3d815851ade3463193da", "number": 2, @@ -222,12 +221,12 @@ exports['GitHub commitsSince limits pagination 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "0cda26c2e7776748072ba5a24302474947b3ebbd", - "message": "build: add release-please bot", - "files": [] + "message": "build: add release-please bot" } ] @@ -235,7 +234,6 @@ exports['GitHub commitsSince paginates through commits 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, @@ -245,12 +243,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "message": "feat: feature-branch-plain-merge commit 2", - "files": [], "pullRequest": { "sha": "b29149f890e6f76ee31ed128585744d4c598924c", "number": 7, @@ -260,12 +258,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "message": "feat: feature-branch-plain-merge commit 1", - "files": [], "pullRequest": { "sha": "27d7d7232e2e312d1380e906984f0823f5decf61", "number": 7, @@ -275,12 +273,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5", "message": "fix: feature-branch-merge fix 1", - "files": [], "pullRequest": { "sha": "2b4e0b3be2e231cd87cc44c411bd8f84b4587ab5", "number": 6, @@ -290,12 +288,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "a257514a541d483425118d973674b1ce006a5489", "message": "chore: feature-branch-merge lint", - "files": [], "pullRequest": { "sha": "a257514a541d483425118d973674b1ce006a5489", "number": 6, @@ -305,12 +303,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "b6a8ab1a50106cfb03f22c2cdaf7abfdcccce088", "message": "feat: feature-branch-merge commit 2", - "files": [], "pullRequest": { "sha": "b6a8ab1a50106cfb03f22c2cdaf7abfdcccce088", "number": 6, @@ -320,12 +318,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "520b6f42551c86002197d033564a76a3f99b0019", "message": "feat: feature-branch-merge commit 1", - "files": [], "pullRequest": { "sha": "520b6f42551c86002197d033564a76a3f99b0019", "number": 6, @@ -335,12 +333,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "9dda1a331d311d0a7643015cc9e6802548c8d943", "message": "chore(main): release 0.1.1-SNAPSHOT (#3)", - "files": [], "pullRequest": { "sha": "9dda1a331d311d0a7643015cc9e6802548c8d943", "number": 3, @@ -352,12 +350,12 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "type: process" ], "files": [] - } + }, + "files": [] }, { "sha": "e86984fb22ccc5eafb6c3d815851ade3463193da", "message": "feat: feature-branch that will be squash merged (#2)\n\n* feat: feature-branch commit 1\r\n\r\n* feat: feature-branch commit 2\r\n\r\n* chore: fix lint\r\n\r\n* fix: feature-branch fix 1", - "files": [], "pullRequest": { "sha": "e86984fb22ccc5eafb6c3d815851ade3463193da", "number": 2, @@ -367,17 +365,16 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "body": "", "labels": [], "files": [] - } + }, + "files": [] }, { "sha": "0cda26c2e7776748072ba5a24302474947b3ebbd", - "message": "build: add release-please bot", - "files": [] + "message": "build: add release-please bot" }, { "sha": "959ee48c95f254300eb040c46ebdc8248317efe4", "message": "Release v0.1.0 (#1)", - "files": [], "pullRequest": { "sha": "959ee48c95f254300eb040c46ebdc8248317efe4", "number": 1, @@ -389,7 +386,8 @@ exports['GitHub commitsSince paginates through commits 1'] = [ "autorelease: tagged" ], "files": [] - } + }, + "files": [] } ] diff --git a/src/commit.ts b/src/commit.ts index 8a99f8563..257895f3d 100644 --- a/src/commit.ts +++ b/src/commit.ts @@ -28,7 +28,7 @@ const conventionalCommitsFilter = require('conventional-commits-filter'); export interface Commit { sha: string; message: string; - files: string[]; + files?: string[]; pullRequest?: PullRequest; } diff --git a/src/github.ts b/src/github.ts index 2e5063757..1c12e4475 100644 --- a/src/github.ts +++ b/src/github.ts @@ -242,16 +242,23 @@ export class GitHub { * commit/pull request matches certain criteria * @param {number} maxResults Limit the number of results searched. * Defaults to unlimited. + * @param {boolean} backfillFiles If set, use the REST API for fetching + * the list of touched files in this commit. Defaults to `false`. * @returns {Commit[]} List of commits to current branch * @throws {GitHubAPIError} on an API error */ async commitsSince( targetBranch: string, filter: CommitFilter, - maxResults: number = Number.MAX_SAFE_INTEGER + maxResults: number = Number.MAX_SAFE_INTEGER, + backfillFiles = false ): Promise { const commits: Commit[] = []; - const generator = this.mergeCommitIterator(targetBranch, maxResults); + const generator = this.mergeCommitIterator( + targetBranch, + maxResults, + backfillFiles + ); for await (const commit of generator) { if (filter(commit)) { break; @@ -267,19 +274,23 @@ export class GitHub { * @param {string} targetBranch target branch of commit * @param {number} maxResults maxResults - Limit the number of results searched. * Defaults to unlimited. + * @param {boolean} backfillFiles If set, use the REST API for fetching + * the list of touched files in this commit. Defaults to `false`. * @yields {Commit} * @throws {GitHubAPIError} on an API error */ async *mergeCommitIterator( targetBranch: string, - maxResults: number = Number.MAX_SAFE_INTEGER + maxResults: number = Number.MAX_SAFE_INTEGER, + backfillFiles = false ) { let cursor: string | undefined = undefined; let results = 0; while (results < maxResults) { const response: CommitHistory | null = await this.mergeCommitsGraphQL( targetBranch, - cursor + cursor, + backfillFiles ); // no response usually means that the branch can't be found if (!response) { @@ -298,7 +309,8 @@ export class GitHub { private async mergeCommitsGraphQL( targetBranch: string, - cursor?: string + cursor?: string, + backfillFiles = false ): Promise { logger.debug( `Fetching merge commits on branch ${targetBranch} with cursor: ${cursor}` @@ -372,7 +384,6 @@ export class GitHub { 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; @@ -392,7 +403,7 @@ export class GitHub { // We cannot directly fetch files on commits via graphql, only provide file // information for commits with associated pull requests commit.files = files; - } else { + } else if (backfillFiles) { // 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 diff --git a/src/manifest.ts b/src/manifest.ts index 92a883394..17d539d95 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -407,7 +407,8 @@ export class Manifest { const commits: Commit[] = []; const commitGenerator = this.github.mergeCommitIterator( this.targetBranch, - 500 + 500, + true // backfill commit files ); const releaseShas = new Set(Object.values(releaseShasByPath)); logger.debug(releaseShas); diff --git a/src/strategies/ruby-yoshi.ts b/src/strategies/ruby-yoshi.ts index 8b55b4f2b..546a2fc5f 100644 --- a/src/strategies/ruby-yoshi.ts +++ b/src/strategies/ruby-yoshi.ts @@ -28,6 +28,7 @@ import {Release} from '../release'; import {Version} from '../version'; import {TagName} from '../util/tag-name'; import {ROOT_PROJECT_PATH} from '../manifest'; +import {logger} from '../util/logger'; const CHANGELOG_SECTIONS = [ {type: 'feat', section: 'Features'}, @@ -135,6 +136,10 @@ export class RubyYoshi extends BaseStrategy { .slice(1) .join('\n')}\n`; } + if (commit.files === undefined) { + logger.error('No files for commit - this is likely a bug.'); + continue; + } commit.files.forEach(file => { if (this.path === ROOT_PROJECT_PATH || file.startsWith(this.path)) { updatedFiles[file] = true; diff --git a/src/util/commit-split.ts b/src/util/commit-split.ts index 3785b91ee..631293a3d 100644 --- a/src/util/commit-split.ts +++ b/src/util/commit-split.ts @@ -85,6 +85,11 @@ export class CommitSplit { split(commits: T[]): Record { const splitCommits: Record = {}; commits.forEach(commit => { + if (commit.files === undefined) { + throw new Error( + `Commit ${commit.sha} is missing files. Did you set "backfillFiles" to "true"?` + ); + } const dedupe: Set = new Set(); for (let i = 0; i < commit.files.length; i++) { const file: string = commit.files[i]; diff --git a/test/github.ts b/test/github.ts index 8f0e00430..1e437e894 100644 --- a/test/github.ts +++ b/test/github.ts @@ -292,7 +292,6 @@ 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') ); @@ -313,7 +312,6 @@ 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') ); @@ -343,7 +341,6 @@ 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') ); @@ -364,7 +361,6 @@ describe('GitHub', () => { }); it('limits pagination', async () => { - sandbox.stub(github, 'getCommitFiles').resolves([]); const graphql1 = JSON.parse( readFileSync(resolve(fixturesPath, 'commits-since-page-1.json'), 'utf8') ); @@ -386,7 +382,6 @@ 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'), @@ -434,7 +429,9 @@ describe('GitHub', () => { commit => { // this commit is the 2nd most recent return commit.sha === 'b29149f890e6f76ee31ed128585744d4c598924c'; - } + }, + Number.MAX_SAFE_INTEGER, + true ); expect(commitsSinceSha.length).to.eql(1); snapshot(commitsSinceSha);