From ea0f19229aa7b71245958ff59687b1861541117f Mon Sep 17 00:00:00 2001 From: Costas Drogos Date: Wed, 25 Jan 2023 12:14:47 +0100 Subject: [PATCH 1/2] GithubDiscoveryProcessor: allow filtering on forks Forks of repositories containing a catalog yml, might override discovery for the upstream repository, if both are discovered. This might not be desired in environments working extensively with forks, as it can create a lot of confusion and potentially, breakage if fork and upstream end up with different versions of the catalog yaml. This changeset provides a way to enable or disable evaluating forked repositories via Github's API `isFork` attribute. In detail, this changeset introduces a new config option, namely `allowForks` (boolean, defaulting to true) and the underlying logic, that will exclude any forked repository from the list of repositories to be discovered, when set to false. Signed-off-by: Costas Drogos --- .../catalog-backend-module-github/config.d.ts | 8 +++++ .../src/lib/github.test.ts | 4 +++ .../src/lib/github.ts | 2 ++ .../src/lib/util.test.ts | 24 ++++++++++++- .../src/lib/util.ts | 11 ++++++ .../GithubDiscoveryProcessor.test.ts | 13 +++++++ .../providers/GithubEntityProvider.test.ts | 8 +++++ .../src/providers/GithubEntityProvider.ts | 7 +++- .../GithubEntityProviderConfig.test.ts | 34 +++++++++++++++++-- .../providers/GithubEntityProviderConfig.ts | 3 ++ 10 files changed, 110 insertions(+), 4 deletions(-) diff --git a/plugins/catalog-backend-module-github/config.d.ts b/plugins/catalog-backend-module-github/config.d.ts index 691caedc46..37852223c1 100644 --- a/plugins/catalog-backend-module-github/config.d.ts +++ b/plugins/catalog-backend-module-github/config.d.ts @@ -86,6 +86,10 @@ export interface Config { /** * (Optional) GitHub topic-based filters. */ + allowForks?: boolean; + /** + * (Optional) Allow Forks to be evaluated. + */ topic?: { /** * (Optional) An array of strings used to filter in results based on their associated GitHub topics. @@ -143,6 +147,10 @@ export interface Config { /** * (Optional) GitHub topic-based filters. */ + allowForks?: boolean; + /** + * (Optional) Allow Forks to be evaluated. + */ topic?: { /** * (Optional) An array of strings used to filter in results based on their associated GitHub topics. diff --git a/plugins/catalog-backend-module-github/src/lib/github.test.ts b/plugins/catalog-backend-module-github/src/lib/github.test.ts index 387dc8a2ee..624ea5c3c9 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.test.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.test.ts @@ -488,6 +488,7 @@ describe('github', () => { name: 'backstage', url: 'https://github.com/backstage/backstage', isArchived: false, + isFork: false, repositoryTopics: { nodes: [{ topic: { name: 'blah' } }], }, @@ -500,6 +501,7 @@ describe('github', () => { name: 'demo', url: 'https://github.com/backstage/demo', isArchived: true, + isFork: true, repositoryTopics: { nodes: [] }, defaultBranchRef: { name: 'main', @@ -524,6 +526,7 @@ describe('github', () => { name: 'backstage', url: 'https://github.com/backstage/backstage', isArchived: false, + isFork: false, repositoryTopics: { nodes: [{ topic: { name: 'blah' } }], }, @@ -536,6 +539,7 @@ describe('github', () => { name: 'demo', url: 'https://github.com/backstage/demo', isArchived: true, + isFork: true, repositoryTopics: { nodes: [] }, defaultBranchRef: { name: 'main', diff --git a/plugins/catalog-backend-module-github/src/lib/github.ts b/plugins/catalog-backend-module-github/src/lib/github.ts index f719e3d506..ae6e14fc23 100644 --- a/plugins/catalog-backend-module-github/src/lib/github.ts +++ b/plugins/catalog-backend-module-github/src/lib/github.ts @@ -89,6 +89,7 @@ export type RepositoryResponse = { name: string; url: string; isArchived: boolean; + isFork: boolean; repositoryTopics: RepositoryTopics; defaultBranchRef: { name: string; @@ -435,6 +436,7 @@ export async function getOrganizationRepositories( } url isArchived + isFork repositoryTopics(first: 100) { nodes { ... on RepositoryTopic { diff --git a/plugins/catalog-backend-module-github/src/lib/util.test.ts b/plugins/catalog-backend-module-github/src/lib/util.test.ts index 7aca6839ea..0de1d24bbf 100644 --- a/plugins/catalog-backend-module-github/src/lib/util.test.ts +++ b/plugins/catalog-backend-module-github/src/lib/util.test.ts @@ -15,7 +15,11 @@ */ import { GithubTopicFilters } from '../providers/GithubEntityProviderConfig'; -import { parseGithubOrgUrl, satisfiesTopicFilter } from './util'; +import { + parseGithubOrgUrl, + satisfiesTopicFilter, + satisfiesForkFilter, +} from './util'; describe('parseGithubOrgUrl', () => { it('only supports clean org urls, and decodes them', () => { @@ -87,3 +91,21 @@ describe('satisfiesTopicFilter', () => { ).toEqual(false); }); }); + +describe('satisfiesForkFilter', () => { + it('handles cases where forks are not allowed and a fork is evaluated', () => { + expect(satisfiesForkFilter(false, true)).toEqual(false); + }); + + it('handles cases where forks are not allowed and a fork is not evaluated', () => { + expect(satisfiesForkFilter(false, false)).toEqual(true); + }); + + it('handles cases where forks are allowed and a fork is evaluated', () => { + expect(satisfiesForkFilter(true, true)).toEqual(true); + }); + + it('handles cases where forks are allowed and a fork is not evaluated', () => { + expect(satisfiesForkFilter(true, false)).toEqual(true); + }); +}); diff --git a/plugins/catalog-backend-module-github/src/lib/util.ts b/plugins/catalog-backend-module-github/src/lib/util.ts index ece463bb40..5c0297c0cc 100644 --- a/plugins/catalog-backend-module-github/src/lib/util.ts +++ b/plugins/catalog-backend-module-github/src/lib/util.ts @@ -77,3 +77,14 @@ export function satisfiesTopicFilter( // not configured at all. return true; } + +export function satisfiesForkFilter( + allowForks: boolean | true, + isFork: boolean | false, +): Boolean { + // we don't want to include forks if forks are not allowed + if (!allowForks && isFork) return false; + + // if forks are allowed, allow all repos through + return true; +} diff --git a/plugins/catalog-backend-module-github/src/processors/GithubDiscoveryProcessor.test.ts b/plugins/catalog-backend-module-github/src/processors/GithubDiscoveryProcessor.test.ts index b69728cc62..f30007c677 100644 --- a/plugins/catalog-backend-module-github/src/processors/GithubDiscoveryProcessor.test.ts +++ b/plugins/catalog-backend-module-github/src/processors/GithubDiscoveryProcessor.test.ts @@ -150,6 +150,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/backstage', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'master', }, @@ -160,6 +161,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/demo', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -202,6 +204,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/tech-docs', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -236,6 +239,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/tech-docs', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: null, catalogInfoFile: null, }, @@ -260,6 +264,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/backstage', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'master', }, @@ -295,6 +300,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/backstage', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -305,6 +311,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/techdocs-cli', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -315,6 +322,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/techdocs-container', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -325,6 +333,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/techdocs-durp', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: null, catalogInfoFile: null, }, @@ -365,6 +374,7 @@ describe('GithubDiscoveryProcessor', () => { name: 'abstest', url: 'https://github.com/backstage/abctest', isArchived: false, + isFork: false, repositoryTopics: { nodes: [] }, defaultBranchRef: { name: 'main', @@ -375,6 +385,7 @@ describe('GithubDiscoveryProcessor', () => { name: 'test', url: 'https://github.com/backstage/test', isArchived: false, + isFork: false, repositoryTopics: { nodes: [] }, defaultBranchRef: { name: 'main', @@ -386,6 +397,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/test', repositoryTopics: { nodes: [] }, isArchived: true, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -396,6 +408,7 @@ describe('GithubDiscoveryProcessor', () => { url: 'https://github.com/backstage/testxyz', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, diff --git a/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.test.ts index 9916825cdc..2be5df09fe 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.test.ts @@ -172,6 +172,7 @@ describe('GithubEntityProvider', () => { url: 'https://github.com/test-org/test-repo', repositoryTopics: { nodes: [] }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -274,6 +275,7 @@ describe('GithubEntityProvider', () => { ], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -353,6 +355,7 @@ describe('GithubEntityProvider', () => { ], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -445,6 +448,7 @@ describe('GithubEntityProvider', () => { nodes: [], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -457,6 +461,7 @@ describe('GithubEntityProvider', () => { nodes: [], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -557,6 +562,7 @@ describe('GithubEntityProvider', () => { ], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -580,6 +586,7 @@ describe('GithubEntityProvider', () => { ], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, @@ -600,6 +607,7 @@ describe('GithubEntityProvider', () => { ], }, isArchived: false, + isFork: false, defaultBranchRef: { name: 'main', }, diff --git a/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.ts index 1344e2f5d1..f84f401191 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubEntityProvider.ts @@ -40,7 +40,7 @@ import { GithubEntityProviderConfig, } from './GithubEntityProviderConfig'; import { getOrganizationRepositories } from '../lib/github'; -import { satisfiesTopicFilter } from '../lib/util'; +import { satisfiesTopicFilter, satisfiesForkFilter } from '../lib/util'; import { EventParams, EventSubscriber } from '@backstage/plugin-events-node'; import { PushEvent, Commit } from '@octokit/webhooks-types'; @@ -52,6 +52,7 @@ type Repository = { name: string; url: string; isArchived: boolean; + isFork: boolean; repositoryTopics: string[]; defaultBranchRef?: string; isCatalogInfoFilePresent: boolean; @@ -216,6 +217,7 @@ export class GithubEntityProvider implements EntityProvider, EventSubscriber { defaultBranchRef: r.defaultBranchRef?.name, repositoryTopics: r.repositoryTopics.nodes.map(t => t.topic.name), isArchived: r.isArchived, + isFork: r.isFork, isCatalogInfoFilePresent: r.catalogInfoFile?.__typename === 'Blob' && r.catalogInfoFile.text !== '', @@ -234,6 +236,7 @@ export class GithubEntityProvider implements EntityProvider, EventSubscriber { private matchesFilters(repositories: Repository[]) { const repositoryFilter = this.config.filters?.repository; const topicFilters = this.config.filters?.topic; + const allowForks = this.config.filters?.allowForks ?? true; const matchingRepositories = repositories.filter(r => { const repoTopics: string[] = r.repositoryTopics; @@ -241,6 +244,7 @@ export class GithubEntityProvider implements EntityProvider, EventSubscriber { !r.isArchived && (!repositoryFilter || repositoryFilter.test(r.name)) && satisfiesTopicFilter(repoTopics, topicFilters) && + satisfiesForkFilter(allowForks, r.isFork) && r.defaultBranchRef ); }); @@ -302,6 +306,7 @@ export class GithubEntityProvider implements EntityProvider, EventSubscriber { defaultBranchRef: event.repository.default_branch, repositoryTopics: event.repository.topics, isArchived: event.repository.archived, + isFork: event.repository.fork, // we can consider this file present because // only the catalog file will be recovered from the commits isCatalogInfoFilePresent: true, diff --git a/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.test.ts index 0d9b5ce65b..a597e9dbe0 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.test.ts @@ -78,6 +78,12 @@ describe('readProviderConfigs', () => { }, }, }, + providerWithForkFilter: { + organization: 'test-org6', + filters: { + allowForks: false, + }, + }, providerWithHost: { organization: 'test-org1', host: 'ghe.internal.com', @@ -97,7 +103,7 @@ describe('readProviderConfigs', () => { }); const providerConfigs = readProviderConfigs(config); - expect(providerConfigs).toHaveLength(7); + expect(providerConfigs).toHaveLength(8); expect(providerConfigs[0]).toEqual({ id: 'providerOrganizationOnly', organization: 'test-org1', @@ -106,6 +112,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: undefined, + allowForks: true, topic: { include: undefined, exclude: undefined, @@ -122,6 +129,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: undefined, + allowForks: true, topic: { include: undefined, exclude: undefined, @@ -138,6 +146,7 @@ describe('readProviderConfigs', () => { filters: { repository: /^repository.*filter$/, // repo branch: undefined, // branch + allowForks: true, topic: { include: undefined, exclude: undefined, @@ -154,6 +163,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: 'branch-name', + allowForks: true, topic: { include: undefined, exclude: undefined, @@ -170,6 +180,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: undefined, + allowForks: true, topic: { include: ['backstage-include'], exclude: ['backstage-exclude'], @@ -179,6 +190,23 @@ describe('readProviderConfigs', () => { validateLocationsExist: false, }); expect(providerConfigs[5]).toEqual({ + id: 'providerWithForkFilter', + organization: 'test-org6', + catalogPath: '/catalog-info.yaml', + host: 'github.com', + filters: { + repository: undefined, + branch: undefined, + allowForks: false, + topic: { + include: undefined, + exclude: undefined, + }, + }, + schedule: undefined, + validateLocationsExist: false, + }); + expect(providerConfigs[6]).toEqual({ id: 'providerWithHost', organization: 'test-org1', catalogPath: '/catalog-info.yaml', @@ -186,6 +214,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: undefined, + allowForks: true, topic: { include: undefined, exclude: undefined, @@ -194,7 +223,7 @@ describe('readProviderConfigs', () => { validateLocationsExist: false, schedule: undefined, }); - expect(providerConfigs[6]).toEqual({ + expect(providerConfigs[7]).toEqual({ id: 'providerWithSchedule', organization: 'test-org1', catalogPath: '/catalog-info.yaml', @@ -202,6 +231,7 @@ describe('readProviderConfigs', () => { filters: { repository: undefined, branch: undefined, + allowForks: true, topic: { include: undefined, exclude: undefined, diff --git a/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.ts b/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.ts index e888743258..a79a557ebf 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubEntityProviderConfig.ts @@ -32,6 +32,7 @@ export type GithubEntityProviderConfig = { repository?: RegExp; branch?: string; topic?: GithubTopicFilters; + allowForks?: boolean; }; validateLocationsExist: boolean; schedule?: TaskScheduleDefinition; @@ -72,6 +73,7 @@ function readProviderConfig( const host = config.getOptionalString('host') ?? 'github.com'; const repositoryPattern = config.getOptionalString('filters.repository'); const branchPattern = config.getOptionalString('filters.branch'); + const allowForks = config.getOptionalBoolean('filters.allowForks') ?? true; const topicFilterInclude = config?.getOptionalStringArray( 'filters.topic.include', ); @@ -103,6 +105,7 @@ function readProviderConfig( ? compileRegExp(repositoryPattern) : undefined, branch: branchPattern || undefined, + allowForks: allowForks, topic: { include: topicFilterInclude, exclude: topicFilterExclude, From 66158754b4aa18579f13e5b8d088d850a38d7b5f Mon Sep 17 00:00:00 2001 From: Costas Drogos Date: Wed, 25 Jan 2023 12:18:57 +0100 Subject: [PATCH 2/2] Add changeset Signed-off-by: Costas Drogos --- .changeset/hot-lions-cover.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/hot-lions-cover.md diff --git a/.changeset/hot-lions-cover.md b/.changeset/hot-lions-cover.md new file mode 100644 index 0000000000..62064188a4 --- /dev/null +++ b/.changeset/hot-lions-cover.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-github': patch +--- + +Add support for filtering out forks