Skip to content

Commit

Permalink
Get rid of github wrappers assuming lookup from target branch
Browse files Browse the repository at this point in the history
  • Loading branch information
dgellow committed Aug 29, 2023
1 parent 19d278c commit f7e97f7
Show file tree
Hide file tree
Showing 12 changed files with 102 additions and 121 deletions.
81 changes: 2 additions & 79 deletions src/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<GitHubFileContents> {
return await this.getFileContentsOnBranch(
path,
this.repository.defaultBranch
);
}

/**
* Fetch the contents of a file
*
Expand Down Expand Up @@ -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<string[]> {
return this.findFilesByFilenameAndRef(
filename,
this.repository.defaultBranch,
prefix
);
}

/**
* Returns a list of paths to all files with a given name.
*
Expand Down Expand Up @@ -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<string[]> {
return this.findFilesByGlobAndRef(
glob,
this.repository.defaultBranch,
prefix
);
}
/**
* Returns a list of paths to all files matching a glob pattern.
*
Expand Down Expand Up @@ -1413,7 +1359,8 @@ export class GitHub {
const body = (
options?.pullRequestOverflowHandler
? await options.pullRequestOverflowHandler.handleOverflow(
releasePullRequest
releasePullRequest,
baseBranch
)
: releasePullRequest.body
)
Expand Down Expand Up @@ -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<string[]> {
return this.findFilesByExtensionAndRef(
extension,
this.repository.defaultBranch,
prefix
);
}

/**
* Create a GitHub release
*
Expand Down
3 changes: 2 additions & 1 deletion src/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions src/strategies/helm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,9 @@ export class Helm extends BaseStrategy {
private async getChartYmlContents(): Promise<GitHubFileContents> {
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) {
Expand Down
8 changes: 5 additions & 3 deletions src/strategies/krm-blueprint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
23 changes: 17 additions & 6 deletions src/strategies/ocaml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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),
Expand All @@ -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 => {
Expand Down
5 changes: 4 additions & 1 deletion src/util/pull-request-overflow-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>;

Expand Down Expand Up @@ -81,6 +83,7 @@ export class FilePullRequestOverflowHandler
*/
async handleOverflow(
pullRequest: ReleasePullRequest,
baseBranch: string,
maxSize: number = MAX_ISSUE_BODY_SIZE
): Promise<string> {
const notes = pullRequest.body.toString();
Expand All @@ -90,7 +93,7 @@ export class FilePullRequestOverflowHandler
RELEASE_NOTES_FILENAME,
notes,
notesBranchName,
this.github.repository.defaultBranch
baseBranch
);
return `${OVERFLOW_MESSAGE} ${url}`;
}
Expand Down
32 changes: 25 additions & 7 deletions test/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand All @@ -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']);
});
Expand All @@ -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();
});
Expand All @@ -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']);
});
Expand All @@ -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']);
});
Expand Down Expand Up @@ -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)
Expand All @@ -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);
});
});
Expand Down
5 changes: 3 additions & 2 deletions test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -396,6 +396,7 @@ export class MockPullRequestOverflowHandler
{
async handleOverflow(
pullRequest: ReleasePullRequest,
_baseBranch: string,
_maxSize?: number | undefined
): Promise<string> {
return pullRequest.body.toString();
Expand Down
11 changes: 8 additions & 3 deletions test/plugins/maven-workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand Down Expand Up @@ -515,8 +515,13 @@ function buildMockPackageUpdate(
};
}

async function renderUpdate(github: GitHub, update: Update): Promise<string> {
async function renderUpdate(
github: GitHub,
branch: string,
update: Update
): Promise<string> {
const fileContents =
update.cachedFileContents || (await github.getFileContents(update.path));
update.cachedFileContents ||
(await github.getFileContentsOnBranch(update.path, branch));
return update.updater.updateContent(fileContents.parsedContent);
}
12 changes: 11 additions & 1 deletion test/strategies/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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',
Expand Down
Loading

0 comments on commit f7e97f7

Please sign in to comment.