From 03afcf1db50d156536c251d0804a4d1a6fe9d689 Mon Sep 17 00:00:00 2001 From: ElaineDeMattosSilvaB Date: Mon, 20 May 2024 22:24:11 +0200 Subject: [PATCH 1/4] fix: remove custom fallback Signed-off-by: ElaineDeMattosSilvaB --- .../src/__testUtils__/handlers.ts | 2 +- .../src/__testUtils__/mocks.ts | 115 +++++++++++++++++- .../GitlabDiscoveryEntityProvider.test.ts | 27 ++-- .../GitlabDiscoveryEntityProvider.ts | 8 +- 4 files changed, 123 insertions(+), 29 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts index 00d4994e94..f2dcac13cd 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts @@ -165,7 +165,7 @@ const httpProjectCatalogDynamic = all_projects_response.map(project => { `${apiBaseUrl}/projects/${path}/repository/files/catalog-info.yaml`, (req, res, ctx) => { const branch = req.url.searchParams.get('ref'); - if (branch === project.default_branch) { + if (branch === (project.default_branch || 'main' || 'develop')) { return res(ctx.status(200)); } return res(ctx.status(404, 'Not Found')); diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts index 5a078ab689..279da0dfc0 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts @@ -190,6 +190,33 @@ export const config_single_integration_branch: MockObject = { }, }; +export const config_single_integration_specific_branch: MockObject = { + integrations: { + gitlab: [ + { + host: 'example.com', + apiBaseUrl: 'https://example.com/api/v4', + token: '1234', + }, + ], + }, + catalog: { + providers: { + gitlab: { + 'test-id': { + host: 'example.com', + group: 'group1', + branch: 'develop', + skipForkedRepos: false, + schedule: { + frequency: 'PT30M', + timeout: 'PT3M', + }, + }, + }, + }, + }, +}; export const config_single_integration_group: MockObject = { integrations: { gitlab: [ @@ -234,7 +261,7 @@ export const config_fallbackBranch_branch: MockObject = { 'test-id': { host: 'example.com', group: 'group1', - fallbackBranch: 'staging', + fallbackBranch: 'main', skipForkedRepos: false, schedule: { frequency: 'PT30M', @@ -634,7 +661,7 @@ export const all_projects_response: GitLabProject[] = [ web_url: 'https://example.com/group1/test-repo5-staging', path_with_namespace: 'group1/test-repo5-staging', }, - // diffrent group + // different group { id: 6, description: 'Project Six Description', @@ -646,6 +673,17 @@ export const all_projects_response: GitLabProject[] = [ web_url: 'https://example.com/group1/test-repo6', path_with_namespace: 'awesome-group/test-repo6', }, + // no default branch + { + id: 7, + description: 'Project Seven Description', + name: 'test-repo7', + path: 'test-repo7', + archived: false, + last_activity_at: new Date().toString(), + web_url: 'https://example.com/group1/test-repo7', + path_with_namespace: 'group1/test-repo7', + }, ]; export const all_users_response: GitLabUser[] = [ @@ -1299,9 +1337,43 @@ export const push_modif_event: EventParams = { /** * Expected Backstage entities */ -export const expected_location_entities: MockObject[] = + +// includes only projects that have a default branch (for when the branch and default branch were not set in the config) +export const expected_location_entities_default_branch: MockObject[] = + all_projects_response + .filter(project => project.default_branch) + .map(project => { + const targetUrl = `https://example.com/${project.path_with_namespace}/-/blob/${project.default_branch}/catalog-info.yaml`; + + return { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Location', + metadata: { + annotations: { + 'backstage.io/managed-by-location': `url:${targetUrl}`, + 'backstage.io/managed-by-origin-location': `url:${targetUrl}`, + }, + name: locationSpecToMetadataName({ + target: targetUrl, + type: 'url', + }), + }, + spec: { + presence: 'optional', + target: targetUrl, + type: 'url', + }, + }, + locationKey: 'GitlabDiscoveryEntityProvider:test-id', + }; + }); + +// includes every GitLab project that has a default branch and the fallback declared in the config +export const expected_location_entities_fallback_branch: MockObject[] = all_projects_response.map(project => { - const targetUrl = `https://example.com/${project.path_with_namespace}/-/blob/${project.default_branch}/catalog-info.yaml`; + const branch = project.default_branch || 'main'; + const targetUrl = `https://example.com/${project.path_with_namespace}/-/blob/${branch}/catalog-info.yaml`; return { entity: { @@ -1312,7 +1384,40 @@ export const expected_location_entities: MockObject[] = 'backstage.io/managed-by-location': `url:${targetUrl}`, 'backstage.io/managed-by-origin-location': `url:${targetUrl}`, }, - name: locationSpecToMetadataName({ target: targetUrl, type: 'url' }), + name: locationSpecToMetadataName({ + target: targetUrl, + type: 'url', + }), + }, + spec: { + presence: 'optional', + target: targetUrl, + type: 'url', + }, + }, + locationKey: 'GitlabDiscoveryEntityProvider:test-id', + }; + }); + +// includes ONLY the projects with the branch declared in the config +export const expected_location_entities_specific_branch: MockObject[] = + all_projects_response.map(project => { + const branch = 'develop'; + const targetUrl = `https://example.com/${project.path_with_namespace}/-/blob/${branch}/catalog-info.yaml`; + + return { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Location', + metadata: { + annotations: { + 'backstage.io/managed-by-location': `url:${targetUrl}`, + 'backstage.io/managed-by-origin-location': `url:${targetUrl}`, + }, + name: locationSpecToMetadataName({ + target: targetUrl, + type: 'url', + }), }, spec: { presence: 'optional', diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts index 7167fc1c2d..0c20732fa7 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts @@ -153,7 +153,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter( + entities: mock.expected_location_entities_default_branch.filter( entity => !entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' @@ -187,7 +187,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter( + entities: mock.expected_location_entities_default_branch.filter( entity => entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' @@ -217,7 +217,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter( + entities: mock.expected_location_entities_default_branch.filter( entity => !entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' @@ -229,8 +229,10 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { }); }); - it('should filter found projects based on the branch', async () => { - const config = new ConfigReader(mock.config_single_integration_branch); + it('should only ingest projects from specific branch', async () => { + const config = new ConfigReader( + mock.config_single_integration_specific_branch, + ); const schedule = new PersistingTaskRunner(); const entityProviderConnection: EntityProviderConnection = { applyMutation: jest.fn(), @@ -251,7 +253,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter( + entities: mock.expected_location_entities_specific_branch.filter( entity => entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' @@ -263,7 +265,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { }); }); - it('should only include projects with fallback branch', async () => { + it('should include projects from fallback branch', async () => { const config = new ConfigReader(mock.config_fallbackBranch_branch); const schedule = new PersistingTaskRunner(); const entityProviderConnection: EntityProviderConnection = { @@ -275,21 +277,14 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { schedule, })[0]; - const configured_branch = - mock.config_fallbackBranch_branch.catalog.providers.gitlab['test-id'] - .fallbackBranch; - await provider.connect(entityProviderConnection); await provider.refresh(logger); expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter( + entities: mock.expected_location_entities_fallback_branch.filter( entity => - entity.entity.metadata.annotations[ - 'backstage.io/managed-by-location' - ].includes(configured_branch) && !entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' ].includes('awesome'), @@ -319,7 +314,7 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'full', - entities: mock.expected_location_entities.filter(entity => + entities: mock.expected_location_entities_default_branch.filter(entity => entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' ].includes(configured_group), diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts index 73ff5f3a01..b2ef475d9d 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { LoggerService } from '@backstage/backend-plugin-api'; import { PluginTaskScheduler, TaskRunner } from '@backstage/backend-tasks'; import { Config } from '@backstage/config'; import { GitLabIntegration, ScmIntegrations } from '@backstage/integration'; @@ -34,7 +35,6 @@ import { paginated, readGitlabConfigs, } from '../lib'; -import { LoggerService } from '@backstage/backend-plugin-api'; import * as path from 'path'; @@ -470,14 +470,8 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { return false; } - const customFallbackBranch = - this.config.fallbackBranch !== 'master' - ? this.config.fallbackBranch - : undefined; - const project_branch = this.config.branch ?? - customFallbackBranch ?? project.default_branch ?? this.config.fallbackBranch; From d0048635f75e3b857d774be6018231b0db28d927 Mon Sep 17 00:00:00 2001 From: ElaineDeMattosSilvaB Date: Tue, 21 May 2024 11:53:20 +0200 Subject: [PATCH 2/4] chore: improve tests Signed-off-by: ElaineDeMattosSilvaB --- .../src/__testUtils__/handlers.ts | 6 ++- .../src/__testUtils__/mocks.ts | 7 ++- .../GitlabDiscoveryEntityProvider.test.ts | 51 ++++++++++++++----- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts index f2dcac13cd..80aa0370d3 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts @@ -165,7 +165,11 @@ const httpProjectCatalogDynamic = all_projects_response.map(project => { `${apiBaseUrl}/projects/${path}/repository/files/catalog-info.yaml`, (req, res, ctx) => { const branch = req.url.searchParams.get('ref'); - if (branch === (project.default_branch || 'main' || 'develop')) { + if ( + branch === project.default_branch || + branch === 'main' || + branch === 'develop' + ) { return res(ctx.status(200)); } return res(ctx.status(404, 'Not Found')); diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts index 279da0dfc0..e3154eb1ce 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts @@ -162,7 +162,7 @@ export const config_github_host: MockObject = { }, }; -export const config_single_integration_branch: MockObject = { +export const config_single_integration: MockObject = { integrations: { gitlab: [ { @@ -178,7 +178,6 @@ export const config_single_integration_branch: MockObject = { 'test-id': { host: 'example.com', group: 'group1', - branch: 'main', skipForkedRepos: false, schedule: { frequency: 'PT30M', @@ -724,7 +723,7 @@ export const all_users_response: GitLabUser[] = [ avatar_url: 'https://secure.gravatar.com/', web_url: 'https://gitlab.example/luigi_mario', }, - // malfomed email address + // malformed email address { id: 5, username: 'MarioMario', @@ -1338,7 +1337,7 @@ export const push_modif_event: EventParams = { * Expected Backstage entities */ -// includes only projects that have a default branch (for when the branch and default branch were not set in the config) +// includes only projects that have a default branch (for when the branch and fallback branch were not set in the config) export const expected_location_entities_default_branch: MockObject[] = all_projects_response .filter(project => project.default_branch) diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts index 0c20732fa7..8c4618efdb 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts @@ -61,7 +61,7 @@ describe('GitlabDiscoveryEntityProvider - configuration', () => { }); it('should fail without schedule nor scheduler', () => { - const config = new ConfigReader(mock.config_single_integration_branch); + const config = new ConfigReader(mock.config_single_integration); expect(() => GitlabDiscoveryEntityProvider.fromConfig(config, { @@ -99,7 +99,7 @@ describe('GitlabDiscoveryEntityProvider - configuration', () => { it('should instantiate provider with single simple discovery config', () => { const schedule = new PersistingTaskRunner(); - const config = new ConfigReader(mock.config_single_integration_branch); + const config = new ConfigReader(mock.config_single_integration); const providers = GitlabDiscoveryEntityProvider.fromConfig(config, { logger, schedule, @@ -229,7 +229,36 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { }); }); - it('should only ingest projects from specific branch', async () => { + // branch and fallback branch are undefined in the config + it('should ingest catalog from project default branch only', async () => { + const config = new ConfigReader(mock.config_single_integration); + const schedule = new PersistingTaskRunner(); + const entityProviderConnection: EntityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + const provider = GitlabDiscoveryEntityProvider.fromConfig(config, { + logger, + schedule, + })[0]; + + await provider.connect(entityProviderConnection); + + await provider.refresh(logger); + + expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ + type: 'full', + entities: mock.expected_location_entities_default_branch.filter( + entity => + !entity.entity.metadata.annotations[ + 'backstage.io/managed-by-location' + ].includes('awesome'), + ), + }); + }); + + // branch was set in the config + it('should ingest catalog from specific branch only', async () => { const config = new ConfigReader( mock.config_single_integration_specific_branch, ); @@ -243,10 +272,6 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { schedule, })[0]; - const configured_branch = - mock.config_single_integration_branch.catalog.providers.gitlab['test-id'] - .branch; - await provider.connect(entityProviderConnection); await provider.refresh(logger); @@ -255,9 +280,6 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { type: 'full', entities: mock.expected_location_entities_specific_branch.filter( entity => - entity.entity.metadata.annotations[ - 'backstage.io/managed-by-location' - ].includes(configured_branch) && !entity.entity.metadata.annotations[ 'backstage.io/managed-by-location' ].includes('awesome'), @@ -265,7 +287,8 @@ describe('GitlabDiscoveryEntityProvider - refresh', () => { }); }); - it('should include projects from fallback branch', async () => { + // fallback branch was set in the config + it('should ingest catalog from default or fallback branch', async () => { const config = new ConfigReader(mock.config_fallbackBranch_branch); const schedule = new PersistingTaskRunner(); const entityProviderConnection: EntityProviderConnection = { @@ -389,7 +412,7 @@ describe('GitlabDiscoveryEntityProvider - events', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(0); }); it('should apply delta mutations on added files from push event', async () => { - const config = new ConfigReader(mock.config_single_integration_branch); + const config = new ConfigReader(mock.config_single_integration); const schedule = new PersistingTaskRunner(); const events = DefaultEventsService.create({ logger }); @@ -416,7 +439,7 @@ describe('GitlabDiscoveryEntityProvider - events', () => { }); it('should apply delta mutations on removed files from push event', async () => { - const config = new ConfigReader(mock.config_single_integration_branch); + const config = new ConfigReader(mock.config_single_integration); const schedule = new PersistingTaskRunner(); const events = DefaultEventsService.create({ logger }); const entityProviderConnection: EntityProviderConnection = { @@ -442,7 +465,7 @@ describe('GitlabDiscoveryEntityProvider - events', () => { }); it('should call refresh on added files from push event', async () => { - const config = new ConfigReader(mock.config_single_integration_branch); + const config = new ConfigReader(mock.config_single_integration); const schedule = new PersistingTaskRunner(); const events = DefaultEventsService.create({ logger }); const entityProviderConnection: EntityProviderConnection = { From f27116436b4216f04e51ce2ca21859b4630ca8b9 Mon Sep 17 00:00:00 2001 From: ElaineDeMattosSilvaB Date: Tue, 21 May 2024 12:02:04 +0200 Subject: [PATCH 3/4] chore: add changeset Signed-off-by: ElaineDeMattosSilvaB --- .changeset/perfect-bikes-invite.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/perfect-bikes-invite.md diff --git a/.changeset/perfect-bikes-invite.md b/.changeset/perfect-bikes-invite.md new file mode 100644 index 0000000000..82734b451d --- /dev/null +++ b/.changeset/perfect-bikes-invite.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-gitlab': patch +--- + +Fixed bug in the GitLab discovery where the fallback branch was taking precedence over the GitLab default branch. Relates to issue #24825. From 153bbd9c60c3b8b26d678edb6db3d99733cb9913 Mon Sep 17 00:00:00 2001 From: Elaine Mattos Date: Tue, 21 May 2024 13:28:33 +0200 Subject: [PATCH 4/4] Update .changeset/perfect-bikes-invite.md Co-authored-by: Vincenzo Scamporlino Signed-off-by: Elaine Mattos --- .changeset/perfect-bikes-invite.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/perfect-bikes-invite.md b/.changeset/perfect-bikes-invite.md index 82734b451d..96a4ad5659 100644 --- a/.changeset/perfect-bikes-invite.md +++ b/.changeset/perfect-bikes-invite.md @@ -2,4 +2,4 @@ '@backstage/plugin-catalog-backend-module-gitlab': patch --- -Fixed bug in the GitLab discovery where the fallback branch was taking precedence over the GitLab default branch. Relates to issue #24825. +Fixed an issue in `GitlabDiscoveryEntityProvider` where the fallback branch was taking precedence over the GitLab default branch.