From 7bd0a8ab3c745cc0231dcc3d23ec54ff0a02cf99 Mon Sep 17 00:00:00 2001 From: Jonathan Nagayoshi Date: Mon, 2 Oct 2023 20:12:01 +0000 Subject: [PATCH 1/2] fix(github-issues): fixes bug where entities that did not belong to the configured github instance were trying to be fetched Signed-off-by: Jonathan Nagayoshi --- .changeset/two-terms-play.md | 5 ++ .../src/api/githubIssuesApi.test.ts | 42 +++++++++++--- .../github-issues/src/api/githubIssuesApi.ts | 55 +++++++++++-------- .../GithubIssues/GithubIssues.test.tsx | 1 + .../src/hooks/useEntityGithubRepositories.ts | 30 +++++++--- .../src/hooks/useGetIssuesByRepoFromGithub.ts | 4 +- 6 files changed, 98 insertions(+), 39 deletions(-) create mode 100644 .changeset/two-terms-play.md diff --git a/.changeset/two-terms-play.md b/.changeset/two-terms-play.md new file mode 100644 index 0000000000..6c7edc01db --- /dev/null +++ b/.changeset/two-terms-play.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-github-issues': patch +--- + +Filters out entities that belonged to a different github instance other than the one configured by the plugin diff --git a/plugins/github-issues/src/api/githubIssuesApi.test.ts b/plugins/github-issues/src/api/githubIssuesApi.test.ts index e960d3123b..f965458858 100644 --- a/plugins/github-issues/src/api/githubIssuesApi.test.ts +++ b/plugins/github-issues/src/api/githubIssuesApi.test.ts @@ -21,9 +21,14 @@ jest.mock('octokit', () => ({ import { ConfigApi, ErrorApi } from '@backstage/core-plugin-api'; import { ForwardedError } from '@backstage/errors'; -import { createFilterByClause, githubIssuesApi } from './githubIssuesApi'; -import type { GithubIssuesFilters } from './githubIssuesApi'; +import { createFilterByClause, githubIssuesApi, Repository, GithubIssuesFilters } from './githubIssuesApi'; +function entityRepository(name: string, locationHostname: string = "github.com"): Repository { + return { + locationHostname, + name + } +} function getFragment( filterBy = '', orderBy = 'field: UPDATED_AT, direction: DESC', @@ -110,7 +115,26 @@ describe('githubIssuesApi', () => { it('should call GitHub API with correct query with fragment for each repo', async () => { await api.fetchIssuesByRepoFromGithub( - ['mrwolny/yo-yo', 'mrwolny/yoyo', 'mrwolny/yo.yo'], + [ + entityRepository('mrwolny/yo-yo'), + entityRepository('mrwolny/yoyo'), + entityRepository('mrwolny/yo.yo'), + ], + 10, + ); + + expect(mockGraphQLQuery).toHaveBeenCalledTimes(1); + expect(mockGraphQLQuery).toHaveBeenCalledWith(getFragment()); + }); + + it("should only fetch data for entities hosted in the same GitHub instance as the plugin's config", async () => { + await api.fetchIssuesByRepoFromGithub( + [ + entityRepository('mrwolny/yo-yo'), + entityRepository('mrwolny/yoyo'), + entityRepository('mrwolny/yo.yo'), + entityRepository('mrwolny/another-repo', "enterprise.github.com"), // This one should be filtered out + ], 10, ); @@ -120,7 +144,11 @@ describe('githubIssuesApi', () => { it('should call Github API with the correct filterBy and orderBy clauses', async () => { await api.fetchIssuesByRepoFromGithub( - ['mrwolny/yo-yo', 'mrwolny/yoyo', 'mrwolny/yo.yo'], + [ + entityRepository('mrwolny/yo-yo'), + entityRepository('mrwolny/yoyo'), + entityRepository('mrwolny/yo.yo') + ], 10, { filterBy: { @@ -231,7 +259,7 @@ describe('githubIssuesApi', () => { ); const data = await api.fetchIssuesByRepoFromGithub( - ['mrwolny/yo-yo', 'mrwolny/notfound'], + [entityRepository('mrwolny/yo-yo'), entityRepository('mrwolny/notfound')], 10, ); @@ -301,7 +329,7 @@ describe('githubIssuesApi', () => { ); const data = await api.fetchIssuesByRepoFromGithub( - ['mrwolny/notfound'], + [entityRepository('mrwolny/notfound')], 10, ); @@ -341,7 +369,7 @@ describe('githubIssuesApi', () => { mockErrorApi as unknown as ErrorApi, ); - await api.fetchIssuesByRepoFromGithub(['mrwolny/notfound'], 10); + await api.fetchIssuesByRepoFromGithub([entityRepository('mrwolny/notfound')], 10); expect(mockErrorApi.post).toHaveBeenCalledTimes(1); expect(mockErrorApi.post).toHaveBeenCalledWith( diff --git a/plugins/github-issues/src/api/githubIssuesApi.ts b/plugins/github-issues/src/api/githubIssuesApi.ts index 32331ef6b1..888d3c4c64 100644 --- a/plugins/github-issues/src/api/githubIssuesApi.ts +++ b/plugins/github-issues/src/api/githubIssuesApi.ts @@ -24,6 +24,11 @@ import { import { readGithubIntegrationConfigs } from '@backstage/integration'; import { ForwardedError } from '@backstage/errors'; +/** @internal */ +export type Repository = { + name: string; + locationHostname: string; +} /** @internal */ export type Assignee = { avatarUrl: string; @@ -116,9 +121,10 @@ export const githubIssuesApi = ( let octokit: Octokit; const getOctokit = async () => { - const baseUrl = readGithubIntegrationConfigs( + const githubConfig = readGithubIntegrationConfigs( configApi.getOptionalConfigArray('integrations.github') ?? [], - )[0].apiBaseUrl; + )[0] + const baseUrl = githubConfig.apiBaseUrl; const token = await githubAuthApi.getAccessToken(['repo']); @@ -126,11 +132,11 @@ export const githubIssuesApi = ( octokit = new Octokit({ auth: token, ...(baseUrl && { baseUrl }) }); } - return octokit.graphql; + return { graphql: octokit.graphql, hostname: githubConfig.host }; }; const fetchIssuesByRepoFromGithub = async ( - repos: Array, + repos: Array, itemsPerRepo: number, { filterBy, @@ -140,30 +146,35 @@ export const githubIssuesApi = ( }, }: GithubIssuesByRepoOptions = {}, ): Promise => { - const graphql = await getOctokit(); + const { graphql, hostname } = await getOctokit(); const safeNames: Array = []; + const repositories = repos + // only tries to fetch issues from repositories that are hosted on the same GitHub instance as the octokit + .filter(repo => repo.locationHostname === hostname) + .map(repo => { + const [owner, name] = repo.name.split('/'); - const repositories = repos.map(repo => { - const [owner, name] = repo.split('/'); + const safeNameRegex = /-|\./gi; + let safeName = name.replace(safeNameRegex, ''); - const safeNameRegex = /-|\./gi; - let safeName = name.replace(safeNameRegex, ''); + while (safeNames.includes(safeName)) { + safeName += 'x'; + } - while (safeNames.includes(safeName)) { - safeName += 'x'; - } + safeNames.push(safeName); - safeNames.push(safeName); - - return { - safeName, - name, - owner, - }; - }); + return { + safeName, + name, + owner, + }; + }); let issuesByRepo: IssuesByRepo = {}; try { + if(repositories.length === 0) { + throw new Error(`No repositories found for ${hostname}`) + } issuesByRepo = await graphql( createIssueByRepoQuery(repositories, itemsPerRepo, { filterBy, @@ -273,12 +284,12 @@ function createIssueByRepoQuery( query { ${repositories.map( - ({ safeName, name, owner }) => ` + ({ safeName, name, owner }) => ` ${safeName}: repository(name: "${name}", owner: "${owner}") { ...issues } `, - )} + )} } `; diff --git a/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx b/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx index 3f9e829de9..8123616721 100644 --- a/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx +++ b/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx @@ -68,6 +68,7 @@ const entityComponent = { metadata: { annotations: { 'github.com/project-slug': 'backstage/backstage', + 'backstage.io/source-location': "url:https://github.com/backstage/backstage" }, name: 'backstage', }, diff --git a/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts b/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts index f3fab72090..b8e02c2109 100644 --- a/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts +++ b/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts @@ -14,10 +14,11 @@ * limitations under the License. */ -import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; +import { Entity, stringifyEntityRef, getEntitySourceLocation } from '@backstage/catalog-model'; import { useApi } from '@backstage/core-plugin-api'; import { catalogApiRef, useEntity } from '@backstage/plugin-catalog-react'; import { useCallback, useEffect, useState } from 'react'; +import { Repository } from '../api'; const GITHUB_PROJECT_SLUG_ANNOTATION = 'github.com/project-slug'; @@ -25,18 +26,26 @@ export const getProjectNameFromEntity = (entity: Entity): string => { return entity?.metadata.annotations?.[GITHUB_PROJECT_SLUG_ANNOTATION] ?? ''; }; +export const getHostnameFromEntity = (entity: Entity): string => { + const { target } = getEntitySourceLocation(entity) + return new URL(target).hostname +} + export function useEntityGithubRepositories() { const { entity } = useEntity(); const catalogApi = useApi(catalogApiRef); - const [repositories, setRepositories] = useState([]); + const [repositories, setRepositories] = useState([]); const getRepositoriesNames = useCallback(async () => { if (entity.kind === 'Component' || entity.kind === 'API') { const entityName = getProjectNameFromEntity(entity); - + const locationHostname = getHostnameFromEntity(entity) if (entityName) { - setRepositories([entityName]); + setRepositories([{ + name: entityName, + locationHostname + }]); } return; @@ -49,11 +58,16 @@ export function useEntityGithubRepositories() { }, }); - const entitiesNames: string[] = entitiesList.items.map(componentEntity => - getProjectNameFromEntity(componentEntity), - ); + const repositoryEntities: Repository[] = entitiesList.items.reduce((acc: Repository[], componentEntity: Entity) => { + const entityName = getProjectNameFromEntity(componentEntity); + const entityLocationHostname = getHostnameFromEntity(componentEntity); + if (entityName && !acc.some((it: Repository) => it.name === entityName) && entityName.length) { + acc.push({ name: entityName, locationHostname: entityLocationHostname }); + } + return acc; + }, []); - setRepositories([...new Set(entitiesNames)].filter(name => name.length)); + setRepositories(repositoryEntities); }, [catalogApi, entity]); useEffect(() => { diff --git a/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts b/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts index 8ec5f3ec65..6891655f61 100644 --- a/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts +++ b/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts @@ -16,10 +16,10 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsyncRetry from 'react-use/lib/useAsyncRetry'; -import { githubIssuesApiRef, GithubIssuesByRepoOptions } from '../api'; +import { Repository, githubIssuesApiRef, GithubIssuesByRepoOptions } from '../api'; export const useGetIssuesByRepoFromGithub = ( - repos: Array, + repos: Array, itemsPerRepo: number, options?: GithubIssuesByRepoOptions, ) => { From 1e95687e9f087813c74a826173fb8050212c7d4d Mon Sep 17 00:00:00 2001 From: Jonathan Nagayoshi Date: Mon, 2 Oct 2023 21:07:32 +0000 Subject: [PATCH 2/2] fix(github-issues): fixes prettier rules Signed-off-by: Jonathan Nagayoshi --- .../src/api/githubIssuesApi.test.ts | 25 +++++++--- .../github-issues/src/api/githubIssuesApi.ts | 12 ++--- .../GithubIssues/GithubIssues.test.tsx | 3 +- .../src/hooks/useEntityGithubRepositories.ts | 50 ++++++++++++------- .../src/hooks/useGetIssuesByRepoFromGithub.ts | 6 ++- 5 files changed, 64 insertions(+), 32 deletions(-) diff --git a/plugins/github-issues/src/api/githubIssuesApi.test.ts b/plugins/github-issues/src/api/githubIssuesApi.test.ts index f965458858..f3c1b29675 100644 --- a/plugins/github-issues/src/api/githubIssuesApi.test.ts +++ b/plugins/github-issues/src/api/githubIssuesApi.test.ts @@ -21,13 +21,21 @@ jest.mock('octokit', () => ({ import { ConfigApi, ErrorApi } from '@backstage/core-plugin-api'; import { ForwardedError } from '@backstage/errors'; -import { createFilterByClause, githubIssuesApi, Repository, GithubIssuesFilters } from './githubIssuesApi'; +import { + createFilterByClause, + githubIssuesApi, + Repository, + GithubIssuesFilters, +} from './githubIssuesApi'; -function entityRepository(name: string, locationHostname: string = "github.com"): Repository { +function entityRepository( + name: string, + locationHostname: string = 'github.com', +): Repository { return { locationHostname, - name - } + name, + }; } function getFragment( filterBy = '', @@ -133,7 +141,7 @@ describe('githubIssuesApi', () => { entityRepository('mrwolny/yo-yo'), entityRepository('mrwolny/yoyo'), entityRepository('mrwolny/yo.yo'), - entityRepository('mrwolny/another-repo', "enterprise.github.com"), // This one should be filtered out + entityRepository('mrwolny/another-repo', 'enterprise.github.com'), // This one should be filtered out ], 10, ); @@ -147,7 +155,7 @@ describe('githubIssuesApi', () => { [ entityRepository('mrwolny/yo-yo'), entityRepository('mrwolny/yoyo'), - entityRepository('mrwolny/yo.yo') + entityRepository('mrwolny/yo.yo'), ], 10, { @@ -369,7 +377,10 @@ describe('githubIssuesApi', () => { mockErrorApi as unknown as ErrorApi, ); - await api.fetchIssuesByRepoFromGithub([entityRepository('mrwolny/notfound')], 10); + await api.fetchIssuesByRepoFromGithub( + [entityRepository('mrwolny/notfound')], + 10, + ); expect(mockErrorApi.post).toHaveBeenCalledTimes(1); expect(mockErrorApi.post).toHaveBeenCalledWith( diff --git a/plugins/github-issues/src/api/githubIssuesApi.ts b/plugins/github-issues/src/api/githubIssuesApi.ts index 888d3c4c64..d7b39989d8 100644 --- a/plugins/github-issues/src/api/githubIssuesApi.ts +++ b/plugins/github-issues/src/api/githubIssuesApi.ts @@ -28,7 +28,7 @@ import { ForwardedError } from '@backstage/errors'; export type Repository = { name: string; locationHostname: string; -} +}; /** @internal */ export type Assignee = { avatarUrl: string; @@ -123,7 +123,7 @@ export const githubIssuesApi = ( const getOctokit = async () => { const githubConfig = readGithubIntegrationConfigs( configApi.getOptionalConfigArray('integrations.github') ?? [], - )[0] + )[0]; const baseUrl = githubConfig.apiBaseUrl; const token = await githubAuthApi.getAccessToken(['repo']); @@ -172,8 +172,8 @@ export const githubIssuesApi = ( let issuesByRepo: IssuesByRepo = {}; try { - if(repositories.length === 0) { - throw new Error(`No repositories found for ${hostname}`) + if (repositories.length === 0) { + throw new Error(`No repositories found for ${hostname}`); } issuesByRepo = await graphql( createIssueByRepoQuery(repositories, itemsPerRepo, { @@ -284,12 +284,12 @@ function createIssueByRepoQuery( query { ${repositories.map( - ({ safeName, name, owner }) => ` + ({ safeName, name, owner }) => ` ${safeName}: repository(name: "${name}", owner: "${owner}") { ...issues } `, - )} + )} } `; diff --git a/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx b/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx index 8123616721..bf3cac7423 100644 --- a/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx +++ b/plugins/github-issues/src/components/GithubIssues/GithubIssues.test.tsx @@ -68,7 +68,8 @@ const entityComponent = { metadata: { annotations: { 'github.com/project-slug': 'backstage/backstage', - 'backstage.io/source-location': "url:https://github.com/backstage/backstage" + 'backstage.io/source-location': + 'url:https://github.com/backstage/backstage', }, name: 'backstage', }, diff --git a/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts b/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts index b8e02c2109..b9763298e6 100644 --- a/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts +++ b/plugins/github-issues/src/hooks/useEntityGithubRepositories.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { Entity, stringifyEntityRef, getEntitySourceLocation } from '@backstage/catalog-model'; +import { + Entity, + stringifyEntityRef, + getEntitySourceLocation, +} from '@backstage/catalog-model'; import { useApi } from '@backstage/core-plugin-api'; import { catalogApiRef, useEntity } from '@backstage/plugin-catalog-react'; import { useCallback, useEffect, useState } from 'react'; @@ -27,9 +31,9 @@ export const getProjectNameFromEntity = (entity: Entity): string => { }; export const getHostnameFromEntity = (entity: Entity): string => { - const { target } = getEntitySourceLocation(entity) - return new URL(target).hostname -} + const { target } = getEntitySourceLocation(entity); + return new URL(target).hostname; +}; export function useEntityGithubRepositories() { const { entity } = useEntity(); @@ -40,12 +44,14 @@ export function useEntityGithubRepositories() { const getRepositoriesNames = useCallback(async () => { if (entity.kind === 'Component' || entity.kind === 'API') { const entityName = getProjectNameFromEntity(entity); - const locationHostname = getHostnameFromEntity(entity) + const locationHostname = getHostnameFromEntity(entity); if (entityName) { - setRepositories([{ - name: entityName, - locationHostname - }]); + setRepositories([ + { + name: entityName, + locationHostname, + }, + ]); } return; @@ -58,14 +64,24 @@ export function useEntityGithubRepositories() { }, }); - const repositoryEntities: Repository[] = entitiesList.items.reduce((acc: Repository[], componentEntity: Entity) => { - const entityName = getProjectNameFromEntity(componentEntity); - const entityLocationHostname = getHostnameFromEntity(componentEntity); - if (entityName && !acc.some((it: Repository) => it.name === entityName) && entityName.length) { - acc.push({ name: entityName, locationHostname: entityLocationHostname }); - } - return acc; - }, []); + const repositoryEntities: Repository[] = entitiesList.items.reduce( + (acc: Repository[], componentEntity: Entity) => { + const entityName = getProjectNameFromEntity(componentEntity); + const entityLocationHostname = getHostnameFromEntity(componentEntity); + if ( + entityName && + !acc.some((it: Repository) => it.name === entityName) && + entityName.length + ) { + acc.push({ + name: entityName, + locationHostname: entityLocationHostname, + }); + } + return acc; + }, + [], + ); setRepositories(repositoryEntities); }, [catalogApi, entity]); diff --git a/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts b/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts index 6891655f61..0b5bad7f57 100644 --- a/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts +++ b/plugins/github-issues/src/hooks/useGetIssuesByRepoFromGithub.ts @@ -16,7 +16,11 @@ import { useApi } from '@backstage/core-plugin-api'; import useAsyncRetry from 'react-use/lib/useAsyncRetry'; -import { Repository, githubIssuesApiRef, GithubIssuesByRepoOptions } from '../api'; +import { + Repository, + githubIssuesApiRef, + GithubIssuesByRepoOptions, +} from '../api'; export const useGetIssuesByRepoFromGithub = ( repos: Array,