From f7e97f7065a470e2ea65fd6df73a495351acce44 Mon Sep 17 00:00:00 2001 From: Samuel El-Borai Date: Tue, 29 Aug 2023 15:25:14 +0200 Subject: [PATCH] Get rid of github wrappers assuming lookup from target branch --- src/github.ts | 81 +--------------------- src/manifest.ts | 3 +- src/strategies/helm.ts | 5 +- src/strategies/krm-blueprint.ts | 8 ++- src/strategies/ocaml.ts | 23 ++++-- src/util/pull-request-overflow-handler.ts | 5 +- test/github.ts | 32 +++++++-- test/helpers.ts | 5 +- test/plugins/maven-workspace.ts | 11 ++- test/strategies/base.ts | 12 +++- test/strategies/ocaml.ts | 16 +++-- test/util/pull-request-overflow-handler.ts | 22 +++--- 12 files changed, 102 insertions(+), 121 deletions(-) diff --git a/src/github.ts b/src/github.ts index 4da7c1a37..ada351d30 100644 --- a/src/github.ts +++ b/src/github.ts @@ -1079,20 +1079,6 @@ export class GitHub { } } - /** - * Fetch the contents of a file from the configured branch - * - * @param {string} path The path to the file in the repository - * @returns {GitHubFileContents} - * @throws {GitHubAPIError} on other API errors - */ - async getFileContents(path: string): Promise { - return await this.getFileContentsOnBranch( - path, - this.repository.defaultBranch - ); - } - /** * Fetch the contents of a file * @@ -1122,28 +1108,6 @@ export class GitHub { return JSON.parse(content.parsedContent); } - /** - * Returns a list of paths to all files with a given name. - * - * If a prefix is specified, only return paths that match - * the provided prefix. - * - * @param filename The name of the file to find - * @param prefix Optional path prefix used to filter results - * @returns {string[]} List of file paths - * @throws {GitHubAPIError} on an API error - */ - async findFilesByFilename( - filename: string, - prefix?: string - ): Promise { - return this.findFilesByFilenameAndRef( - filename, - this.repository.defaultBranch, - prefix - ); - } - /** * Returns a list of paths to all files with a given name. * @@ -1171,24 +1135,6 @@ export class GitHub { } ); - /** - * Returns a list of paths to all files matching a glob pattern. - * - * If a prefix is specified, only return paths that match - * the provided prefix. - * - * @param glob The glob to match - * @param prefix Optional path prefix used to filter results - * @returns {string[]} List of file paths - * @throws {GitHubAPIError} on an API error - */ - async findFilesByGlob(glob: string, prefix?: string): Promise { - return this.findFilesByGlobAndRef( - glob, - this.repository.defaultBranch, - prefix - ); - } /** * Returns a list of paths to all files matching a glob pattern. * @@ -1413,7 +1359,8 @@ export class GitHub { const body = ( options?.pullRequestOverflowHandler ? await options.pullRequestOverflowHandler.handleOverflow( - releasePullRequest + releasePullRequest, + baseBranch ) : releasePullRequest.body ) @@ -1535,30 +1482,6 @@ export class GitHub { } ); - /** - * Returns a list of paths to all files with a given file - * extension. - * - * If a prefix is specified, only return paths that match - * the provided prefix. - * - * @param extension The file extension used to filter results. - * Example: `js`, `java` - * @param prefix Optional path prefix used to filter results - * @returns {string[]} List of file paths - * @throws {GitHubAPIError} on an API error - */ - async findFilesByExtension( - extension: string, - prefix?: string - ): Promise { - return this.findFilesByExtensionAndRef( - extension, - this.repository.defaultBranch, - prefix - ); - } - /** * Create a GitHub release * diff --git a/src/manifest.ts b/src/manifest.ts index 4a1dd1236..4e0a400cd 100644 --- a/src/manifest.ts +++ b/src/manifest.ts @@ -961,7 +961,8 @@ export class Manifest { } const body = await this.pullRequestOverflowHandler.handleOverflow( - pullRequest + pullRequest, + this.changesBranch ); const message = this.signoffUser ? signoffCommitMessage(pullRequest.title.toString(), this.signoffUser) diff --git a/src/strategies/helm.ts b/src/strategies/helm.ts index fbc847566..7a473bc21 100644 --- a/src/strategies/helm.ts +++ b/src/strategies/helm.ts @@ -65,8 +65,9 @@ export class Helm extends BaseStrategy { private async getChartYmlContents(): Promise { if (!this.chartYmlContents) { try { - this.chartYmlContents = await this.github.getFileContents( - this.addPath('Chart.yaml') + this.chartYmlContents = await this.github.getFileContentsOnBranch( + this.addPath('Chart.yaml'), + this.changesBranch ); } catch (e) { if (e instanceof FileNotFoundError) { diff --git a/src/strategies/krm-blueprint.ts b/src/strategies/krm-blueprint.ts index e6abac6d4..aa1dda879 100644 --- a/src/strategies/krm-blueprint.ts +++ b/src/strategies/krm-blueprint.ts @@ -54,9 +54,11 @@ export class KRMBlueprint extends BaseStrategy { this.path ); for (const yamlPath of yamlPaths) { - const contents: GitHubFileContents = await this.github.getFileContents( - this.addPath(yamlPath) - ); + const contents: GitHubFileContents = + await this.github.getFileContentsOnBranch( + this.addPath(yamlPath), + this.changesBranch + ); if (hasKRMBlueprintAttrib(contents.parsedContent)) { updates.push({ path: this.addPath(yamlPath), diff --git a/src/strategies/ocaml.ts b/src/strategies/ocaml.ts index 95051d283..afeb6b211 100644 --- a/src/strategies/ocaml.ts +++ b/src/strategies/ocaml.ts @@ -41,12 +41,18 @@ export class OCaml extends BaseStrategy { }), }); - const jsonPaths = await this.github.findFilesByExtension('json', this.path); + const jsonPaths = await this.github.findFilesByExtensionAndRef( + 'json', + this.changesBranch, + this.path + ); for (const path of jsonPaths) { if (notEsyLock(path)) { - const contents: GitHubFileContents = await this.github.getFileContents( - this.addPath(path) - ); + const contents: GitHubFileContents = + await this.github.getFileContentsOnBranch( + this.addPath(path), + this.changesBranch + ); const pkg = JSON.parse(contents.parsedContent); if (pkg.version !== undefined) { updates.push({ @@ -61,7 +67,11 @@ export class OCaml extends BaseStrategy { } } - const opamPaths = await this.github.findFilesByExtension('opam', this.path); + const opamPaths = await this.github.findFilesByExtensionAndRef( + 'opam', + this.changesBranch, + this.path + ); opamPaths.filter(notEsyLock).forEach(path => { updates.push({ path: this.addPath(path), @@ -72,8 +82,9 @@ export class OCaml extends BaseStrategy { }); }); - const opamLockedPaths = await this.github.findFilesByExtension( + const opamLockedPaths = await this.github.findFilesByExtensionAndRef( 'opam.locked', + this.changesBranch, this.path ); opamLockedPaths.filter(notEsyLock).forEach(path => { diff --git a/src/util/pull-request-overflow-handler.ts b/src/util/pull-request-overflow-handler.ts index 43bc85245..e41f1bf79 100644 --- a/src/util/pull-request-overflow-handler.ts +++ b/src/util/pull-request-overflow-handler.ts @@ -37,11 +37,13 @@ export interface PullRequestOverflowHandler { * If a pull request's body is too big, store it somewhere and return * a new pull request body with information about the new location. * @param {ReleasePullRequest} pullRequest The candidate release pull request + * @param {string} baseBranch The branch to use as the base branch to fork from * @returns {string} The new pull request body which may contain a link to * the full content. */ handleOverflow( pullRequest: ReleasePullRequest, + baseBranch: string, maxSize?: number ): Promise; @@ -81,6 +83,7 @@ export class FilePullRequestOverflowHandler */ async handleOverflow( pullRequest: ReleasePullRequest, + baseBranch: string, maxSize: number = MAX_ISSUE_BODY_SIZE ): Promise { const notes = pullRequest.body.toString(); @@ -90,7 +93,7 @@ export class FilePullRequestOverflowHandler RELEASE_NOTES_FILENAME, notes, notesBranchName, - this.github.repository.defaultBranch + baseBranch ); return `${OVERFLOW_MESSAGE} ${url}`; } diff --git a/test/github.ts b/test/github.ts index 8ab3ffc94..d398797d9 100644 --- a/test/github.ts +++ b/test/github.ts @@ -137,7 +137,10 @@ describe('GitHub', () => { req .get('/repos/fake/fake/git/trees/main?recursive=true') .reply(200, fileSearchResponse); - const pomFiles = await github.findFilesByFilename('pom.xml'); + const pomFiles = await github.findFilesByFilenameAndRef( + 'pom.xml', + 'main' + ); snapshot(pomFiles); req.done(); }); @@ -162,7 +165,11 @@ describe('GitHub', () => { req .get('/repos/fake/fake/git/trees/main?recursive=true') .reply(200, fileSearchResponse); - const pomFiles = await github.findFilesByFilename('pom.xml', prefix); + const pomFiles = await github.findFilesByFilenameAndRef( + 'pom.xml', + 'main', + prefix + ); req.done(); expect(pomFiles).to.deep.equal(['pom.xml', 'foo/pom.xml']); }); @@ -177,7 +184,7 @@ describe('GitHub', () => { req .get('/repos/fake/fake/git/trees/main?recursive=true') .reply(200, fileSearchResponse); - const pomFiles = await github.findFilesByExtension('xml'); + const pomFiles = await github.findFilesByExtensionAndRef('xml', 'main'); snapshot(pomFiles); req.done(); }); @@ -202,7 +209,11 @@ describe('GitHub', () => { req .get('/repos/fake/fake/git/trees/main?recursive=true') .reply(200, fileSearchResponse); - const pomFiles = await github.findFilesByExtension('xml', prefix); + const pomFiles = await github.findFilesByExtensionAndRef( + 'xml', + 'main', + prefix + ); req.done(); expect(pomFiles).to.deep.equal(['pom.xml', 'foo/pom.xml']); }); @@ -217,7 +228,11 @@ describe('GitHub', () => { req .get('/repos/fake/fake/git/trees/main?recursive=true') .reply(200, fileSearchResponse); - const pomFiles = await github.findFilesByExtension('xml', 'appengine'); + const pomFiles = await github.findFilesByExtensionAndRef( + 'xml', + 'main', + 'appengine' + ); req.done(); expect(pomFiles).to.deep.equal(['pom.xml', 'foo/pom.xml']); }); @@ -257,7 +272,10 @@ describe('GitHub', () => { ) .reply(200, dataAPIBlobResponse); - const fileContents = await github.getFileContents('package-lock.json'); + const fileContents = await github.getFileContentsOnBranch( + 'package-lock.json', + 'main' + ); expect(fileContents).to.have.property('content'); expect(fileContents).to.have.property('parsedContent'); expect(fileContents) @@ -269,7 +287,7 @@ describe('GitHub', () => { it('should throw a missing file error', async () => { await assert.rejects(async () => { - await github.getFileContents('non-existent-file'); + await github.getFileContentsOnBranch('non-existent-file', 'main'); }, FileNotFoundError); }); }); diff --git a/test/helpers.ts b/test/helpers.ts index 6b3fe3d2e..8a2a8e6b6 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -133,7 +133,7 @@ export interface StubFiles { sandbox: sinon.SinonSandbox; github: GitHub; - // "master" TODO update all test code to use "main" + // "main" targetBranch?: string; // Example1: test/updaters/fixtures/python @@ -175,7 +175,7 @@ export function stubFilesFromFixtures(options: StubFiles) { 'Overlap between files and inlineFiles: ' + JSON.stringify(overlap) ); } - const targetBranch = options.targetBranch ?? 'master'; + const targetBranch = options.targetBranch ?? 'main'; const flatten = options.flatten ?? true; const stub = sandbox.stub(github, 'getFileContentsOnBranch'); for (const file of files) { @@ -396,6 +396,7 @@ export class MockPullRequestOverflowHandler { async handleOverflow( pullRequest: ReleasePullRequest, + _baseBranch: string, _maxSize?: number | undefined ): Promise { return pullRequest.body.toString(); diff --git a/test/plugins/maven-workspace.ts b/test/plugins/maven-workspace.ts index 9b3ddc735..4d1381121 100644 --- a/test/plugins/maven-workspace.ts +++ b/test/plugins/maven-workspace.ts @@ -402,7 +402,7 @@ describe('MavenWorkspace plugin', () => { 'bom/pom.xml', PomXml ); - safeSnapshot(await renderUpdate(github, bomUpdate)); + safeSnapshot(await renderUpdate(github, 'main', bomUpdate)); }); it('skips updating manifest with snapshot versions', async () => { plugin = new MavenWorkspace(github, 'main', { @@ -515,8 +515,13 @@ function buildMockPackageUpdate( }; } -async function renderUpdate(github: GitHub, update: Update): Promise { +async function renderUpdate( + github: GitHub, + branch: string, + update: Update +): Promise { const fileContents = - update.cachedFileContents || (await github.getFileContents(update.path)); + update.cachedFileContents || + (await github.getFileContentsOnBranch(update.path, branch)); return update.updater.updateContent(fileContents.parsedContent); } diff --git a/test/strategies/base.ts b/test/strategies/base.ts index 54a1b39e6..91242f360 100644 --- a/test/strategies/base.ts +++ b/test/strategies/base.ts @@ -94,7 +94,15 @@ describe('Strategy', () => { targetBranch: 'main', github, component: 'google-cloud-automl', - extraFiles: ['0', 'foo/1.~csv', 'foo/2.bak', 'foo/baz/bar/', '/3.java'], + extraFiles: [ + 'README.md', + 'build.gradle.kts', + '0', + 'foo/1.~csv', + 'foo/2.bak', + 'foo/baz/bar/', + '/3.java', + ], }); const pullRequest = await strategy.buildReleasePullRequest( buildMockConventionalCommit('fix: a bugfix'), @@ -104,6 +112,8 @@ describe('Strategy', () => { expect(pullRequest?.updates).to.be.an('array'); expect(pullRequest?.updates.map(update => update.path)) .to.include.members([ + 'README.md', + 'build.gradle.kts', '0', '3.java', 'foo/1.~csv', diff --git a/test/strategies/ocaml.ts b/test/strategies/ocaml.ts index e73e73e2b..68343f6b8 100644 --- a/test/strategies/ocaml.ts +++ b/test/strategies/ocaml.ts @@ -63,7 +63,7 @@ describe('OCaml', () => { github, component: 'google-cloud-automl', }); - sandbox.stub(github, 'findFilesByExtension').resolves([]); + sandbox.stub(github, 'findFilesByExtensionAndRef').resolves([]); const latestRelease = undefined; const release = await strategy.buildReleasePullRequest( COMMITS, @@ -78,7 +78,7 @@ describe('OCaml', () => { github, component: 'google-cloud-automl', }); - sandbox.stub(github, 'findFilesByExtension').resolves([]); + sandbox.stub(github, 'findFilesByExtensionAndRef').resolves([]); const latestRelease = { tag: new TagName(Version.parse('0.123.4'), 'google-cloud-automl'), sha: 'abc123', @@ -98,7 +98,7 @@ describe('OCaml', () => { github, component: 'google-cloud-automl', }); - sandbox.stub(github, 'findFilesByExtension').resolves([]); + sandbox.stub(github, 'findFilesByExtensionAndRef').resolves([]); const latestRelease = undefined; const release = await strategy.buildReleasePullRequest( COMMITS, @@ -116,11 +116,13 @@ describe('OCaml', () => { github, component: 'google-cloud-automl', }); - const findFilesStub = sandbox.stub(github, 'findFilesByExtension'); - findFilesStub.withArgs('json', '.').resolves(['esy.json', 'other.json']); - findFilesStub.withArgs('opam', '.').resolves(['sample.opam']); + const findFilesStub = sandbox.stub(github, 'findFilesByExtensionAndRef'); findFilesStub - .withArgs('opam.locked', '.') + .withArgs('json', 'main', '.') + .resolves(['esy.json', 'other.json']); + findFilesStub.withArgs('opam', 'main', '.').resolves(['sample.opam']); + findFilesStub + .withArgs('opam.locked', 'main', '.') .resolves(['sample.opam.locked']); stubFilesFromFixtures({ sandbox, diff --git a/test/util/pull-request-overflow-handler.ts b/test/util/pull-request-overflow-handler.ts index 34fa67c7b..8e73e21e9 100644 --- a/test/util/pull-request-overflow-handler.ts +++ b/test/util/pull-request-overflow-handler.ts @@ -62,27 +62,31 @@ describe('FilePullRequestOverflowHandler', () => { ); const newContents = await overflowHandler.handleOverflow( { - title: PullRequestTitle.ofTargetBranch('main', 'main'), + title: PullRequestTitle.ofTargetBranch('main', 'next'), body, labels: [], updates: [], draft: false, headRefName: 'my-head-branch', }, + 'next', 50 ); snapshot(newContents); sinon.assert.calledOnce(createFileStub); }); it('ignores small pull request body contents', async () => { - const newContents = await overflowHandler.handleOverflow({ - title: PullRequestTitle.ofTargetBranch('main', 'main'), - body, - labels: [], - updates: [], - draft: false, - headRefName: 'my-head-branch', - }); + const newContents = await overflowHandler.handleOverflow( + { + title: PullRequestTitle.ofTargetBranch('main', 'next'), + body, + labels: [], + updates: [], + draft: false, + headRefName: 'my-head-branch', + }, + 'next' + ); expect(newContents).to.eql(body.toString()); }); });