diff --git a/.changeset/ten-vans-agree.md b/.changeset/ten-vans-agree.md new file mode 100644 index 0000000000..69e1d85e02 --- /dev/null +++ b/.changeset/ten-vans-agree.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-github': patch +--- + +Improved efficiency of `GithubOrgEntityProvider` membership event handling and edit team. The provider now fetches only the specific user's teams instead of all organization users when processing membership events, and uses `addEntitiesOperation` instead of `replaceEntitiesOperation` to avoid unnecessary entity deletions. diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts index 9be5a3e161..86b0090263 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts @@ -840,6 +840,9 @@ describe('GithubOrgEntityProvider', () => { name: { from: 'mygroup with spaces', }, + description: { + from: 'old description', + }, }, team: { node_id: 'xpto', @@ -956,21 +959,21 @@ describe('GithubOrgEntityProvider', () => { 'url:https://github.com/orgs/backstage/teams/mygroup-with-spaces', 'backstage.io/managed-by-origin-location': 'url:https://github.com/orgs/backstage/teams/mygroup-with-spaces', + 'github.com/team-slug': 'backstage/mygroup-with-spaces', }, name: 'mygroup-with-spaces', - description: 'description-from-the-new-team', + description: 'old description', }, apiVersion: 'backstage.io/v1alpha1', kind: 'Group', spec: { type: 'team', profile: { - displayName: 'Team', - picture: 'http://example.com/team.jpeg', + displayName: 'mygroup with spaces', }, children: [], - parent: 'parent', - members: ['a', 'githubuser'], + parent: 'father-team', + members: [], }, }, }, @@ -1012,6 +1015,7 @@ describe('GithubOrgEntityProvider', () => { const mockClient = jest.fn(); mockClient + // getOrganizationTeam .mockResolvedValueOnce({ organization: { team: { @@ -1032,31 +1036,7 @@ describe('GithubOrgEntityProvider', () => { }, }, }) - .mockResolvedValueOnce({ - organization: { - membersWithRole: { - pageInfo: { hasNextPage: false }, - nodes: [ - { - login: 'a', - id: 'f', - name: 'b', - bio: 'c', - email: 'd', - avatarUrl: 'e', - }, - { - login: 'githubuser', - id: 'githubuserId', - name: 'githubuser', - bio: 'githubuser', - email: 'd', - avatarUrl: 'e', - }, - ], - }, - }, - }) + // getOrganizationTeamsForUser .mockResolvedValueOnce({ organization: { teams: { @@ -1070,12 +1050,6 @@ describe('GithubOrgEntityProvider', () => { avatarUrl: 'http://example.com/team.jpeg', parentTeam: { slug: 'parent', - combinedSlug: '', - members: { pageInfo: { hasNextPage: false }, nodes: [] }, - }, - members: { - pageInfo: { hasNextPage: false }, - nodes: [{ login: 'a' }, { login: 'githubuser' }], }, }, ], @@ -1101,6 +1075,10 @@ describe('GithubOrgEntityProvider', () => { }, member: { login: 'githubuser', + avatar_url: 'e', + email: 'd', + name: 'githubuser', + node_id: 'githubuserId', }, organization: { login: 'test-org', @@ -1114,33 +1092,6 @@ describe('GithubOrgEntityProvider', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ added: [ - { - locationKey: 'github-org-provider:my-id', - entity: { - metadata: { - annotations: { - 'backstage.io/managed-by-location': - 'url:https://github.com/githubuser', - 'backstage.io/managed-by-origin-location': - 'url:https://github.com/githubuser', - 'github.com/user-login': 'githubuser', - 'github.com/user-id': 'githubuserId', - }, - name: 'githubuser', - description: 'githubuser', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'User', - spec: { - profile: { - displayName: 'githubuser', - email: 'd', - picture: 'e', - }, - memberOf: ['team'], - }, - }, - }, { locationKey: 'github-org-provider:my-id', entity: { @@ -1169,8 +1120,6 @@ describe('GithubOrgEntityProvider', () => { }, }, }, - ], - removed: [ { locationKey: 'github-org-provider:my-id', entity: { @@ -1184,7 +1133,6 @@ describe('GithubOrgEntityProvider', () => { 'github.com/user-id': 'githubuserId', }, name: 'githubuser', - description: 'githubuser', }, apiVersion: 'backstage.io/v1alpha1', kind: 'User', @@ -1198,35 +1146,8 @@ describe('GithubOrgEntityProvider', () => { }, }, }, - { - locationKey: 'github-org-provider:my-id', - entity: { - metadata: { - annotations: { - 'backstage.io/managed-by-location': - 'url:https://github.com/orgs/backstage/teams/team', - 'backstage.io/managed-by-origin-location': - 'url:https://github.com/orgs/backstage/teams/team', - 'github.com/team-slug': 'blah/team', - }, - name: 'team', - description: 'The one and only team', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'Group', - spec: { - type: 'team', - profile: { - displayName: 'Team', - picture: 'http://example.com/team.jpeg', - }, - children: [], - parent: 'parent', - members: ['a', 'githubuser'], - }, - }, - }, ], + removed: [], type: 'delta', }); }); @@ -1264,6 +1185,7 @@ describe('GithubOrgEntityProvider', () => { const mockClient = jest.fn(); mockClient + // getOrganizationTeam .mockResolvedValueOnce({ organization: { team: { @@ -1284,23 +1206,7 @@ describe('GithubOrgEntityProvider', () => { }, }, }) - .mockResolvedValueOnce({ - organization: { - membersWithRole: { - pageInfo: { hasNextPage: false }, - nodes: [ - { - login: 'githubuser', - id: 'githubuserId', - name: 'githubuser', - bio: 'githubuser', - email: 'd', - avatarUrl: 'e', - }, - ], - }, - }, - }) + // getOrganizationTeamsForUser - returns empty teams since user was removed .mockResolvedValueOnce({ organization: { teams: { @@ -1328,6 +1234,10 @@ describe('GithubOrgEntityProvider', () => { }, member: { login: 'githubuser', + avatar_url: 'e', + email: 'd', + name: 'githubuser', + node_id: 'githubuserId', }, organization: { login: 'test-org', @@ -1341,33 +1251,6 @@ describe('GithubOrgEntityProvider', () => { expect(entityProviderConnection.applyMutation).toHaveBeenCalledTimes(1); expect(entityProviderConnection.applyMutation).toHaveBeenCalledWith({ added: [ - { - locationKey: 'github-org-provider:my-id', - entity: { - metadata: { - annotations: { - 'backstage.io/managed-by-location': - 'url:https://github.com/githubuser', - 'backstage.io/managed-by-origin-location': - 'url:https://github.com/githubuser', - 'github.com/user-login': 'githubuser', - 'github.com/user-id': 'githubuserId', - }, - name: 'githubuser', - description: 'githubuser', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'User', - spec: { - profile: { - displayName: 'githubuser', - email: 'd', - picture: 'e', - }, - memberOf: [], - }, - }, - }, { locationKey: 'github-org-provider:my-id', entity: { @@ -1396,8 +1279,6 @@ describe('GithubOrgEntityProvider', () => { }, }, }, - ], - removed: [ { locationKey: 'github-org-provider:my-id', entity: { @@ -1411,7 +1292,6 @@ describe('GithubOrgEntityProvider', () => { 'github.com/user-id': 'githubuserId', }, name: 'githubuser', - description: 'githubuser', }, apiVersion: 'backstage.io/v1alpha1', kind: 'User', @@ -1425,35 +1305,8 @@ describe('GithubOrgEntityProvider', () => { }, }, }, - { - locationKey: 'github-org-provider:my-id', - entity: { - metadata: { - annotations: { - 'backstage.io/managed-by-location': - 'url:https://github.com/orgs/backstage/teams/team', - 'backstage.io/managed-by-origin-location': - 'url:https://github.com/orgs/backstage/teams/team', - 'github.com/team-slug': 'blah/team', - }, - name: 'team', - description: 'The one and only team', - }, - apiVersion: 'backstage.io/v1alpha1', - kind: 'Group', - spec: { - type: 'team', - profile: { - displayName: 'Team', - picture: 'http://example.com/team.jpeg', - }, - children: [], - parent: 'parent', - members: ['a'], - }, - }, - }, ], + removed: [], type: 'delta', }); }); diff --git a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts index bb4b268e62..d13520f112 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.ts @@ -18,7 +18,7 @@ import { LoggerService, SchedulerServiceTaskRunner, } from '@backstage/backend-plugin-api'; -import { Entity, isGroupEntity } from '@backstage/catalog-model'; +import { Entity, isGroupEntity, isUserEntity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { DefaultGithubCredentialsProvider, @@ -52,18 +52,22 @@ import { createAddEntitiesOperation, createGraphqlClient, createRemoveEntitiesOperation, - createReplaceEntitiesOperation, DEFAULT_PAGE_SIZES, DeferredEntitiesBuilder, getOrganizationTeam, getOrganizationTeams, + getOrganizationTeamsForUser, getOrganizationTeamsFromUsers, getOrganizationUsers, GithubPageSizes, GithubTeam, } from '../lib/github'; import { areGroupEntities, areUserEntities } from '../lib/guards'; -import { assignGroupsToUsers, buildOrgHierarchy } from '../lib/org'; +import { + assignGroupsToUser, + assignGroupsToUsers, + buildOrgHierarchy, +} from '../lib/org'; import { parseGithubOrgUrl } from '../lib/util'; import { withLocations } from '../lib/withLocations'; @@ -311,11 +315,6 @@ export class GithubOrgEntityProvider implements EntityProvider { this.options.gitHubConfig.host, ); - const replaceEntitiesOperation = createReplaceEntitiesOperation( - this.options.id, - this.options.gitHubConfig.host, - ); - // handle change users in the org // https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#organization if (params.topic.includes('organization')) { @@ -346,7 +345,8 @@ export class GithubOrgEntityProvider implements EntityProvider { } else if (teamEvent.action === 'edited') { await this.onTeamEditedInOrganization( teamEvent, - replaceEntitiesOperation, + addEntitiesOperation, + removeEntitiesOperation, ); } } @@ -355,9 +355,9 @@ export class GithubOrgEntityProvider implements EntityProvider { // https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#membership if (params.topic.includes('membership')) { const membershipEvent = params.eventPayload as MembershipEvent; - this.onMembershipChangedInOrganization( + await this.onMembershipChangedInOrganization( membershipEvent, - replaceEntitiesOperation, + addEntitiesOperation, ); } @@ -366,7 +366,8 @@ export class GithubOrgEntityProvider implements EntityProvider { private async onTeamEditedInOrganization( event: TeamEditedEvent, - createDeltaOperation: DeferredEntitiesBuilder, + addEntitiesOperation: DeferredEntitiesBuilder, + removeEntitiesOperation: DeferredEntitiesBuilder, ) { if (!this.connection) { throw new Error('Not initialized'); @@ -424,25 +425,33 @@ export class GithubOrgEntityProvider implements EntityProvider { } } - const oldName = event.changes.name?.from || event.team.name; + const teamTransformer = + this.options.teamTransformer || defaultOrganizationTeamTransformer; + + const oldName = event.changes.name?.from || ''; const oldSlug = oldName.toLowerCase().replaceAll(/\s/gi, '-'); - - const oldDescription = - event.changes.description?.from || event.team.description; - const oldDescriptionSlug = oldDescription - ?.toLowerCase() - .replaceAll(/\s/gi, '-'); - - const { removed } = createDeltaOperation(org, [ + const oldGroup = (await teamTransformer( { - ...team, - metadata: { - name: oldSlug, - description: oldDescriptionSlug, - }, + name: event.changes.name?.from, + slug: oldSlug, + combinedSlug: `${org}/${oldSlug}`, + description: event.changes.description?.from, + parentTeam: event.team?.parent?.slug + ? ({ slug: event.team.parent.slug } as GithubTeam) + : undefined, + // entity will be removed + members: [], }, - ]); - const { added } = createDeltaOperation(org, [...usersToRebuild, ...teams]); + { + org, + client, + query: '', + }, + )) as Entity; + + // Remove the old group entity in case the entity ref is now different + const { removed } = removeEntitiesOperation(org, [oldGroup]); + const { added } = addEntitiesOperation(org, [...usersToRebuild, team]); await this.connection.applyMutation({ type: 'delta', removed, @@ -467,11 +476,9 @@ export class GithubOrgEntityProvider implements EntityProvider { } const teamSlug = event.team.slug; - const userLogin = event.member.login; - const { headers, type: tokenType } = - await this.credentialsProvider.getCredentials({ - url: this.options.orgUrl, - }); + const { headers } = await this.credentialsProvider.getCredentials({ + url: this.options.orgUrl, + }); const client = graphql.defaults({ baseUrl: this.options.gitHubConfig.apiBaseUrl, headers, @@ -484,43 +491,48 @@ export class GithubOrgEntityProvider implements EntityProvider { org, teamSlug, this.options.teamTransformer, - ); - - const { users } = await getOrganizationUsers( - client, - org, - tokenType, - this.options.userTransformer, - pageSizes, - this.options.excludeSuspendedUsers, - ); - - const usersToRebuild = users.filter(u => u.metadata.name === userLogin); - - const { teams } = await getOrganizationTeamsFromUsers( - client, - org, - [userLogin], - this.options.teamTransformer, pageSizes, ); - // we include group because the removed event need to update the old group too - if (!teams.some(t => t.metadata.name === team.metadata.name)) { - teams.push(team); - } + const userTransformer = + this.options.userTransformer || defaultUserTransformer; + const { name, avatar_url: avatarUrl, email, login, node_id } = event.member; + const user = await userTransformer( + { + name, + avatarUrl, + login, + email: email ?? undefined, + id: node_id, + }, + { + org, + client, + query: '', + }, + ); - if (areGroupEntities(teams)) { - buildOrgHierarchy(teams); - if (areUserEntities(usersToRebuild)) { - assignGroupsToUsers(usersToRebuild, teams); + const mutationEntities: Entity[] = [team]; + + if (user && isUserEntity(user)) { + const teamTransformer = + this.options.teamTransformer || defaultOrganizationTeamTransformer; + const { teams } = await getOrganizationTeamsForUser( + client, + org, + login, + teamTransformer, + pageSizes, + ); + + if (areGroupEntities(teams)) { + assignGroupsToUser(user, teams); } + + mutationEntities.push(user); } - const { added, removed } = createDeltaOperation(org, [ - ...usersToRebuild, - ...teams, - ]); + const { added, removed } = createDeltaOperation(org, mutationEntities); await this.connection.applyMutation({ type: 'delta', removed,