From 26ad3398a876d7f683a0855001262359d1d92ce5 Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Thu, 19 Jan 2023 10:26:22 +0100 Subject: [PATCH 1/8] add pattern matching Signed-off-by: Kiss Miklos --- .../src/modules/codeowners/CodeOwnersProcessor.ts | 4 +++- plugins/catalog-backend/src/modules/codeowners/lib/read.ts | 3 ++- plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts index 186b732271..f50ef6b922 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts @@ -79,11 +79,13 @@ export class CodeOwnersProcessor implements CatalogProcessor { if (!scmIntegration) { return entity; } - + const pattern = + entity.metadata?.annotations['backstage.io/managed-by-location']; const owner = await findCodeOwnerByTarget( this.reader, location.target, scmIntegration, + pattern, ); if (!owner) { diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts index 2987e162cc..eae4e7ce91 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts @@ -52,6 +52,7 @@ export async function findCodeOwnerByTarget( reader: UrlReader, targetUrl: string, scmIntegration: ScmIntegration, + pattern: string, ): Promise { const codeownersPaths = scmCodeOwnersPaths[scmIntegration?.type ?? '']; @@ -70,7 +71,7 @@ export async function findCodeOwnerByTarget( return undefined; } - const owner = resolveCodeOwner(contents); + const owner = resolveCodeOwner(contents, pattern); return owner; } diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts index 00645f0c9d..f9bc19b1d1 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts @@ -29,7 +29,7 @@ export function resolveCodeOwner( const owners = codeowners.parse(contents); return pipe( - filter((e: CodeOwnersEntry) => e.pattern === pattern), + filter((e: CodeOwnersEntry) => pattern.includes(e.pattern)), reverse, head, get('owners'), From 54cffaf07c5cee318eaaea9011171891fb21feef Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Feb 2023 17:53:57 +0100 Subject: [PATCH 2/8] add pattern match Signed-off-by: Kiss Miklos --- .../codeowners/CodeOwnersProcessor.test.ts | 27 ++++++++++++++++--- .../modules/codeowners/CodeOwnersProcessor.ts | 4 ++- .../src/modules/codeowners/lib/read.ts | 2 +- .../modules/codeowners/lib/resolve.test.ts | 10 ++++++- .../src/modules/codeowners/lib/resolve.ts | 18 +++++++++---- 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts index 9cb4d414c0..a33ce84e0d 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts @@ -21,7 +21,7 @@ import { LocationSpec } from '@backstage/plugin-catalog-node'; const mockCodeOwnersText = () => ` * @acme/team-foo @acme/team-bar -/docs @acme/team-bar +/docs @acme/team-docs `; describe('CodeOwnersProcessor', () => { @@ -34,8 +34,12 @@ describe('CodeOwnersProcessor', () => { }); describe('preProcessEntity', () => { - const setupTest = ({ kind = 'Component', spec = {} } = {}) => { - const entity = { kind, spec }; + const setupTest = ({ + kind = 'Component', + spec = {}, + metadata = {}, + } = {}) => { + const entity = { kind, spec, metadata }; const config = new ConfigReader({}); const reader = { read: jest.fn(), @@ -101,5 +105,22 @@ describe('CodeOwnersProcessor', () => { spec: { owner: 'team-foo' }, }); }); + it('should use the "backstage.io/managed-by-location" annotation as pattern', async () => { + const { entity, processor } = setupTest({ + metadata: { + annotations: { 'backstage.io/managed-by-location': '/docs' }, + }, + }); + + const result = await processor.preProcessEntity( + entity as any, + mockLocation(), + ); + + expect(result).toEqual({ + ...entity, + spec: { owner: 'team-docs' }, + }); + }); }); }); diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts index f50ef6b922..9a9c58934c 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts @@ -79,8 +79,10 @@ export class CodeOwnersProcessor implements CatalogProcessor { if (!scmIntegration) { return entity; } + const pattern = - entity.metadata?.annotations['backstage.io/managed-by-location']; + entity.metadata?.annotations?.['backstage.io/managed-by-location']; + const owner = await findCodeOwnerByTarget( this.reader, location.target, diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts index eae4e7ce91..cd15a17997 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts @@ -52,7 +52,7 @@ export async function findCodeOwnerByTarget( reader: UrlReader, targetUrl: string, scmIntegration: ScmIntegration, - pattern: string, + pattern?: string, ): Promise { const codeownersPaths = scmCodeOwnersPaths[scmIntegration?.type ?? '']; diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts index 4e6c646073..ec9321284f 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts @@ -18,13 +18,21 @@ import { normalizeCodeOwner, resolveCodeOwner } from './resolve'; const mockCodeOwnersText = () => ` * @acme/team-foo @acme/team-bar -/docs @acme/team-bar +/docs @acme/team-docs `; describe('resolveCodeOwner', () => { it('should parse the codeowners file', () => { expect(resolveCodeOwner(mockCodeOwnersText())).toBe('team-foo'); }); + it('should include the codeowners path into the provided pattern', () => { + expect( + resolveCodeOwner( + mockCodeOwnersText(), + 'url:https://github.com/acem/repo/main/docs/catalog-info.yaml', + ), + ).toBe('team-docs'); + }); }); describe('normalizeCodeOwner', () => { diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts index f9bc19b1d1..69865fde21 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts @@ -16,7 +16,7 @@ import * as codeowners from 'codeowners-utils'; import { CodeOwnersEntry } from 'codeowners-utils'; -import { filter, get, head, pipe, reverse } from 'lodash/fp'; +import { get, head, pipe, reverse } from 'lodash/fp'; const USER_PATTERN = /^@.*/; const GROUP_PATTERN = /^@.*\/.*/; @@ -24,18 +24,26 @@ const EMAIL_PATTERN = /^.*@.*\..*$/; export function resolveCodeOwner( contents: string, - pattern = '*', + pattern?: string, ): string | undefined { const owners = codeowners.parse(contents); - return pipe( - filter((e: CodeOwnersEntry) => pattern.includes(e.pattern)), + let relevantOwners = owners.filter((e: CodeOwnersEntry) => + pattern?.includes(e.pattern), + ); + if (relevantOwners.length === 0) { + relevantOwners = owners.filter((e: CodeOwnersEntry) => e.pattern === '*'); + } + + const result = pipe( reverse, head, get('owners'), head, normalizeCodeOwner, - )(owners); + )(relevantOwners); + + return result; } export function normalizeCodeOwner(owner: string) { From 92a4590fc3ab0ee02ae8a0ea93d8dbf4aa6411b2 Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Feb 2023 18:17:50 +0100 Subject: [PATCH 3/8] add changeset Signed-off-by: Kiss Miklos --- .changeset/spicy-carrots-argue.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/spicy-carrots-argue.md diff --git a/.changeset/spicy-carrots-argue.md b/.changeset/spicy-carrots-argue.md new file mode 100644 index 0000000000..559335a646 --- /dev/null +++ b/.changeset/spicy-carrots-argue.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': minor +--- + +Add support to CodeOwnersProcces From f09e37af60f42b24b919585d47199a7785c9d89e Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Feb 2023 18:18:25 +0100 Subject: [PATCH 4/8] add more info to changeset Signed-off-by: Kiss Miklos --- .changeset/spicy-carrots-argue.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/spicy-carrots-argue.md b/.changeset/spicy-carrots-argue.md index 559335a646..b6475b1f7d 100644 --- a/.changeset/spicy-carrots-argue.md +++ b/.changeset/spicy-carrots-argue.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend': minor --- -Add support to CodeOwnersProcces +Add monorepo support to CodeOwnersProccesor. From 76df7bd736dc79b42cf7bdff759904c34c188d4b Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Feb 2023 18:19:54 +0100 Subject: [PATCH 5/8] use more descriptive name Signed-off-by: Kiss Miklos --- .../catalog-backend/src/modules/codeowners/lib/resolve.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts index 69865fde21..139ea881d8 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts @@ -28,11 +28,11 @@ export function resolveCodeOwner( ): string | undefined { const owners = codeowners.parse(contents); - let relevantOwners = owners.filter((e: CodeOwnersEntry) => + let filteredOwners = owners.filter((e: CodeOwnersEntry) => pattern?.includes(e.pattern), ); - if (relevantOwners.length === 0) { - relevantOwners = owners.filter((e: CodeOwnersEntry) => e.pattern === '*'); + if (filteredOwners.length === 0) { + filteredOwners = owners.filter((e: CodeOwnersEntry) => e.pattern === '*'); } const result = pipe( @@ -41,7 +41,7 @@ export function resolveCodeOwner( get('owners'), head, normalizeCodeOwner, - )(relevantOwners); + )(filteredOwners); return result; } From c85b7360161c31487b18ddfad73ae8247a566aeb Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Mar 2023 16:27:48 +0100 Subject: [PATCH 6/8] use more robust matching for CODEOWNERS entries Signed-off-by: Kiss Miklos --- .../codeowners/CodeOwnersProcessor.test.ts | 8 ++--- .../modules/codeowners/CodeOwnersProcessor.ts | 4 --- .../src/modules/codeowners/lib/read.ts | 3 +- .../modules/codeowners/lib/resolve.test.ts | 17 +++++++++-- .../src/modules/codeowners/lib/resolve.ts | 29 +++++++------------ 5 files changed, 29 insertions(+), 32 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts index a33ce84e0d..8e1fbca726 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts @@ -106,15 +106,11 @@ describe('CodeOwnersProcessor', () => { }); }); it('should use the "backstage.io/managed-by-location" annotation as pattern', async () => { - const { entity, processor } = setupTest({ - metadata: { - annotations: { 'backstage.io/managed-by-location': '/docs' }, - }, - }); + const { entity, processor } = setupTest(); const result = await processor.preProcessEntity( entity as any, - mockLocation(), + mockLocation({ basePath: 'docs/' }), ); expect(result).toEqual({ diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts index 9a9c58934c..186b732271 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.ts @@ -80,14 +80,10 @@ export class CodeOwnersProcessor implements CatalogProcessor { return entity; } - const pattern = - entity.metadata?.annotations?.['backstage.io/managed-by-location']; - const owner = await findCodeOwnerByTarget( this.reader, location.target, scmIntegration, - pattern, ); if (!owner) { diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts index cd15a17997..6a5234c94f 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/read.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/read.ts @@ -52,7 +52,6 @@ export async function findCodeOwnerByTarget( reader: UrlReader, targetUrl: string, scmIntegration: ScmIntegration, - pattern?: string, ): Promise { const codeownersPaths = scmCodeOwnersPaths[scmIntegration?.type ?? '']; @@ -71,7 +70,7 @@ export async function findCodeOwnerByTarget( return undefined; } - const owner = resolveCodeOwner(contents, pattern); + const owner = resolveCodeOwner(contents, targetUrl); return owner; } diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts index ec9321284f..f9cdf79530 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.test.ts @@ -23,16 +23,29 @@ const mockCodeOwnersText = () => ` describe('resolveCodeOwner', () => { it('should parse the codeowners file', () => { - expect(resolveCodeOwner(mockCodeOwnersText())).toBe('team-foo'); + expect( + resolveCodeOwner( + mockCodeOwnersText(), + 'https://github.com/can/be/tree/anything/catalog-info.yaml', + ), + ).toBe('team-foo'); }); it('should include the codeowners path into the provided pattern', () => { expect( resolveCodeOwner( mockCodeOwnersText(), - 'url:https://github.com/acem/repo/main/docs/catalog-info.yaml', + 'https://github.com/acme/repo/tree/main/docs/catalog-info.yaml', ), ).toBe('team-docs'); }); + it('should match only in resource path', () => { + expect( + resolveCodeOwner( + mockCodeOwnersText(), + 'https://github.com/acme/repo/tree/docs/catalog-info.yaml', + ), + ).toBe('team-foo'); + }); }); describe('normalizeCodeOwner', () => { diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts index 139ea881d8..92fa4a0ef6 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts @@ -15,8 +15,7 @@ */ import * as codeowners from 'codeowners-utils'; -import { CodeOwnersEntry } from 'codeowners-utils'; -import { get, head, pipe, reverse } from 'lodash/fp'; +import parseGitUrl from 'git-url-parse'; const USER_PATTERN = /^@.*/; const GROUP_PATTERN = /^@.*\/.*/; @@ -24,26 +23,20 @@ const EMAIL_PATTERN = /^.*@.*\..*$/; export function resolveCodeOwner( contents: string, - pattern?: string, + catalogInfoFileUrl: string, ): string | undefined { - const owners = codeowners.parse(contents); + const codeOwnerEntries = codeowners.parse(contents); - let filteredOwners = owners.filter((e: CodeOwnersEntry) => - pattern?.includes(e.pattern), - ); - if (filteredOwners.length === 0) { - filteredOwners = owners.filter((e: CodeOwnersEntry) => e.pattern === '*'); + const { filepath } = parseGitUrl(catalogInfoFileUrl); + const match = codeowners.matchFile(filepath, codeOwnerEntries); + + if (!match) { + throw new Error( + `There is no matching entry for path: ${filepath} in the CODEOWNERS file`, + ); } - const result = pipe( - reverse, - head, - get('owners'), - head, - normalizeCodeOwner, - )(filteredOwners); - - return result; + return normalizeCodeOwner(match.owners[0]); } export function normalizeCodeOwner(owner: string) { From 9d6c429801c2ae1bddad6ae1694b9a2caff01f9e Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 6 Mar 2023 18:45:30 +0100 Subject: [PATCH 7/8] add glob and wildcard tests Signed-off-by: Kiss Miklos --- .../codeowners/CodeOwnersProcessor.test.ts | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts index 8e1fbca726..6c2774a6fb 100644 --- a/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts +++ b/plugins/catalog-backend/src/modules/codeowners/CodeOwnersProcessor.test.ts @@ -17,11 +17,13 @@ import { getVoidLogger } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; import { CodeOwnersProcessor } from './CodeOwnersProcessor'; -import { LocationSpec } from '@backstage/plugin-catalog-node'; +import { LocationSpec } from '@backstage/plugin-catalog-common'; const mockCodeOwnersText = () => ` -* @acme/team-foo @acme/team-bar -/docs @acme/team-docs +* @acme/team-foo @acme/team-bar +/docs @acme/team-docs +/plugins/catalog-* @backstage/maintainers @backstage/catalog-core +**/logs @logs-owner `; describe('CodeOwnersProcessor', () => { @@ -105,7 +107,7 @@ describe('CodeOwnersProcessor', () => { spec: { owner: 'team-foo' }, }); }); - it('should use the "backstage.io/managed-by-location" annotation as pattern', async () => { + it('should match owner based on the targetUrl', async () => { const { entity, processor } = setupTest(); const result = await processor.preProcessEntity( @@ -118,5 +120,31 @@ describe('CodeOwnersProcessor', () => { spec: { owner: 'team-docs' }, }); }); + it('should match wildcard pattern', async () => { + const { entity, processor } = setupTest(); + + const result = await processor.preProcessEntity( + entity as any, + mockLocation({ basePath: 'plugins/catalog-foo' }), + ); + + expect(result).toEqual({ + ...entity, + spec: { owner: 'maintainers' }, + }); + }); + it('should match glob pattern', async () => { + const { entity, processor } = setupTest(); + + const result = await processor.preProcessEntity( + entity as any, + mockLocation({ basePath: 'plugins/catalog-foo/logs/1.txt' }), + ); + + expect(result).toEqual({ + ...entity, + spec: { owner: 'User:logs-owner' }, + }); + }); }); }); From f235e4064fc5358faccafb51b73d5f685a99bc8a Mon Sep 17 00:00:00 2001 From: Kiss Miklos Date: Mon, 13 Mar 2023 10:18:46 +0100 Subject: [PATCH 8/8] return undefined if there is no match Signed-off-by: Kiss Miklos --- .../catalog-backend/src/modules/codeowners/lib/resolve.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts index 92fa4a0ef6..675f6542aa 100644 --- a/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts +++ b/plugins/catalog-backend/src/modules/codeowners/lib/resolve.ts @@ -30,13 +30,7 @@ export function resolveCodeOwner( const { filepath } = parseGitUrl(catalogInfoFileUrl); const match = codeowners.matchFile(filepath, codeOwnerEntries); - if (!match) { - throw new Error( - `There is no matching entry for path: ${filepath} in the CODEOWNERS file`, - ); - } - - return normalizeCodeOwner(match.owners[0]); + return match ? normalizeCodeOwner(match.owners[0]) : undefined; } export function normalizeCodeOwner(owner: string) {