diff --git a/.changeset/thirty-dolls-admire.md b/.changeset/thirty-dolls-admire.md new file mode 100644 index 0000000000..1011508a98 --- /dev/null +++ b/.changeset/thirty-dolls-admire.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-github': minor +--- + +Prevent Entity Providers from eliminating Users and Groups from the DB when the synchronisation fails diff --git a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts index 504e5772dd..78c12d92cf 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.test.ts @@ -17,10 +17,7 @@ import { getVoidLogger } from '@backstage/backend-common'; import { GroupEntity, UserEntity } from '@backstage/catalog-model'; import { ConfigReader } from '@backstage/config'; -import { - GithubCredentialsProvider, - GithubIntegrationConfig, -} from '@backstage/integration'; +import { GithubCredentialsProvider } from '@backstage/integration'; import { EntityProviderConnection } from '@backstage/plugin-catalog-node'; import { EventSubscriber } from '@backstage/plugin-events-node'; import { graphql } from '@octokit/graphql'; @@ -28,6 +25,7 @@ import { GithubMultiOrgEntityProvider, withLocations, } from './GithubMultiOrgEntityProvider'; +import { Logger } from 'winston'; jest.mock('@octokit/graphql'); @@ -43,11 +41,50 @@ jest.mock('@backstage/integration', () => ({ describe('GithubMultiOrgEntityProvider', () => { describe('read', () => { + let mockClient: jest.Mock; + let entityProviderConnection: EntityProviderConnection; + let logger: Logger; + let gitHubConfig: { host: string }; + let mockGetCredentials: jest.Mock; + let entityProvider: GithubMultiOrgEntityProvider; + + beforeEach(() => { + mockClient = jest.fn(); + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + + entityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + logger = getVoidLogger(); + + gitHubConfig = { host: 'github.com' }; + + mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + }); + + const githubCredentialsProvider: GithubCredentialsProvider = { + getCredentials: mockGetCredentials, + }; + + entityProvider = new GithubMultiOrgEntityProvider({ + id: 'my-id', + gitHubConfig, + githubCredentialsProvider, + githubUrl: 'https://github.com', + logger, + orgs: ['orgA', 'orgB'], // only include for tests that require it + }); + + entityProvider.connect(entityProviderConnection); + }); + afterEach(() => jest.resetAllMocks()); it('should read specified orgs', async () => { - const mockClient = jest.fn(); - mockClient .mockResolvedValueOnce({ organization: { @@ -155,36 +192,6 @@ describe('GithubMultiOrgEntityProvider', () => { (graphql.defaults as jest.Mock).mockReturnValue(mockClient); - const entityProviderConnection: EntityProviderConnection = { - applyMutation: jest.fn(), - refresh: jest.fn(), - }; - - const logger = getVoidLogger(); - const gitHubConfig: GithubIntegrationConfig = { - host: 'github.com', - }; - - const mockGetCredentials = jest.fn().mockReturnValue({ - headers: { token: 'blah' }, - type: 'app', - }); - - const githubCredentialsProvider: GithubCredentialsProvider = { - getCredentials: mockGetCredentials, - }; - - const entityProvider = new GithubMultiOrgEntityProvider({ - id: 'my-id', - gitHubConfig, - githubCredentialsProvider, - githubUrl: 'https://github.com', - logger, - orgs: ['orgA', 'orgB'], - }); - - entityProvider.connect(entityProviderConnection); - await entityProvider.read(); expect(mockGetCredentials).toHaveBeenCalledWith({ @@ -353,8 +360,6 @@ describe('GithubMultiOrgEntityProvider', () => { }, ]); - const mockClient = jest.fn(); - mockClient .mockResolvedValueOnce({ organization: { @@ -462,26 +467,11 @@ describe('GithubMultiOrgEntityProvider', () => { (graphql.defaults as jest.Mock).mockReturnValue(mockClient); - const entityProviderConnection: EntityProviderConnection = { - applyMutation: jest.fn(), - refresh: jest.fn(), - }; - - const logger = getVoidLogger(); - const gitHubConfig: GithubIntegrationConfig = { - host: 'github.com', - }; - - const mockGetCredentials = jest.fn().mockReturnValue({ - headers: { token: 'blah' }, - type: 'app', - }); - const githubCredentialsProvider: GithubCredentialsProvider = { getCredentials: mockGetCredentials, }; - const entityProvider = new GithubMultiOrgEntityProvider({ + entityProvider = new GithubMultiOrgEntityProvider({ id: 'my-id', gitHubConfig, githubCredentialsProvider, @@ -642,6 +632,16 @@ describe('GithubMultiOrgEntityProvider', () => { type: 'full', }); }); + + it('should not call applyMutation if an error is thrown', async () => { + mockClient.mockImplementationOnce(() => { + throw new Error('Network Error'); + }); + + await expect(entityProvider.read()).rejects.toThrow('Network Error'); + + expect(entityProviderConnection.applyMutation).not.toHaveBeenCalled(); + }); }); describe('withLocations', () => { diff --git a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts index cabb9e21ce..8387a91e3a 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubMultiOrgEntityProvider.ts @@ -248,53 +248,49 @@ export class GithubMultiOrgEntityProvider implements EntityProvider { : await this.getAllOrgs(this.options.gitHubConfig); for (const org of orgsToProcess) { - try { - const { headers, type: tokenType } = - await this.options.githubCredentialsProvider.getCredentials({ - url: `${this.options.githubUrl}/${org}`, - }); - const client = graphql.defaults({ - baseUrl: this.options.gitHubConfig.apiBaseUrl, - headers, + const { headers, type: tokenType } = + await this.options.githubCredentialsProvider.getCredentials({ + url: `${this.options.githubUrl}/${org}`, }); + const client = graphql.defaults({ + baseUrl: this.options.gitHubConfig.apiBaseUrl, + headers, + }); - logger.info(`Reading GitHub users and teams for org: ${org}`); + logger.info(`Reading GitHub users and teams for org: ${org}`); - const { users } = await getOrganizationUsers( - client, - org, - tokenType, - this.options.userTransformer, - ); + const { users } = await getOrganizationUsers( + client, + org, + tokenType, + this.options.userTransformer, + ); - const { teams } = await getOrganizationTeams( - client, - org, - this.defaultMultiOrgTeamTransformer.bind(this), - ); + const { teams } = await getOrganizationTeams( + client, + org, + this.defaultMultiOrgTeamTransformer.bind(this), + ); - // Grab current users from `allUsersMap` if they already exist in our - // pending users so we can append to their group membership relations - const pendingUsers = users.map(u => { - const userRef = stringifyEntityRef(u); - if (!allUsersMap.has(userRef)) { - allUsersMap.set(userRef, u); - } - - return allUsersMap.get(userRef); - }); - - if (areGroupEntities(teams)) { - buildOrgHierarchy(teams); - if (areUserEntities(pendingUsers)) { - assignGroupsToUsers(pendingUsers, teams); - } + // Grab current users from `allUsersMap` if they already exist in our + // pending users so we can append to their group membership relations + const pendingUsers = users.map(u => { + const userRef = stringifyEntityRef(u); + if (!allUsersMap.has(userRef)) { + allUsersMap.set(userRef, u); } - allTeams.push(...teams); - } catch (e) { - logger.error(`Failed to read GitHub org data for ${org}: ${e}`); + return allUsersMap.get(userRef); + }); + + if (areGroupEntities(teams)) { + buildOrgHierarchy(teams); + if (areUserEntities(pendingUsers)) { + assignGroupsToUsers(pendingUsers, teams); + } } + + allTeams.push(...teams); } const allUsers = Array.from(allUsersMap.values()); 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 c054f26522..486ff47f1c 100644 --- a/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts +++ b/plugins/catalog-backend-module-github/src/providers/GithubOrgEntityProvider.test.ts @@ -30,13 +30,51 @@ jest.mock('@octokit/graphql'); describe('GithubOrgEntityProvider', () => { describe('read', () => { + let mockClient; + let entityProviderConnection: EntityProviderConnection; + let entityProvider: GithubOrgEntityProvider; + + const setupMocks = (response: ((...args: any) => any) | undefined) => { + mockClient = jest.fn().mockImplementation(response); + (graphql.defaults as jest.Mock).mockReturnValue(mockClient); + }; + + beforeEach(() => { + entityProviderConnection = { + applyMutation: jest.fn(), + refresh: jest.fn(), + }; + + const logger = getVoidLogger(); + const gitHubConfig = { + host: 'https://github.com', + }; + + const mockGetCredentials = jest.fn().mockReturnValue({ + headers: { token: 'blah' }, + type: 'app', + }); + + const githubCredentialsProvider = { + getCredentials: mockGetCredentials, + }; + + entityProvider = new GithubOrgEntityProvider({ + id: 'my-id', + githubCredentialsProvider, + orgUrl: 'https://github.com/backstage', + gitHubConfig, + logger, + }); + + entityProvider.connect(entityProviderConnection); + }); + afterEach(() => jest.resetAllMocks()); it('should read org data and apply mutation', async () => { - const mockClient = jest.fn(); - - mockClient - .mockResolvedValueOnce({ + setupMocks(() => + Promise.resolve({ organization: { membersWithRole: { pageInfo: { hasNextPage: false }, @@ -50,10 +88,6 @@ describe('GithubOrgEntityProvider', () => { }, ], }, - }, - }) - .mockResolvedValueOnce({ - organization: { teams: { pageInfo: { hasNextPage: false }, nodes: [ @@ -76,38 +110,8 @@ describe('GithubOrgEntityProvider', () => { ], }, }, - }); - - (graphql.defaults as jest.Mock).mockReturnValue(mockClient); - - const entityProviderConnection: EntityProviderConnection = { - applyMutation: jest.fn(), - refresh: jest.fn(), - }; - - const logger = getVoidLogger(); - const gitHubConfig: GithubIntegrationConfig = { - host: 'https://github.com', - }; - - const mockGetCredentials = jest.fn().mockReturnValue({ - headers: { token: 'blah' }, - type: 'app', - }); - - const githubCredentialsProvider: GithubCredentialsProvider = { - getCredentials: mockGetCredentials, - }; - - const entityProvider = new GithubOrgEntityProvider({ - id: 'my-id', - githubCredentialsProvider, - orgUrl: 'https://github.com/backstage', - gitHubConfig, - logger, - }); - - entityProvider.connect(entityProviderConnection); + }), + ); await entityProvider.read(); @@ -171,6 +175,14 @@ describe('GithubOrgEntityProvider', () => { type: 'full', }); }); + + it('should not apply mutation if a request fails', async () => { + setupMocks(() => Promise.reject(new Error('Network error'))); + + await expect(entityProvider.read()).rejects.toThrow('Network error'); + + expect(entityProviderConnection.applyMutation).not.toHaveBeenCalled(); + }); }); describe('withLocations', () => {