From 512d709736cdf046045a6190e9fde98e1a65bb58 Mon Sep 17 00:00:00 2001 From: Oliver Sand Date: Thu, 8 Oct 2020 14:36:22 +0200 Subject: [PATCH] feat: use the new UrlReader in the CodeOwnersProcessor --- .changeset/codeowner-processor-url-reader.md | 5 ++ .../src/ingestion/LocationReaders.ts | 2 +- .../processors/CodeOwnersProcessor.test.ts | 70 ++++++++----------- .../processors/CodeOwnersProcessor.ts | 51 ++++++-------- 4 files changed, 60 insertions(+), 68 deletions(-) create mode 100644 .changeset/codeowner-processor-url-reader.md diff --git a/.changeset/codeowner-processor-url-reader.md b/.changeset/codeowner-processor-url-reader.md new file mode 100644 index 0000000000..af4eb0997f --- /dev/null +++ b/.changeset/codeowner-processor-url-reader.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +Use the new `UrlReader` in the `CodeOwnersProcessor`. diff --git a/plugins/catalog-backend/src/ingestion/LocationReaders.ts b/plugins/catalog-backend/src/ingestion/LocationReaders.ts index 5ee6e38b28..8cbaaa3e8e 100644 --- a/plugins/catalog-backend/src/ingestion/LocationReaders.ts +++ b/plugins/catalog-backend/src/ingestion/LocationReaders.ts @@ -125,7 +125,7 @@ export class LocationReaders implements LocationReader { new UrlReaderProcessor(options), new YamlProcessor(), PlaceholderProcessor.default({ reader: options.reader }), - new CodeOwnersProcessor(), + new CodeOwnersProcessor({ reader: options.reader }), new EntityPolicyProcessor(entityPolicy), new LocationRefProcessor(), new AnnotateLocationEntityProcessor(), diff --git a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts index 29f94dd0d7..c1c83b7c6c 100644 --- a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts +++ b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.test.ts @@ -17,7 +17,7 @@ import { LocationSpec } from '@backstage/catalog-model'; import { CodeOwnersEntry } from 'codeowners-utils'; import { - buildCodeOwnerLocation, + buildCodeOwnerUrl, buildUrl, CodeOwnersProcessor, findPrimaryCodeOwner, @@ -28,18 +28,18 @@ import { } from './CodeOwnersProcessor'; describe(CodeOwnersProcessor, () => { + const mockUrl = ({ basePath = '' } = {}): string => + `https://github.com/spotify/backstage/blob/master/${basePath}catalog-info.yaml`; const mockLocation = ({ basePath = '', type = 'github', } = {}): LocationSpec => ({ type, - target: `https://github.com/spotify/backstage/blob/master/${basePath}catalog-info.yaml`, + target: mockUrl({ basePath }), }); - const mockReadLocation = (basePath = '') => ({ - type: 'github', - target: `https://github.com/spotify/backstage/blob/master/${basePath}CODEOWNERS`, - }); + const mockReadUrl = (basePath = '') => + `https://github.com/spotify/backstage/blob/master/${basePath}CODEOWNERS`; const mockGitUri = (codeOwnersPath: string = '') => { return { @@ -99,27 +99,20 @@ describe(CodeOwnersProcessor, () => { }); }); - describe(buildCodeOwnerLocation, () => { - it('should builds a location spec to the codeowners', () => { - expect( - buildCodeOwnerLocation(mockLocation(), '/docs/CODEOWNERS'), - ).toEqual({ - type: 'github', - target: - 'https://github.com/spotify/backstage/blob/master/docs/CODEOWNERS', - }); + describe(buildCodeOwnerUrl, () => { + it('should build a location spec to the codeowners', () => { + expect(buildCodeOwnerUrl(mockUrl(), '/docs/CODEOWNERS')).toEqual( + 'https://github.com/spotify/backstage/blob/master/docs/CODEOWNERS', + ); }); it('should handle nested paths from original location spec', () => { expect( - buildCodeOwnerLocation( - mockLocation({ basePath: 'packages/foo/' }), + buildCodeOwnerUrl( + mockUrl({ basePath: 'packages/foo/' }), '/CODEOWNERS', ), - ).toEqual({ - type: 'github', - target: 'https://github.com/spotify/backstage/blob/master/CODEOWNERS', - }); + ).toEqual('https://github.com/spotify/backstage/blob/master/CODEOWNERS'); }); }); @@ -158,15 +151,17 @@ describe(CodeOwnersProcessor, () => { const read = jest .fn() .mockResolvedValue(mockReadResult({ data: ownersText })); - const result = await findRawCodeOwners(mockLocation(), read); + const reader = { read }; + const result = await findRawCodeOwners(mockLocation(), reader); expect(result).toEqual(ownersText); }); it('should raise error when no codeowner', async () => { const read = jest.fn().mockRejectedValue(mockReadResult()); + const reader = { read }; await expect( - findRawCodeOwners(mockLocation(), read), + findRawCodeOwners(mockLocation(), reader), ).rejects.toBeInstanceOf(Error); }); @@ -177,13 +172,14 @@ describe(CodeOwnersProcessor, () => { .mockImplementationOnce(() => mockReadResult({ error: 'foo' })) .mockImplementationOnce(() => mockReadResult({ error: 'bar' })) .mockResolvedValue(mockReadResult({ data: ownersText })); + const reader = { read }; - const result = await findRawCodeOwners(mockLocation(), read); + const result = await findRawCodeOwners(mockLocation(), reader); expect(read.mock.calls.length).toBe(3); - expect(read.mock.calls[0]).toEqual([mockReadLocation('.github/')]); - expect(read.mock.calls[1]).toEqual([mockReadLocation('')]); - expect(read.mock.calls[2]).toEqual([mockReadLocation('docs/')]); + expect(read.mock.calls[0]).toEqual([mockReadUrl('.github/')]); + expect(read.mock.calls[1]).toEqual([mockReadUrl('')]); + expect(read.mock.calls[2]).toEqual([mockReadUrl('docs/')]); expect(result).toEqual(ownersText); }); }); @@ -193,7 +189,9 @@ describe(CodeOwnersProcessor, () => { const read = jest .fn() .mockResolvedValue(mockReadResult({ data: mockCodeOwnersText() })); - const owner = await resolveCodeOwner(mockLocation(), read); + const reader = { read }; + + const owner = await resolveCodeOwner(mockLocation(), reader); expect(owner).toBe('backstage-core'); }); @@ -201,9 +199,10 @@ describe(CodeOwnersProcessor, () => { const read = jest .fn() .mockImplementation(() => mockReadResult({ error: 'error: foo' })); + const reader = { read }; await expect( - resolveCodeOwner(mockLocation(), read), + resolveCodeOwner(mockLocation(), reader), ).rejects.toBeInstanceOf(Error); }); }); @@ -211,10 +210,11 @@ describe(CodeOwnersProcessor, () => { describe(CodeOwnersProcessor, () => { const setupTest = ({ kind = 'Component', spec = {} } = {}) => { const entity = { kind, spec }; - const processor = new CodeOwnersProcessor(); const read = jest .fn() .mockResolvedValue(mockReadResult({ data: mockCodeOwnersText() })); + const reader = { read }; + const processor = new CodeOwnersProcessor({ reader }); return { entity, processor, read }; }; @@ -227,8 +227,6 @@ describe(CodeOwnersProcessor, () => { const result = await processor.processEntity( entity as any, mockLocation(), - null as any, - null as any, ); expect(result).toEqual(entity); @@ -240,8 +238,6 @@ describe(CodeOwnersProcessor, () => { const result = await processor.processEntity( entity as any, mockLocation({ type: 'url' }), - null as any, - null as any, ); expect(result).toEqual(entity); @@ -253,21 +249,17 @@ describe(CodeOwnersProcessor, () => { const result = await processor.processEntity( entity as any, mockLocation(), - null as any, - null as any, ); expect(result).toEqual(entity); }); it('should set owner from codeowner', async () => { - const { entity, processor, read } = setupTest(); + const { entity, processor } = setupTest(); const result = await processor.processEntity( entity as any, mockLocation(), - null as any, - read, ); expect(result).toEqual({ diff --git a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.ts index c875bdc799..e624af91ee 100644 --- a/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.ts +++ b/plugins/catalog-backend/src/ingestion/processors/CodeOwnersProcessor.ts @@ -14,12 +14,9 @@ * limitations under the License. */ +import { UrlReader } from '@backstage/backend-common'; import { Entity, LocationSpec } from '@backstage/catalog-model'; -import { - LocationProcessor, - LocationProcessorEmit, - LocationProcessorRead, -} from './types'; +import { LocationProcessor } from './types'; import * as codeowners from 'codeowners-utils'; import { CodeOwnersEntry } from 'codeowners-utils'; import parseGitUri from 'git-url-parse'; @@ -37,13 +34,14 @@ const ALLOWED_LOCATION_TYPES = [ 'gitlab/api', ]; +type Options = { + reader: UrlReader; +}; + export class CodeOwnersProcessor implements LocationProcessor { - async processEntity( - entity: Entity, - location: LocationSpec, - _emit: LocationProcessorEmit, - read: LocationProcessorRead, - ): Promise { + constructor(private readonly options: Options) {} + + async processEntity(entity: Entity, location: LocationSpec): Promise { // Only continue if the owner is not set if ( !entity || @@ -54,7 +52,7 @@ export class CodeOwnersProcessor implements LocationProcessor { return entity; } - const owner = await resolveCodeOwner(location, read); + const owner = await resolveCodeOwner(location, this.options.reader); return { ...entity, @@ -65,9 +63,9 @@ export class CodeOwnersProcessor implements LocationProcessor { export async function resolveCodeOwner( location: LocationSpec, - read: LocationProcessorRead, + reader: UrlReader, ): Promise { - const ownersText = await findRawCodeOwners(location, read); + const ownersText = await findRawCodeOwners(location, reader); if (!ownersText) { throw Error(`Unable to find codeowners file for: ${location.target}`); @@ -80,15 +78,14 @@ export async function resolveCodeOwner( export async function findRawCodeOwners( location: LocationSpec, - read: LocationProcessorRead, + reader: UrlReader, ): Promise { const readOwnerLocation = async (basePath: string): Promise => { - const ownerLocation = buildCodeOwnerLocation( - location, + const ownerUrl = buildCodeOwnerUrl( + location.target, `${basePath}/CODEOWNERS`, ); - - const data = await read(ownerLocation); + const data = await reader.read(ownerUrl); return data.toString(); }; @@ -101,6 +98,13 @@ export async function findRawCodeOwners( ]); } +export function buildCodeOwnerUrl( + basePath: string, + codeOwnersPath: string, +): string { + return buildUrl({ ...parseGitUri(basePath), codeOwnersPath }); +} + export function parseCodeOwners(ownersText: string) { return codeowners.parse(ownersText); } @@ -128,15 +132,6 @@ export function normalizeCodeOwner(owner: string) { return owner; } -export function buildCodeOwnerLocation( - location: LocationSpec, - codeOwnersPath: string, -): LocationSpec { - const { type, target } = location; - - return { type, target: buildUrl({ ...parseGitUri(target), codeOwnersPath }) }; -} - export function buildUrl({ protocol = 'https', source = 'github.com',