From c2f20d5803856a6ddd903094dc689cfe51922002 Mon Sep 17 00:00:00 2001 From: ElaineDeMattosSilvaB Date: Mon, 25 Mar 2024 20:15:34 +0100 Subject: [PATCH] feat: incorporate review changes Signed-off-by: ElaineDeMattosSilvaB --- .../src/__testUtils__/handlers.ts | 16 ++ .../src/__testUtils__/mocks.ts | 94 +++++++++--- .../GitlabDiscoveryEntityProvider.test.ts | 2 +- .../GitlabDiscoveryEntityProvider.ts | 26 ++-- .../GitlabOrgDiscoveryEntityProvider.test.ts | 4 +- .../GitlabOrgDiscoveryEntityProvider.ts | 141 +++++++++--------- 6 files changed, 176 insertions(+), 107 deletions(-) diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts index 2f464dc1fe..00d4994e94 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/handlers.ts @@ -210,6 +210,22 @@ const graphqlHandlers = [ }), ); } + // group with no associated members + if (group === 'group3' && relations.includes('DIRECT')) { + return res( + ctx.data({ + group: { + groupMembers: { + nodes: [{}], + pageInfo: { + endCursor: 'end', + hasNextPage: false, + }, + }, + }, + }), + ); + } if (group === 'saas-multi-user-group') { return res( ctx.data({ diff --git a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts index 9e0f55d05d..5a078ab689 100644 --- a/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts +++ b/plugins/catalog-backend-module-gitlab/src/__testUtils__/mocks.ts @@ -139,7 +139,7 @@ export const config_saas_no_group: MockObject = { }, }; -export const config_non_gitlab_host: MockObject = { +export const config_github_host: MockObject = { integrations: { github: [ { @@ -910,9 +910,9 @@ export const group_destroy_event: EventParams = { event_name: 'group_destroy', created_at: '2024-02-02T10:53:09Z', updated_at: '2024-02-02T10:53:09Z', - name: 'group1', - path: 'group1', - full_path: 'group1', + name: 'group3', + path: 'group3', + full_path: 'group3', group_id: 123, }, }; @@ -951,10 +951,10 @@ export const group_create_event: EventParams = { event_name: 'group_create', created_at: '2024-02-02T10:53:09Z', updated_at: '2024-02-02T10:53:09Z', - name: 'group1', - path: 'group1', - full_path: 'group1', - group_id: 1, + name: 'group3', + path: 'group3', + full_path: 'group3', + group_id: 3, }, }; export const group_create_event_unmatched: EventParams = { @@ -1038,9 +1038,9 @@ export const user_remove_from_group_event: EventParams = { eventPayload: { created_at: '2024-02-02T10:53:09Z', updated_at: '2024-02-02T10:53:09Z', - group_name: 'group1', - group_path: 'my-groups/group1', - group_id: 1, + group_name: 'group3', + group_path: 'my-groups/group3', + group_id: 3, user_username: 'user1', user_name: 'John Doe', user_email: 'john.doe@company.com', @@ -1382,17 +1382,17 @@ export const expected_group_entity: MockObject[] = [ kind: 'Group', metadata: { annotations: { - 'backstage.io/managed-by-location': 'url:https://example.com/group1', + 'backstage.io/managed-by-location': 'url:https://example.com/group3', 'backstage.io/managed-by-origin-location': - 'url:https://example.com/group1', - 'example.com/team-path': 'group1', + 'url:https://example.com/group3', + 'example.com/team-path': 'group3', }, - name: 'group1', + name: 'group3', }, spec: { children: [], profile: { - displayName: 'group1', + displayName: 'group3', }, type: 'team', }, @@ -1489,17 +1489,44 @@ export const expected_transformed_group_entity: MockObject[] = [ kind: 'Group', metadata: { annotations: { - 'backstage.io/managed-by-location': 'url:https://example.com/group1', + 'backstage.io/managed-by-location': 'url:https://example.com/group3', 'backstage.io/managed-by-origin-location': - 'url:https://example.com/group1', - 'example.com/team-path': 'group1', + 'url:https://example.com/group3', + 'example.com/team-path': 'group3', }, - name: '1', + name: '3', }, spec: { children: [], profile: { - displayName: 'group1', + displayName: 'group3', + }, + type: 'team', + }, + }, + locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id', + }, +]; + +export const expected_removed_user_entity: MockObject[] = [ + { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Group', + metadata: { + annotations: { + 'backstage.io/managed-by-location': 'url:https://example.com/group3', + 'backstage.io/managed-by-origin-location': + 'url:https://example.com/group3', + 'example.com/team-path': 'group3', + }, + name: '3', + }, + spec: { + children: [], + members: [], + profile: { + displayName: 'group3', }, type: 'team', }, @@ -1661,8 +1688,29 @@ export const expected_full_org_scan_entities: MockObject[] = [ }, locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id', }, - - expected_group_entity[0], + { + entity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Group', + metadata: { + annotations: { + 'backstage.io/managed-by-location': 'url:https://example.com/group1', + 'backstage.io/managed-by-origin-location': + 'url:https://example.com/group1', + 'example.com/team-path': 'group1', + }, + name: 'group1', + }, + spec: { + children: [], + profile: { + displayName: 'group1', + }, + type: 'team', + }, + }, + locationKey: 'GitlabOrgDiscoveryEntityProvider:test-id', + }, ]; export const expected_full_org_scan_entities_saas: MockObject[] = [ 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 bd725ee0ce..7167fc1c2d 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.test.ts @@ -87,7 +87,7 @@ describe('GitlabDiscoveryEntityProvider - configuration', () => { }); it('should throw error when no matching GitLab integration config found', () => { const schedule = new PersistingTaskRunner(); - const config = new ConfigReader(mock.config_non_gitlab_host); + const config = new ConfigReader(mock.config_github_host); expect(() => { GitlabDiscoveryEntityProvider.fromConfig(config, { diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts index 653b12060e..73ff5f3a01 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabDiscoveryEntityProvider.ts @@ -61,6 +61,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { private readonly scheduleFn: () => Promise; private connection?: EntityProviderConnection; private readonly events?: EventsService; + private readonly gitLabClient: GitLabClient; static fromConfig( config: Config, @@ -129,6 +130,10 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { }); this.scheduleFn = this.createScheduleFn(options.taskRunner); this.events = options.events; + this.gitLabClient = new GitLabClient({ + config: this.integration.config, + logger: this.logger, + }); } getProviderName(): string { @@ -196,13 +201,9 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { `Gitlab discovery connection not initialized for ${this.getProviderName()}`, ); } - const client = new GitLabClient({ - config: this.integration.config, - logger: logger, - }); const projects = paginated( - options => client.listProjects(options), + options => this.gitLabClient.listProjects(options), { archived: false, group: this.config.group, @@ -217,13 +218,17 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { }; for await (const project of projects) { - if (await this.shouldProcessProject(project, client)) { + if (await this.shouldProcessProject(project, this.gitLabClient)) { res.scanned++; res.matches.push(project); } } const locations = res.matches.map(p => this.createLocationSpec(p)); + + logger.info( + `Processed ${locations.length} from scanned ${res.scanned} projects.`, + ); await this.connection.applyMutation({ type: 'full', entities: locations.map(location => ({ @@ -261,12 +266,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { `Received push event for ${event.project.path_with_namespace}`, ); - const client = new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); - - const project = await client.getProjectById(event.project_id); + const project = await this.gitLabClient.getProjectById(event.project_id); if (!project) { this.logger.debug( @@ -276,7 +276,7 @@ export class GitlabDiscoveryEntityProvider implements EntityProvider { return; } - if (!(await this.shouldProcessProject(project, client))) { + if (!(await this.shouldProcessProject(project, this.gitLabClient))) { this.logger.debug(`Skipping event ${event.project.path_with_namespace}`); return; } diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts index a65b0a1bc2..b5a049538d 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.test.ts @@ -63,7 +63,7 @@ describe('GitlabOrgDiscoveryEntityProvider - configuration', () => { it('should throw error when no matching GitLab integration config found', () => { const schedule = new PersistingTaskRunner(); - const config = new ConfigReader(mock.config_non_gitlab_host); + const config = new ConfigReader(mock.config_github_host); expect(() => { GitlabOrgDiscoveryEntityProvider.fromConfig(config, { @@ -625,7 +625,7 @@ describe('GitlabOrgDiscoveryEntityProvider with events support', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ type: 'delta', - added: mock.expected_transformed_group_entity, + added: mock.expected_removed_user_entity, removed: [], }); }); diff --git a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts index 30105dcc1f..0135c7b4a9 100644 --- a/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts +++ b/plugins/catalog-backend-module-gitlab/src/providers/GitlabOrgDiscoveryEntityProvider.ts @@ -107,6 +107,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { private userTransformer: UserTransformer; private groupEntitiesTransformer: GroupEntitiesTransformer; private groupNameTransformer: GroupNameTransformer; + private readonly gitLabClient: GitLabClient; static fromConfig( config: Config, @@ -192,6 +193,11 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { options.groupEntitiesTransformer ?? defaultGroupEntitiesTransformer; this.groupNameTransformer = options.groupNameTransformer ?? defaultGroupNameTransformer; + + this.gitLabClient = new GitLabClient({ + config: this.integration.config, + logger: this.logger, + }); } getProviderName(): string { @@ -346,31 +352,33 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { ); } - const client = new GitLabClient({ - config: this.integration.config, - logger: logger, - }); - let groups; let users; - if (client.isSelfManaged()) { - groups = paginated(options => client.listGroups(options), { - page: 1, - per_page: 100, - all_available: true, - }); + if (this.gitLabClient.isSelfManaged()) { + groups = paginated( + options => this.gitLabClient.listGroups(options), + { + page: 1, + per_page: 100, + all_available: true, + }, + ); - users = paginated(options => client.listUsers(options), { - page: 1, - per_page: 100, - active: true, - }); + users = paginated( + options => this.gitLabClient.listUsers(options), + { + page: 1, + per_page: 100, + active: true, + }, + ); } else { - groups = (await client.listDescendantGroups(this.config.group)).items; + groups = (await this.gitLabClient.listDescendantGroups(this.config.group)) + .items; const rootGroup = this.config.group.split('/')[0]; users = paginated( - options => client.listSaaSUsers(rootGroup, options), + options => this.gitLabClient.listSaaSUsers(rootGroup, options), { page: 1, per_page: 100, @@ -418,7 +426,10 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { const relations = this.config.allowInherited ? ['DIRECT', 'INHERITED'] : ['DIRECT']; - groupUsers = await client.getGroupMembers(group.full_path, relations); + groupUsers = await this.gitLabClient.getGroupMembers( + group.full_path, + relations, + ); } catch (e) { logger.error( `Failed fetching users for group '${group.full_path}': ${e}`, @@ -488,12 +499,6 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { ); } - const getClient = () => - new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); - let group: GitLabGroup | undefined; if (event.event_name === 'group_destroy') { group = { @@ -504,8 +509,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { parent_id: 0, }; } else { - const client = getClient(); - group = await client.getGroupById(event.group_id); + group = await this.gitLabClient.getGroupById(event.group_id); } if (!this.shouldProcessGroup(group)) { @@ -522,12 +526,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { // we need to fetch the parent group's object because its representation might be changed by the groupTransformer if (group.parent_id) { - const client = new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); - - const parentGroup = await client.getGroupById(group.parent_id); + const parentGroup = await this.gitLabClient.getGroupById(group.parent_id); groupEntity[0].spec.parent = this.groupNameTransformer({ group: parentGroup, @@ -575,12 +574,7 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { groupNameTransformer: this.groupNameTransformer, }); - const client = new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); - - const groupToAdd = await client.getGroupById(event.group_id); + const groupToAdd = await this.gitLabClient.getGroupById(event.group_id); if (!this.shouldProcessGroup(groupToAdd)) { this.logger.debug(`Skipped group ${groupToAdd.full_path}.`); @@ -594,7 +588,9 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { }); if (groupToAdd.parent_id) { - const parentGroup = await client.getGroupById(groupToAdd.parent_id); + const parentGroup = await this.gitLabClient.getGroupById( + groupToAdd.parent_id, + ); groupEntityToAdd[0].spec.parent = this.groupNameTransformer({ group: parentGroup, @@ -602,8 +598,8 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { }); } - const { added } = createDeltaOperation([...groupEntityToAdd]); - const { removed } = createDeltaOperation([...groupEntityToRemove]); + const { added } = createDeltaOperation(groupEntityToAdd); + const { removed } = createDeltaOperation(groupEntityToRemove); this.logger.debug(`Applying mutation for group ${groupToAdd.full_path}.`); await this.connection.applyMutation({ @@ -626,7 +622,9 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { ); } - let user: GitLabUser; + let user: GitLabUser | undefined = undefined; + + // if user destroy event is received, retrieve user data from the event itself if (event.event_name === 'user_destroy') { user = { id: event.user_id, @@ -637,13 +635,18 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { web_url: '', avatar_url: '', }; - } else { - const client = new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); + } - user = await client.getUserById(event.user_id); + // if user create event received fetch data from gitlab + if (event.event_name === 'user_create') { + user = await this.gitLabClient.getUserById(event.user_id); + } + + if (!user) { + this.logger.debug( + `Couldn't retrieve user data. Skipped ${event.event_name} event processing for user ${event.username}`, + ); + return; } if (!this.shouldProcessUser(user)) { @@ -681,16 +684,19 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { ); } - const client = new GitLabClient({ - config: this.integration.config, - logger: this.logger, - }); - - const groupToRebuild: GitLabGroup = await client.getGroupById( + // fetch group data from GitLab + const groupToRebuild: GitLabGroup = await this.gitLabClient.getGroupById( event.group_id, ); - // If the group is outside the scope there is no point creating anything related to it. + if (!groupToRebuild) { + this.logger.debug( + `Couldn't retrieve group data. Skipped ${event.event_name} event processing.`, + ); + return; + } + + // if the group is outside the scope there is no point creating anything related to it. if (!this.shouldProcessGroup(groupToRebuild)) { this.logger.debug(`Skipped group ${groupToRebuild.full_path}.`); return; @@ -700,16 +706,12 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { ? ['DIRECT', 'INHERITED'] : ['DIRECT']; - const groupMembers = await client.getGroupMembers( + // fetch group members from GitLab + const groupMembers = await this.gitLabClient.getGroupMembers( groupToRebuild.full_path, relations, ); - - // new members of the group - const usersToBeAdded = - event.event_name === 'user_remove_from_group' - ? groupMembers.items.filter(e => e.username !== event.user_username) - : groupMembers.items; + const usersToBeAdded = groupMembers.items; const groupEntityToModify = await this.groupEntitiesTransformer({ groups: [groupToRebuild], @@ -719,19 +721,22 @@ export class GitlabOrgDiscoveryEntityProvider implements EntityProvider { // we need to fetch the parent group's object because its representation might be changed by the groupTransformer if (groupToRebuild.parent_id) { - const parentGroup = await client.getGroupById(groupToRebuild.parent_id); + const parentGroup = await this.gitLabClient.getGroupById( + groupToRebuild.parent_id, + ); + // update parent of the group entity groupEntityToModify[0].spec.parent = this.groupNameTransformer({ group: parentGroup, providerConfig: this.config, }); } - if (usersToBeAdded.length !== 0) { - groupEntityToModify[0].spec.members = usersToBeAdded.map(e => e.username); - } + // update members of the group entity + groupEntityToModify[0].spec.members = + usersToBeAdded.length !== 0 ? usersToBeAdded.map(e => e.username) : []; - const { added, removed } = createDeltaOperation([...groupEntityToModify]); + const { added, removed } = createDeltaOperation(groupEntityToModify); this.logger.debug( `Applying mutation for group ${groupToRebuild.full_path}.`,