From 564170b765622866bb456379970b4db7869f9cff Mon Sep 17 00:00:00 2001 From: Thomas Cardonne Date: Wed, 27 Nov 2024 11:00:58 +0100 Subject: [PATCH] feat: add config flag Signed-off-by: Thomas Cardonne --- .changeset/tiny-spoons-repeat.md | 8 ++- app-config.yaml | 1 + plugins/catalog-backend/config.d.ts | 14 +++++ plugins/catalog-backend/package.json | 1 + plugins/catalog-backend/report.api.md | 6 ++- .../src/processors/UrlReaderProcessor.test.ts | 36 ++++++++++++- .../src/processors/UrlReaderProcessor.ts | 51 ++++++++++++++++--- .../src/service/CatalogBuilder.ts | 2 +- yarn.lock | 1 + 9 files changed, 109 insertions(+), 11 deletions(-) diff --git a/.changeset/tiny-spoons-repeat.md b/.changeset/tiny-spoons-repeat.md index 2977e0dde8..8f9b6ce800 100644 --- a/.changeset/tiny-spoons-repeat.md +++ b/.changeset/tiny-spoons-repeat.md @@ -3,9 +3,13 @@ '@backstage/plugin-catalog-backend': minor --- -**BREAKING:** The `UrlReaderProccessor` now always calls the `search` method of the `UrlReaders`. Previous behavior was to call the `search` method only if the parsed Git URL's filename contained a wildcard and use `readUrl` otherwise. `UrlReaderService` must implement this logic in the `search` method instead. +**BREAKING**: The `UrlReaderProccessor` accepts a new config flag `catalog.useUrlReadersSearch` to always call the `search` method of `UrlReaders`. + +This flag currently defaults to `false`, but adopters are encouraged to enable it as this behavior will be the default in a future release. + +Previous behavior was to call the `search` method only if the parsed Git URL's filename contained a wildcard and use `readUrl` otherwise. `UrlReaderService` must implement this logic in the `search` method instead. This allows each `UrlReaderService` implementation to check whether it's a search URL (that contains a wildcard pattern) or not using logic that is specific to each provider. In the different `UrlReadersService`, the `search` method have been updated to use the `readUrl` if the given URL doesn't contain a pattern. -For `UrlReaders` that didn't implement the `search` method, `readUrl` is called internally. +For `UrlReaders` that didn't implement the `search` method, `readUrl` is now called internally. diff --git a/app-config.yaml b/app-config.yaml index 64c2fa5c4c..89e417fad3 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -244,6 +244,7 @@ integrations: secretAccessKey: ${AWS_SECRET_ACCESS_KEY} catalog: + useUrlReadersSearch: true import: entityFilename: catalog-info.yaml pullRequestBranchName: backstage-integration diff --git a/plugins/catalog-backend/config.d.ts b/plugins/catalog-backend/config.d.ts index 384765f92e..a005e7e81a 100644 --- a/plugins/catalog-backend/config.d.ts +++ b/plugins/catalog-backend/config.d.ts @@ -202,5 +202,19 @@ export interface Config { * housing catalog-info files. */ processingInterval?: HumanDuration | false; + + /** + * Defines if the UrlReaderProcessor should always call the search method of the + * different UrlReaders. + * + * If set to false, the UrlReaderProcessor will use the legacy behavior that tries to + * parse a Git URL and calls search if there's wildcard patterns and readUrl otherwise. + * + * If set to true, the UrlReaderProcessor always call the search method and lets each UrlReader + * determine if it's a search pattern or not. + * + * This flag is temporary and will be enabled by default in future releases. + */ + useUrlReadersSearch?: boolean; }; } diff --git a/plugins/catalog-backend/package.json b/plugins/catalog-backend/package.json index 32b1d1147b..a4507ba395 100644 --- a/plugins/catalog-backend/package.json +++ b/plugins/catalog-backend/package.json @@ -103,6 +103,7 @@ "@backstage/cli": "workspace:^", "@backstage/plugin-permission-common": "workspace:^", "@backstage/repo-tools": "workspace:^", + "@backstage/test-utils": "workspace:^", "@types/core-js": "^2.5.4", "@types/git-url-parse": "^9.0.0", "@types/glob": "^8.0.0", diff --git a/plugins/catalog-backend/report.api.md b/plugins/catalog-backend/report.api.md index a1391e9b44..e3cd3e6721 100644 --- a/plugins/catalog-backend/report.api.md +++ b/plugins/catalog-backend/report.api.md @@ -494,7 +494,11 @@ export function transformLegacyPolicyToProcessor( // @public (undocumented) export class UrlReaderProcessor implements CatalogProcessor_2 { - constructor(options: { reader: UrlReaderService; logger: LoggerService }); + constructor(options: { + reader: UrlReaderService; + logger: LoggerService; + config?: Config; + }); // (undocumented) getProcessorName(): string; // (undocumented) diff --git a/plugins/catalog-backend/src/processors/UrlReaderProcessor.test.ts b/plugins/catalog-backend/src/processors/UrlReaderProcessor.test.ts index 447cab40ed..1b6d7f1ca8 100644 --- a/plugins/catalog-backend/src/processors/UrlReaderProcessor.test.ts +++ b/plugins/catalog-backend/src/processors/UrlReaderProcessor.test.ts @@ -31,6 +31,8 @@ import { import { defaultEntityDataParser } from '../util/parse'; import { UrlReaderProcessor } from './UrlReaderProcessor'; import { UrlReaders } from '@backstage/backend-defaults/urlReader'; +import { UrlReaderService } from '@backstage/backend-plugin-api'; +import { mockApis } from '@backstage/test-utils'; describe('UrlReaderProcessor', () => { const mockApiOrigin = 'http://localhost'; @@ -184,7 +186,7 @@ describe('UrlReaderProcessor', () => { expect(generated.location).toBe(spec); expect(generated.error.name).toBe('NotFoundError'); expect(generated.error.message).toBe( - `Unable to read url, no matching files found for ${mockApiOrigin}/component-notfound.yaml`, + `Unable to read url, NotFoundError: could not read ${mockApiOrigin}/component-notfound.yaml, 404 Not Found`, ); }); @@ -209,4 +211,36 @@ describe('UrlReaderProcessor', () => { expect(reader.search).toHaveBeenCalledTimes(1); }); + + it('uses search when catalog.useUrlReadersSearch flag is set to true', async () => { + const logger = mockServices.logger.mock(); + + const reader: jest.Mocked = { + readUrl: jest.fn(), + readTree: jest.fn(), + search: jest.fn().mockImplementation(async () => []), + }; + + const config = mockApis.config({ + data: { + catalog: { + useUrlReadersSearch: true, + }, + }, + }); + + const processor = new UrlReaderProcessor({ reader, logger, config }); + + const emit = jest.fn(); + + await processor.readLocation( + { type: 'url', target: 'https://github.com/a/b/blob/x/b.yaml' }, + false, + emit, + defaultEntityDataParser, + mockCache, + ); + + expect(reader.search).toHaveBeenCalledTimes(1); + }); }); diff --git a/plugins/catalog-backend/src/processors/UrlReaderProcessor.ts b/plugins/catalog-backend/src/processors/UrlReaderProcessor.ts index 8852f37de4..bc383512be 100644 --- a/plugins/catalog-backend/src/processors/UrlReaderProcessor.ts +++ b/plugins/catalog-backend/src/processors/UrlReaderProcessor.ts @@ -18,6 +18,7 @@ import { Entity } from '@backstage/catalog-model'; import { assertError } from '@backstage/errors'; import limiterFactory, { Limit } from 'p-limit'; import { LocationSpec } from '@backstage/plugin-catalog-common'; +import parseGitUrl from 'git-url-parse'; import { CatalogProcessor, CatalogProcessorCache, @@ -28,6 +29,7 @@ import { processingResult, } from '@backstage/plugin-catalog-node'; import { LoggerService, UrlReaderService } from '@backstage/backend-plugin-api'; +import { Config } from '@backstage/config'; const CACHE_KEY = 'v1'; @@ -46,14 +48,25 @@ export class UrlReaderProcessor implements CatalogProcessor { // This limiter is used for only consuming a limited number of read streams // concurrently. #limiter: Limit; + #useUrlReadersSearch: boolean; constructor( private readonly options: { reader: UrlReaderService; logger: LoggerService; + config?: Config; }, ) { this.#limiter = limiterFactory(5); + + this.#useUrlReadersSearch = + this.options.config?.getOptionalBoolean('catalog.useUrlReadersSearch') || + false; + if (!this.#useUrlReadersSearch) { + this.options.logger.warn( + 'UrlReaderProcessor uses the legacy readUrl/search behavior which will be removed in a future release. Set catalog.useUrlReadersSearch to true to adopt the new behavior.', + ); + } } getProcessorName() { @@ -120,6 +133,10 @@ export class UrlReaderProcessor implements CatalogProcessor { } emit(processingResult.refresh(`${location.type}:${location.target}`)); await cache.set(CACHE_KEY, cacheItem); + } else if (error.name === 'NotFoundError') { + if (!optional) { + emit(processingResult.notFoundError(location, message)); + } } else { emit(processingResult.generalError(location, message)); } @@ -132,13 +149,35 @@ export class UrlReaderProcessor implements CatalogProcessor { location: string, etag?: string, ): Promise<{ response: { data: Buffer; url: string }[]; etag?: string }> { - const response = await this.options.reader.search(location, { etag }); + // New behavior: always use the search method + if (this.#useUrlReadersSearch) { + const response = await this.options.reader.search(location, { etag }); - const output = response.files.map(async file => ({ - url: file.url, - data: await this.#limiter(file.content), - })); + const output = response.files.map(async file => ({ + url: file.url, + data: await this.#limiter(file.content), + })); - return { response: await Promise.all(output), etag: response.etag }; + return { response: await Promise.all(output), etag: response.etag }; + } + + // Old behavior: Does it contain globs? I.e. does it contain asterisks or question marks + // (no curly braces for now) + + const { filepath } = parseGitUrl(location); + if (filepath?.match(/[*?]/)) { + const response = await this.options.reader.search(location, { etag }); + const output = response.files.map(async file => ({ + url: file.url, + data: await this.#limiter(file.content), + })); + return { response: await Promise.all(output), etag: response.etag }; + } + + const data = await this.options.reader.readUrl(location, { etag }); + return { + response: [{ url: location, data: await data.buffer() }], + etag: data.etag, + }; } } diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index bf7f31919c..4d6609dd28 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -380,7 +380,7 @@ export class CatalogBuilder { return [ new FileReaderProcessor(), - new UrlReaderProcessor({ reader, logger }), + new UrlReaderProcessor({ reader, logger, config }), CodeOwnersProcessor.fromConfig(config, { logger, reader }), new AnnotateLocationEntityProcessor({ integrations }), ]; diff --git a/yarn.lock b/yarn.lock index fb1806e230..cdbc53f9e8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6001,6 +6001,7 @@ __metadata: "@backstage/plugin-search-backend-module-catalog": "workspace:^" "@backstage/plugin-search-common": "workspace:^" "@backstage/repo-tools": "workspace:^" + "@backstage/test-utils": "workspace:^" "@backstage/types": "workspace:^" "@opentelemetry/api": ^1.9.0 "@types/core-js": ^2.5.4