diff --git a/packages/backend-common/src/reading/GithubUrlReader.test.ts b/packages/backend-common/src/reading/GithubUrlReader.test.ts index e165745790..658d908f84 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.test.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.test.ts @@ -29,8 +29,11 @@ import { } from './GithubUrlReader'; import fs from 'fs'; import path from 'path'; -import mockfs from 'mock-fs'; -import recursive from 'recursive-readdir'; +import { ReadTreeResponseFactory } from './tree'; + +const treeResponseFactory = ReadTreeResponseFactory.create({ + config: new ConfigReader({}), +}); describe('GithubUrlReader', () => { describe('getApiRequestOptions', () => { @@ -226,10 +229,13 @@ describe('GithubUrlReader', () => { describe('implementation', () => { it('rejects unknown targets', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - apiBaseUrl: 'https://api.github.com', - }); + const processor = new GithubUrlReader( + { + host: 'github.com', + apiBaseUrl: 'https://api.github.com', + }, + { treeResponseFactory }, + ); await expect( processor.read('https://not.github.com/apa'), ).rejects.toThrow( @@ -250,7 +256,7 @@ describe('GithubUrlReader', () => { beforeEach(() => { worker.use( rest.get( - 'https://github.com/spotify/mock/archive/repo.tar.gz', + 'https://github.com/backstage/mock/archive/repo.tar.gz', (_, res, ctx) => res( ctx.status(200), @@ -262,18 +268,20 @@ describe('GithubUrlReader', () => { }); it('returns the wanted files from an archive', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - }); + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); const response = await processor.readTree( - 'https://github.com/spotify/mock', - 'repo', - ['mkdocs.yml', 'docs'], + 'https://github.com/backstage/mock/tree/repo', ); const files = await response.files(); + expect(files.length).toBe(2); const mkDocsFile = await files[0].content(); const indexMarkdownFile = await files[1].content(); @@ -281,33 +289,39 @@ describe('GithubUrlReader', () => { expect(indexMarkdownFile.toString()).toBe('# Test\n'); }); - it('returns a folder path from an archive', async () => { - const processor = new GithubUrlReader({ - host: 'github.com', - }); + it('must specify a branch', async () => { + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); + + await expect( + processor.readTree('https://github.com/backstage/mock'), + ).rejects.toThrow( + 'GitHub URL must contain branch to be able to fetch tree', + ); + }); + + it('returns the wanted files from an archive with a subpath', async () => { + const processor = new GithubUrlReader( + { + host: 'github.com', + }, + { treeResponseFactory }, + ); const response = await processor.readTree( - 'https://github.com/spotify/mock', - 'repo', - ['mkdocs.yml', 'docs'], + 'https://github.com/backstage/mock/tree/repo/docs', ); - mockfs(); - const directory = await response.dir({ targetDir: '/tmp/fs' }); + const files = await response.files(); - const writtenToDirectory = fs.existsSync(directory); - const paths = await recursive(directory); - mockfs.restore(); + expect(files.length).toBe(1); + const indexMarkdownFile = await files[0].content(); - expect(writtenToDirectory).toBe(true); - expect(paths.sort()).toEqual( - [ - '/tmp/fs/mock-repo/docs/index.md', - '/tmp/fs/mock-repo/mkdocs.yml', - ].sort(), - ); - - worker.resetHandlers(); + expect(indexMarkdownFile.toString()).toBe('# Test\n'); }); }); }); diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index e3a47ee420..ca67a6cfcb 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -17,19 +17,15 @@ import { Config } from '@backstage/config'; import parseGitUri from 'git-url-parse'; import fetch from 'cross-fetch'; -import { NotFoundError } from '../errors'; +import { Readable } from 'stream'; +import { InputError, NotFoundError } from '../errors'; import { ReaderFactory, ReadTreeResponse, UrlReader, - File, - ReadTreeResponseDirOptions, + ReadTreeOptions, } from './types'; -import tar from 'tar'; -import fs from 'fs-extra'; -import concatStream from 'concat-stream'; -import path from 'path'; -import os from 'os'; +import { ReadTreeResponseFactory } from './tree'; /** * The configuration parameters for a single GitHub API provider. @@ -198,19 +194,18 @@ export function readConfig(config: Config): ProviderConfig[] { * the one exposed by GitHub itself. */ export class GithubUrlReader implements UrlReader { - private config: ProviderConfig; - - static factory: ReaderFactory = ({ config }) => { + static factory: ReaderFactory = ({ config, treeResponseFactory }) => { return readConfig(config).map(provider => { - const reader = new GithubUrlReader(provider); + const reader = new GithubUrlReader(provider, { treeResponseFactory }); const predicate = (url: URL) => url.host === provider.host; return { reader, predicate }; }); }; - constructor(config: ProviderConfig) { - this.config = config; - } + constructor( + private readonly config: ProviderConfig, + private readonly deps: { treeResponseFactory: ReadTreeResponseFactory }, + ) {} async read(url: string): Promise { const useApi = @@ -240,90 +235,48 @@ export class GithubUrlReader implements UrlReader { throw new Error(message); } - private async getRepositoryArchive( - repoUrl: string, - branchName: string, - ): Promise { - return fetch(new URL(`${repoUrl}/archive/${branchName}.tar.gz`).toString()); - } - - private async writeBufferToFile( - filePath: string, - content: Buffer, - ): Promise { - await fs.outputFile(filePath, content.toString()); - } - async readTree( - repoUrl: string, - branchName: string, - paths: Array, + url: string, + options?: ReadTreeOptions, ): Promise { - const { name: repoName } = parseGitUri(repoUrl); + const { + name: repoName, + ref, + protocol, + source, + full_name, + filepath, + } = parseGitUri(url); - const repoArchive = await this.getRepositoryArchive(repoUrl, branchName); + if (!ref) { + // TODO(Rugvip): We should add support for defaulting to the default branch + throw new InputError( + 'GitHub URL must contain branch to be able to fetch tree', + ); + } - const files: File[] = []; - return new Promise(resolve => { - const parser = new (tar.Parse as any)({ - filter: (path: string) => - !!paths.filter(file => { - return path.startsWith(`${repoName}-${branchName}/${file}`); - }).length, - onentry: (entry: tar.ReadEntry) => { - if (entry.type === 'Directory') { - entry.resume(); - return; - } + // TODO(Rugvip): use API to fetch URL instead + const response = await fetch( + new URL( + `${protocol}://${source}/${full_name}/archive/${ref}.tar.gz`, + ).toString(), + ); + if (!response.ok) { + const message = `Failed to read tree from ${url}, ${response.status} ${response.statusText}`; + if (response.status === 404) { + throw new NotFoundError(message); + } + throw new Error(message); + } - const contentPromise: Promise = new Promise(res => { - entry.pipe(concatStream(res)); - }); + const path = `${repoName}-${ref}/${filepath}`; - files.push({ - path: entry.path, - content: () => contentPromise, - }); - - entry.resume(); - }, - }); - - // @ts-ignore Typescript doesn't consider .pipe a method on ReadableStream. Don't know why. - repoArchive.body?.pipe(parser).on('finish', () => { - resolve({ - files: async () => { - return files; - }, - archive: () => { - return new Promise(resolve => - resolve(Buffer.from('Archive is not yet implemented')), - ); - }, - dir: (options?: ReadTreeResponseDirOptions) => { - const targetDirectory = - options?.targetDir || - fs.mkdtempSync(path.join(os.tmpdir(), 'backstage-')); - - return new Promise((res, rej) => { - Promise.all( - files.map(async file => { - return this.writeBufferToFile( - `${targetDirectory}/${file.path}`, - await file.content(), - ); - }), - ) - .then(() => { - res(`${targetDirectory}/${repoName}-${branchName}`); - }) - .catch(err => { - rej(err); - }); - }); - }, - }); - }); + return this.deps.treeResponseFactory.fromArchive({ + // TODO(Rugvip): Underlying implementation of fetch will be node-fetch, we probably want + // to stick to using that in exclusively backend code. + stream: (response.body as unknown) as Readable, + path, + filter: options?.filter, }); } diff --git a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts index 74f94f0295..9b0566106a 100644 --- a/packages/backend-common/src/reading/UrlReaderPredicateMux.ts +++ b/packages/backend-common/src/reading/UrlReaderPredicateMux.ts @@ -14,7 +14,12 @@ * limitations under the License. */ -import { ReadTreeResponse, UrlReader, UrlReaderPredicateTuple } from './types'; +import { + ReadTreeOptions, + ReadTreeResponse, + UrlReader, + UrlReaderPredicateTuple, +} from './types'; type Options = { // UrlReader to fall back to if no other reader is matched @@ -55,14 +60,13 @@ export class UrlReaderPredicateMux implements UrlReader { readTree( repoUrl: string, - branchName: string, - paths: Array, + options?: ReadTreeOptions, ): Promise { const parsed = new URL(repoUrl); for (const { predicate, reader } of this.readers) { if (predicate(parsed)) { - if (reader.readTree) return reader.readTree(repoUrl, branchName, paths); + if (reader.readTree) return reader.readTree(repoUrl, options); throw new Error( `Trying to call readTree on UrlReader which does not support the feature.`, ); @@ -71,7 +75,7 @@ export class UrlReaderPredicateMux implements UrlReader { if (this.fallback) { if (this.fallback.readTree) - return this.fallback.readTree(repoUrl, branchName, paths); + return this.fallback.readTree(repoUrl, options); throw new Error( `Trying to call readTree on UrlReader which does not support the feature.`, ); diff --git a/packages/backend-common/src/reading/tree/ArchiveResponse.ts b/packages/backend-common/src/reading/tree/ArchiveResponse.ts index 83f6f6a8e5..0e06ff3804 100644 --- a/packages/backend-common/src/reading/tree/ArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ArchiveResponse.ts @@ -42,10 +42,12 @@ export class ArchiveResponse implements ReadTreeResponse { ) { if (subPath) { if (!subPath.endsWith('/')) { - throw new TypeError('ArchiveResponse subPath must end with a /'); + this.subPath += '/'; } if (subPath.startsWith('/')) { - throw new TypeError('ArchiveResponse subPath must not start with a /'); + throw new TypeError( + `ArchiveResponse subPath must not start with a /, got '${subPath}'`, + ); } } } diff --git a/packages/backend-common/src/reading/types.ts b/packages/backend-common/src/reading/types.ts index bf53e98ef2..c7e5915401 100644 --- a/packages/backend-common/src/reading/types.ts +++ b/packages/backend-common/src/reading/types.ts @@ -28,11 +28,7 @@ export type ReadTreeOptions = { */ export type UrlReader = { read(url: string): Promise; - readTree?( - repoUrl: string, - branchName: string, - paths: Array, - ): Promise; + readTree?(url: string, options?: ReadTreeOptions): Promise; }; export type UrlReaderPredicateTuple = { diff --git a/plugins/techdocs-backend/src/helpers.test.ts b/plugins/techdocs-backend/src/helpers.test.ts index c3f2261637..5e98ab51c8 100644 --- a/plugins/techdocs-backend/src/helpers.test.ts +++ b/plugins/techdocs-backend/src/helpers.test.ts @@ -19,7 +19,7 @@ import { UrlReader, ReadTreeResponse } from '@backstage/backend-common'; import { Entity } from '@backstage/catalog-model'; describe('getDocFilesFromRepository', () => { - it('should take the directory from UrlReader.readTree and add the docs path when mkdocs.yml is in root', async () => { + it('should read a remote directory using UrlReader.readTree', async () => { class MockUrlReader implements UrlReader { async read() { return Buffer.from('mock'); @@ -45,7 +45,7 @@ describe('getDocFilesFromRepository', () => { namespace: 'default', annotations: { 'backstage.io/techdocs-ref': - 'url:https://github.com/backstage/backstage/blob/master/mkdocs.yml', + 'url:https://github.com/backstage/backstage/blob/master/subfolder/', }, name: 'mytestcomponent', description: 'A component for testing', @@ -64,54 +64,6 @@ describe('getDocFilesFromRepository', () => { mockEntity, ); - expect(output).toBe('/tmp/testfolder/.'); - }); - - it('should take the directory from UrlReader.readTree and add the docs path when mkdocs.yml is in a subfolder', async () => { - class MockUrlReader implements UrlReader { - async read() { - return Buffer.from('mock'); - } - - async readTree(): Promise { - return { - dir: async () => { - return '/tmp/testfolder'; - }, - files: async () => { - return []; - }, - archive: async () => { - return Buffer.from(''); - }, - }; - } - } - - const mockEntity: Entity = { - metadata: { - namespace: 'default', - annotations: { - 'backstage.io/techdocs-ref': - 'url:https://github.com/backstage/backstage/blob/master/subfolder/mkdocs.yml', - }, - name: 'mytestcomponent', - description: 'A component for testing', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - spec: { - type: 'documentation', - lifecycle: 'experimental', - owner: 'testuser', - }, - }; - - const output = await getDocFilesFromRepository( - new MockUrlReader(), - mockEntity, - ); - - expect(output).toBe('/tmp/testfolder/subfolder'); + expect(output).toBe('/tmp/testfolder'); }); }); diff --git a/plugins/techdocs-backend/src/helpers.ts b/plugins/techdocs-backend/src/helpers.ts index f861d4375e..97b087558f 100644 --- a/plugins/techdocs-backend/src/helpers.ts +++ b/plugins/techdocs-backend/src/helpers.ts @@ -179,21 +179,10 @@ export const getDocFilesFromRepository = async ( entity, ); - const { ref, filepath: mkdocsPath } = parseGitUrl(target); - - const docsRootPath = path.dirname(mkdocsPath); - const docsFolderPath = path.join(docsRootPath, 'docs'); - if (reader.readTree) { - const readTreeResponse = await reader.readTree( - parseGitUrl(target).toString(), - ref, - [mkdocsPath, docsFolderPath], - ); + const response = await reader.readTree(target); - const tmpDir = await readTreeResponse.dir(); - - return `${tmpDir}/${docsRootPath}`; + return response.dir(); } throw new Error(