From a1002df2dc24ce87e411b1ccddf538e5cbe65dfa Mon Sep 17 00:00:00 2001 From: Patrick Jungermann Date: Sat, 8 Apr 2023 03:13:17 +0200 Subject: [PATCH] feat(github): support commit hashes at GithubUrlReader.readTree/search Support commit hashes additionally to branch names at the `GithubUrlReader`'s `readTree` and `search`. This is achieved by using the "combined commit status" API instead of the branch API which works with commit hashes and branch names. When used with branch names, it will return the result for the HEAD of the branch which is what the branch API was used for before. We are not really interested in commit statuses, though, and can ignore them. The "combined commit status" API support branch names with slashes as well, like `branch/name`. Additionally, this will reduce the number of API calls from 2 to 1 for retrieving the "repo details" for all cases besides when the default branch has to be resolved and used (e.g., repo URL without any branch or commit hash). Relates-to: PR #16721 Signed-off-by: Patrick Jungermann --- .changeset/brave-forks-pull.md | 9 ++ .../src/reading/GithubUrlReader.test.ts | 132 ++++++++++++------ .../src/reading/GithubUrlReader.ts | 52 ++++--- 3 files changed, 130 insertions(+), 63 deletions(-) create mode 100644 .changeset/brave-forks-pull.md diff --git a/.changeset/brave-forks-pull.md b/.changeset/brave-forks-pull.md new file mode 100644 index 0000000000..b7088f2721 --- /dev/null +++ b/.changeset/brave-forks-pull.md @@ -0,0 +1,9 @@ +--- +'@backstage/backend-common': patch +--- + +Support commit hashes at `GithubUrlReader.readTree/search` additionally to branch names. + +Additionally, this will reduce the number of API calls from 2 to 1 for retrieving the "repo details" +for all cases besides when the default branch has to be resolved and used +(e.g., repo URL without any branch or commit hash). diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index 1ee25920f2..499f97d6ec 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -30,7 +30,7 @@ import path from 'path'; import { NotFoundError, NotModifiedError } from '@backstage/errors'; import { GhBlobResponse, - GhBranchResponse, + GhCombinedCommitStatusResponse, GhRepoResponse, GhTreeResponse, GithubUrlReader, @@ -309,12 +309,15 @@ describe('GithubUrlReader', () => { 'https://ghe.github.com/api/v3/repos/backstage/mock/{archive_format}{/ref}', } as Partial; - const branchesApiResponse = { - name: 'main', - commit: { - sha: 'etag123abc', - }, - } as Partial; + const commitStatusGithubResponse = { + sha: 'etag123abc', + repository: reposGithubApiResponse, + } as Partial; + + const commitStatusGheResponse = { + sha: 'etag123abc', + repository: reposGheApiResponse, + } as Partial; beforeEach(() => { worker.use( @@ -326,13 +329,18 @@ describe('GithubUrlReader', () => { ), ), rest.get( - 'https://api.github.com/repos/backstage/mock/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(branchesApiResponse), - ), + 'https://api.github.com/repos/backstage/mock/commits/main/status', + (req, res, ctx) => { + if (req.url.searchParams.get('per_page') === '0') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(commitStatusGithubResponse), + ); + } + + return res(ctx.status(500)); + }, ), rest.get( 'https://api.github.com/repos/backstage/mock/tarball/etag123abc', @@ -348,7 +356,7 @@ describe('GithubUrlReader', () => { ), ), rest.get( - 'https://api.github.com/repos/backstage/mock/branches/branchDoesNotExist', + 'https://api.github.com/repos/backstage/mock/commits/branchDoesNotExist/status', (_, res, ctx) => res(ctx.status(404)), ), rest.get( @@ -374,13 +382,18 @@ describe('GithubUrlReader', () => { ), ), rest.get( - 'https://ghe.github.com/api/v3/repos/backstage/mock/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(branchesApiResponse), - ), + 'https://ghe.github.com/api/v3/repos/backstage/mock/commits/main/status', + (req, res, ctx) => { + if (req.url.searchParams.get('per_page') === '0') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(commitStatusGheResponse), + ); + } + + return res(ctx.status(500)); + }, ), ); }); @@ -684,36 +697,69 @@ describe('GithubUrlReader', () => { ); }); - // Branch details + // commit status for branch details beforeEach(() => { - const response = { - name: 'main', - commit: { - sha: 'etag123abc', + const githubResponse = { + sha: 'etag123abc', + repository: { + id: 123, + full_name: 'backstage/mock', + default_branch: 'main', + branches_url: + 'https://api.github.com/repos/backstage/mock/branches{/branch}', + archive_url: + 'https://api.github.com/repos/backstage/mock/{archive_format}{/ref}', + trees_url: + 'https://api.github.com/repos/backstage/mock/git/trees{/sha}', }, - } as Partial; + } as Partial; + + const gheResponse = { + sha: 'etag123abc', + repository: { + id: 123, + full_name: 'backstage/mock', + default_branch: 'main', + branches_url: + 'https://ghe.github.com/api/v3/repos/backstage/mock/branches{/branch}', + archive_url: + 'https://ghe.github.com/api/v3/repos/backstage/mock/{archive_format}{/ref}', + trees_url: + 'https://ghe.github.com/api/v3/repos/backstage/mock/git/trees{/sha}', + }, + } as Partial; worker.use( rest.get( - 'https://api.github.com/repos/backstage/mock/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(response), - ), + 'https://api.github.com/repos/backstage/mock/commits/main/status', + (req, res, ctx) => { + if (req.url.searchParams.get('per_page') === '0') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(githubResponse), + ); + } + + return res(ctx.status(500)); + }, ), rest.get( - 'https://ghe.github.com/api/v3/repos/backstage/mock/branches/main', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/json'), - ctx.json(response), - ), + 'https://ghe.github.com/api/v3/repos/backstage/mock/commits/main/status', + (req, res, ctx) => { + if (req.url.searchParams.get('per_page') === '0') { + return res( + ctx.status(200), + ctx.set('Content-Type', 'application/json'), + ctx.json(gheResponse), + ); + } + + return res(ctx.status(500)); + }, ), rest.get( - 'https://api.github.com/repos/backstage/mock/branches/branchDoesNotExist', + 'https://api.github.com/repos/backstage/mock/commits/branchDoesNotExist/status', (_, res, ctx) => res(ctx.status(404)), ), ); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index ee781daf0b..8300127c5b 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -20,6 +20,7 @@ import { GithubCredentialsProvider, GithubIntegration, ScmIntegrations, + GithubCredentials, } from '@backstage/integration'; import { RestEndpointMethodTypes } from '@octokit/rest'; import fetch, { RequestInit, Response } from 'node-fetch'; @@ -44,8 +45,8 @@ import { parseLastModified } from './util'; export type GhRepoResponse = RestEndpointMethodTypes['repos']['get']['response']['data']; -export type GhBranchResponse = - RestEndpointMethodTypes['repos']['getBranch']['response']['data']; +export type GhCombinedCommitStatusResponse = + RestEndpointMethodTypes['repos']['getCombinedStatusForRef']['response']['data']; export type GhTreeResponse = RestEndpointMethodTypes['git']['getTree']['response']['data']; export type GhBlobResponse = @@ -163,7 +164,7 @@ export class GithubUrlReader implements UrlReader { options?: ReadTreeOptions, ): Promise { const repoDetails = await this.getRepoDetails(url); - const commitSha = repoDetails.branch.commit.sha!; + const commitSha = repoDetails.commitSha; if (options?.etag && options.etag === commitSha) { throw new NotModifiedError(); @@ -191,7 +192,7 @@ export class GithubUrlReader implements UrlReader { async search(url: string, options?: SearchOptions): Promise { const repoDetails = await this.getRepoDetails(url); - const commitSha = repoDetails.branch.commit.sha!; + const commitSha = repoDetails.commitSha; if (options?.etag && options.etag === commitSha) { throw new NotModifiedError(); @@ -302,32 +303,43 @@ export class GithubUrlReader implements UrlReader { } private async getRepoDetails(url: string): Promise<{ - repo: GhRepoResponse; - branch: GhBranchResponse; + commitSha: string; + repo: { + archive_url: string; + trees_url: string; + }; }> { const parsed = parseGitUrl(url); const { ref, full_name } = parsed; - // Caveat: The ref will totally be incorrect if the branch name includes a - // slash. Thus, some operations can not work on URLs containing branch - // names that have a slash in them. - - const { headers } = await this.deps.credentialsProvider.getCredentials({ + const credentials = await this.deps.credentialsProvider.getCredentials({ url, }); + const { headers } = credentials; + const commitStatus: GhCombinedCommitStatusResponse = await this.fetchJson( + `${this.integration.config.apiBaseUrl}/repos/${full_name}/commits/${ + ref || (await this.getDefaultBranch(full_name, credentials)) + }/status?per_page=0`, + { headers }, + ); + + return { + commitSha: commitStatus.sha, + repo: commitStatus.repository, + }; + } + + private async getDefaultBranch( + repoFullName: string, + credentials: GithubCredentials, + ): Promise { const repo: GhRepoResponse = await this.fetchJson( - `${this.integration.config.apiBaseUrl}/repos/${full_name}`, - { headers }, + `${this.integration.config.apiBaseUrl}/repos/${repoFullName}`, + { headers: credentials.headers }, ); - // branches_url looks like "https://api.github.com/repos/owner/repo/branches{/branch}" - const branch: GhBranchResponse = await this.fetchJson( - repo.branches_url.replace('{/branch}', `/${ref || repo.default_branch}`), - { headers }, - ); - - return { repo, branch }; + return repo.default_branch; } private async fetchResponse(