diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index 26ba91bd29..0b7d56480f 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -108,7 +108,12 @@ describe('GithubUrlReader', () => { describe('readTree', () => { const repoBuffer = fs.readFileSync( - path.resolve('src', 'reading', '__fixtures__', 'mock-main.tar.gz'), + path.resolve( + 'src', + 'reading', + '__fixtures__', + 'backstage-mock-etag123.tar.gz', + ), ); const reposGithubApiResponse = { @@ -117,12 +122,16 @@ describe('GithubUrlReader', () => { 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}', }; const reposGheApiResponse = { ...reposGithubApiResponse, 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}', }; const branchesApiResponse = { @@ -134,15 +143,6 @@ describe('GithubUrlReader', () => { beforeEach(() => { worker.use( - rest.get( - 'https://github.com/backstage/mock/archive/main.tar.gz', - (_, res, ctx) => - res( - ctx.status(200), - ctx.set('Content-Type', 'application/x-gzip'), - ctx.body(repoBuffer), - ), - ), rest.get('https://api.github.com/repos/backstage/mock', (_, res, ctx) => res( ctx.status(200), @@ -159,12 +159,21 @@ describe('GithubUrlReader', () => { ctx.json(branchesApiResponse), ), ), + rest.get( + 'https://api.github.com/repos/backstage/mock/tarball/etag123abc', + (_, res, ctx) => + res( + ctx.status(200), + ctx.set('Content-Type', 'application/x-gzip'), + ctx.body(repoBuffer), + ), + ), rest.get( 'https://api.github.com/repos/backstage/mock/branches/branchDoesNotExist', (_, res, ctx) => res(ctx.status(404)), ), rest.get( - 'https://ghe.github.com/backstage/mock/archive/main.tar.gz', + 'https://ghe.github.com/api/v3/repos/backstage/mock/tarball/etag123abc', (_, res, ctx) => res( ctx.status(200), @@ -224,7 +233,7 @@ describe('GithubUrlReader', () => { worker.use( rest.get( - 'https://ghe.github.com/backstage/mock/archive/main.tar.gz', + 'https://ghe.github.com/api/v3/repos/backstage/mock/tarball/etag123abc', (req, res, ctx) => { expect(req.headers.get('authorization')).toBe( mockHeaders.Authorization, diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index d6612a9da2..b5b4e9c1a7 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -98,14 +98,9 @@ export class GithubUrlReader implements UrlReader { url: string, options?: ReadTreeOptions, ): Promise { - const { - protocol, - resource, - name: repoName, - ref, - filepath, - full_name, - } = parseGitUrl(url); + const { ref, filepath, full_name } = parseGitUrl(url); + // Caveat: The ref will totally be incorrect if the branch name includes a / + // Thus, readTree can not work on url containing branch name that has a / const { headers } = await this.deps.credentialsProvider.getCredentials({ url, @@ -132,6 +127,7 @@ export class GithubUrlReader implements UrlReader { // Use GitHub API to get the default branch of the repository. const branch = ref || repoResponseJson.default_branch; const branchesApiUrl = repoResponseJson.branches_url; + const archiveApiUrl = repoResponseJson.archive_url; // Fetch the latest commit in the provided or default branch to compare against // the provided sha. @@ -155,19 +151,12 @@ export class GithubUrlReader implements UrlReader { throw new NotModifiedError(); } - // Note: the API way of downloading an archive URL does not return a real time archive. - // https://github.community/t/archive-downloaded-via-v3-rest-api-is-not-real-time/14827 - // It looks like this https://api.github.com/repos/owner/repo/{archive_format}{/ref} - // and can be used from `repoResponseJson.archive_url`. - // Continue using the "direct" way i.e. https://github.com/:owner/:repo/archive/branch.tar.gz - // until the bug? is fixed. const archive = await fetch( - new URL( - `${protocol}://${resource}/${full_name}/archive/${branch}.tar.gz`, - ).toString(), - { - headers, - }, + // archiveApiUrl looks like "https://api.github.com/repos/owner/repo/{archive_format}{/ref}" + archiveApiUrl + .replace('{archive_format}', 'tarball') + .replace('{/ref}', `/${commitSha}`), + { headers }, ); if (!archive.ok) { const message = `Failed to read tree (archive) from ${url}, ${archive.status} ${archive.statusText}`; @@ -177,7 +166,18 @@ export class GithubUrlReader implements UrlReader { throw new Error(message); } - const path = `${repoName}-${branch}/${filepath}`; + // Note that repoResponseJson.full_name must be used over full_name because the path + // is case sensitive and full_name may not be inq the correct case. + // TODO(OrkoHunter): The directory name inside the tarball should be retrieved from the tar + // instead of being constructed here. Same goes for GitLab, Bitbucket and Azure. + const extractedDirName = `${repoResponseJson.full_name.replace( + '/', + '-', + )}-${commitSha.substr(0, 7)}`; + + // The path includes the name of the directory inside the tarball and a sub path + // if requested in readTree. + const path = `${extractedDirName}/${filepath}`; return await this.deps.treeResponseFactory.fromTarArchive({ // TODO(Rugvip): Underlying implementation of fetch will be node-fetch, we probably want diff --git a/packages/backend-common/src/reading/__fixtures__/backstage-mock-etag123.tar.gz b/packages/backend-common/src/reading/__fixtures__/backstage-mock-etag123.tar.gz new file mode 100644 index 0000000000..e1ac2579de Binary files /dev/null and b/packages/backend-common/src/reading/__fixtures__/backstage-mock-etag123.tar.gz differ