diff --git a/.changeset/lovely-cycles-shout.md b/.changeset/lovely-cycles-shout.md new file mode 100644 index 0000000000..f65e2520bf --- /dev/null +++ b/.changeset/lovely-cycles-shout.md @@ -0,0 +1,5 @@ +--- +'@backstage/catalog-client': patch +--- + +Update `AddLocationResponse` to optionally return `exists` to signal that the location already exists, this is only returned when calling `addLocation` in dryRun. diff --git a/.changeset/lovely-keys-occur.md b/.changeset/lovely-keys-occur.md new file mode 100644 index 0000000000..b01ccd3bf5 --- /dev/null +++ b/.changeset/lovely-keys-occur.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend': patch +--- + +Update `createLocation` to optionally return `exists` to signal that the location already exists, this is only returned for dry runs. diff --git a/.changeset/unlucky-laws-divide.md b/.changeset/unlucky-laws-divide.md new file mode 100644 index 0000000000..0a1c69c73a --- /dev/null +++ b/.changeset/unlucky-laws-divide.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-import': patch +--- + +The import form is now aware of locations that already exist. It lists them separately and shows a button for triggering a refresh. diff --git a/packages/catalog-client/api-report.md b/packages/catalog-client/api-report.md index c2b78aaa90..8a81c0eb10 100644 --- a/packages/catalog-client/api-report.md +++ b/packages/catalog-client/api-report.md @@ -19,6 +19,7 @@ export type AddLocationRequest = { export type AddLocationResponse = { location: Location_2; entities: Entity[]; + exists?: boolean; }; // @public (undocumented) diff --git a/packages/catalog-client/src/CatalogClient.ts b/packages/catalog-client/src/CatalogClient.ts index 3b697fc597..1a6bc5c70d 100644 --- a/packages/catalog-client/src/CatalogClient.ts +++ b/packages/catalog-client/src/CatalogClient.ts @@ -176,7 +176,7 @@ export class CatalogClient implements CatalogApi { throw new Error(await response.text()); } - const { location, entities } = await response.json(); + const { location, entities, exists } = await response.json(); if (!location) { throw new Error(`Location wasn't added: ${target}`); @@ -185,6 +185,7 @@ export class CatalogClient implements CatalogApi { return { location, entities, + exists, }; } diff --git a/packages/catalog-client/src/types/api.ts b/packages/catalog-client/src/types/api.ts index b95ea335c2..679588b10a 100644 --- a/packages/catalog-client/src/types/api.ts +++ b/packages/catalog-client/src/types/api.ts @@ -93,4 +93,6 @@ export type AddLocationRequest = { export type AddLocationResponse = { location: Location; entities: Entity[]; + // Exists is only set in DryRun mode. + exists?: boolean; }; diff --git a/plugins/catalog-backend/api-report.md b/plugins/catalog-backend/api-report.md index 72216ad151..89ef2c09d1 100644 --- a/plugins/catalog-backend/api-report.md +++ b/plugins/catalog-backend/api-report.md @@ -1148,6 +1148,7 @@ export interface LocationService { ): Promise<{ location: Location_2; entities: Entity[]; + exists?: boolean; }>; // (undocumented) deleteLocation(id: string): Promise; diff --git a/plugins/catalog-backend/src/next/DefaultLocationService.test.ts b/plugins/catalog-backend/src/next/DefaultLocationService.test.ts index f965ec3077..939f54e793 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationService.test.ts @@ -36,6 +36,7 @@ describe('DefaultLocationServiceTest', () => { describe('createLocation', () => { it('should support dry run', async () => { + store.listLocations.mockResolvedValueOnce([]); orchestrator.process.mockResolvedValueOnce({ ok: true, state: new Map(), @@ -116,6 +117,63 @@ describe('DefaultLocationServiceTest', () => { expect(store.createLocation).not.toBeCalled(); }); + it('should check for location existence when running in dry run', async () => { + const locationSpec = { + type: 'url', + target: 'https://backstage.io/catalog-info.yaml', + }; + orchestrator.process.mockResolvedValueOnce({ + ok: true, + state: new Map(), + completedEntity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'bar', + }, + }, + deferredEntities: [], + relations: [], + errors: [], + }); + + store.listLocations.mockResolvedValueOnce([ + { id: '137', ...locationSpec }, + ]); + const result = await locationService.createLocation( + { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, + true, + ); + + expect(result.exists).toBe(true); + }); + + it('should return exists false when the location does not exist beforehand', async () => { + orchestrator.process.mockResolvedValueOnce({ + ok: true, + state: new Map(), + completedEntity: { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'bar', + }, + }, + deferredEntities: [], + relations: [], + errors: [], + }); + + store.listLocations.mockResolvedValueOnce([ + { id: '987', type: 'url', target: 'https://example.com' }, + ]); + const result = await locationService.createLocation( + { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, + true, + ); + expect(result.exists).toBe(false); + }); + it('should create location', async () => { const locationSpec = { type: 'url', diff --git a/plugins/catalog-backend/src/next/DefaultLocationService.ts b/plugins/catalog-backend/src/next/DefaultLocationService.ts index 2820ba14b1..7d485bea40 100644 --- a/plugins/catalog-backend/src/next/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/next/DefaultLocationService.ts @@ -36,7 +36,7 @@ export class DefaultLocationService implements LocationService { async createLocation( spec: LocationSpec, dryRun: boolean, - ): Promise<{ location: Location; entities: Entity[] }> { + ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> { if (dryRun) { return this.dryRunCreateLocation(spec); } @@ -56,7 +56,14 @@ export class DefaultLocationService implements LocationService { private async dryRunCreateLocation( spec: LocationSpec, - ): Promise<{ location: Location; entities: Entity[] }> { + ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> { + // Run the existence check in parallel with the processing + const existsPromise = this.store + .listLocations() + .then(locations => + locations.some(l => l.type === spec.type && l.target === spec.target), + ); + const entity = { apiVersion: 'backstage.io/v1alpha1', kind: 'Location', @@ -100,6 +107,7 @@ export class DefaultLocationService implements LocationService { } return { + exists: await existsPromise, location: { ...spec, id: `${spec.type}:${spec.target}` }, entities, }; diff --git a/plugins/catalog-backend/src/next/types.ts b/plugins/catalog-backend/src/next/types.ts index fa058c4138..4131b5835b 100644 --- a/plugins/catalog-backend/src/next/types.ts +++ b/plugins/catalog-backend/src/next/types.ts @@ -21,7 +21,7 @@ export interface LocationService { createLocation( spec: LocationSpec, dryRun: boolean, - ): Promise<{ location: Location; entities: Entity[] }>; + ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }>; listLocations(): Promise; getLocation(id: string): Promise; deleteLocation(id: string): Promise; diff --git a/plugins/catalog-import/api-report.md b/plugins/catalog-import/api-report.md index 753244b28f..4deb66e01b 100644 --- a/plugins/catalog-import/api-report.md +++ b/plugins/catalog-import/api-report.md @@ -34,6 +34,7 @@ export type AnalyzeResult = type: 'locations'; locations: Array<{ target: string; + exists?: boolean; entities: EntityName[]; }>; } @@ -248,5 +249,5 @@ export const StepPrepareCreatePullRequest: ({ // Warnings were encountered during analysis: // -// src/api/CatalogImportApi.d.ts:14:5 - (ae-forgotten-export) The symbol "PartialEntity" needs to be exported by the entry point index.d.ts +// src/api/CatalogImportApi.d.ts:15:5 - (ae-forgotten-export) The symbol "PartialEntity" needs to be exported by the entry point index.d.ts ``` diff --git a/plugins/catalog-import/package.json b/plugins/catalog-import/package.json index 76d11edda3..8a152d9e50 100644 --- a/plugins/catalog-import/package.json +++ b/plugins/catalog-import/package.json @@ -52,7 +52,8 @@ "react-router": "6.0.0-beta.0", "react-router-dom": "6.0.0-beta.0", "react-use": "^17.2.4", - "yaml": "^1.10.0" + "yaml": "^1.10.0", + "lodash": "^4.17.21" }, "devDependencies": { "@backstage/cli": "^0.7.12", diff --git a/plugins/catalog-import/src/api/CatalogImportApi.ts b/plugins/catalog-import/src/api/CatalogImportApi.ts index a4645dc7ef..89c27f1875 100644 --- a/plugins/catalog-import/src/api/CatalogImportApi.ts +++ b/plugins/catalog-import/src/api/CatalogImportApi.ts @@ -29,6 +29,7 @@ export type AnalyzeResult = type: 'locations'; locations: Array<{ target: string; + exists?: boolean; entities: EntityName[]; }>; } diff --git a/plugins/catalog-import/src/api/CatalogImportClient.ts b/plugins/catalog-import/src/api/CatalogImportClient.ts index 242f8440d1..c804d1193d 100644 --- a/plugins/catalog-import/src/api/CatalogImportClient.ts +++ b/plugins/catalog-import/src/api/CatalogImportClient.ts @@ -31,6 +31,7 @@ import { Base64 } from 'js-base64'; import { PartialEntity } from '../types'; import { AnalyzeResult, CatalogImportApi } from './CatalogImportApi'; import { getGithubIntegrationConfig } from './GitHub'; +import { trimEnd } from 'lodash'; export class CatalogImportClient implements CatalogImportApi { private readonly discoveryApi: DiscoveryApi; @@ -71,6 +72,7 @@ export class CatalogImportClient implements CatalogImportApi { type: 'locations', locations: [ { + exists: location.exists, target: location.location.target, entities: location.entities.map(e => ({ kind: e.kind, @@ -238,29 +240,23 @@ the component will become available.\n\nFor more information, read an \ return await Promise.all( searchResult.data.items - .map( - i => `${url.replace(/[\/]*$/, '')}/blob/${defaultBranch}/${i.path}`, - ) - .map( - async i => - ({ - target: i, - entities: ( - await this.catalogApi.addLocation({ - type: 'url', - target: i, - dryRun: true, - }) - ).entities.map(e => ({ - kind: e.kind, - namespace: e.metadata.namespace ?? 'default', - name: e.metadata.name, - })), - } as { - target: string; - entities: EntityName[]; - }), - ), + .map(i => `${trimEnd(url, '/')}/blob/${defaultBranch}/${i.path}`) + .map(async target => { + const result = await this.catalogApi.addLocation({ + type: 'url', + target, + dryRun: true, + }); + return { + target, + exists: result.exists, + entities: result.entities.map(e => ({ + kind: e.kind, + namespace: e.metadata.namespace ?? 'default', + name: e.metadata.name, + })), + }; + }), ); } diff --git a/plugins/catalog-import/src/components/StepFinishImportLocation/StepFinishImportLocation.tsx b/plugins/catalog-import/src/components/StepFinishImportLocation/StepFinishImportLocation.tsx index 423a60cef0..8d1a588079 100644 --- a/plugins/catalog-import/src/components/StepFinishImportLocation/StepFinishImportLocation.tsx +++ b/plugins/catalog-import/src/components/StepFinishImportLocation/StepFinishImportLocation.tsx @@ -21,15 +21,22 @@ import { BackButton } from '../Buttons'; import { EntityListComponent } from '../EntityListComponent'; import { PrepareResult } from '../useImportState'; import { Link } from '@backstage/core-components'; +import partition from 'lodash/partition'; type Props = { prepareResult: PrepareResult; onReset: () => void; }; -export const StepFinishImportLocation = ({ prepareResult, onReset }: Props) => ( - <> - {prepareResult.type === 'repository' && ( +export const StepFinishImportLocation = ({ prepareResult, onReset }: Props) => { + const continueButton = ( + + Register another + + ); + + if (prepareResult.type === 'repository') { + return ( <> The following Pull Request has been opened:{' '} @@ -45,21 +52,46 @@ export const StepFinishImportLocation = ({ prepareResult, onReset }: Props) => ( Your entities will be imported as soon as the Pull Request is merged. + + {continueButton} - )} + ); + } - - The following entities have been added to the catalog: - + const [existingLocations, newLocations] = partition( + prepareResult.locations, + l => l.exists, + ); - } - withLinks - /> + return ( + <> + {newLocations.length > 0 && ( + <> + + The following entities have been added to the catalog: + - - Register another - - -); + } + withLinks + /> + + )} + {existingLocations.length > 0 && ( + <> + + A refresh was triggered for the following locations: + + + } + withLinks + /> + + )} + {continueButton} + + ); +}; diff --git a/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.test.tsx b/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.test.tsx index eb1d95e14d..6a5e952491 100644 --- a/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.test.tsx +++ b/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.test.tsx @@ -26,7 +26,7 @@ describe('', () => { type: 'locations', locations: [ { - target: 'url', + target: 'url-1', entities: [ { kind: 'component', @@ -57,8 +57,8 @@ describe('', () => { jest.resetAllMocks(); }); - it('renders without exploding', async () => { - const { getByRole } = await renderInTestApp( + it('renders display locations to be added', async () => { + const rendered = await renderInTestApp( undefined} @@ -66,7 +66,48 @@ describe('', () => { />, ); - expect(getByRole('button', { name: /Review/i })).toBeDisabled(); + expect(rendered.getByText('url-1')).toBeInTheDocument(); + expect(rendered.getByText('url-2')).toBeInTheDocument(); + expect( + rendered.queryByText(/Select one or more locations/), + ).toBeInTheDocument(); + expect( + rendered.queryByText(/locations already exist/), + ).not.toBeInTheDocument(); + expect(rendered.getByRole('button', { name: /Review/i })).toBeDisabled(); + }); + + it('should display existing locations only', async () => { + const analyzeResultWithExistingLocation = { + type: 'locations', + locations: [ + { + target: 'my-target', + exists: true, + entities: [ + { + kind: 'component', + namespace: 'default', + name: 'name', + }, + ], + }, + ], + } as Extract; + + const rendered = await renderInTestApp( + undefined} + onGoBack={() => undefined} + />, + ); + + expect(rendered.getByText(/my-target/)).toBeInTheDocument(); + expect(rendered.queryByText(/locations already exist/)).toBeInTheDocument(); + expect( + rendered.queryByText(/Select one or more locations/), + ).not.toBeInTheDocument(); }); it('should select and deselect all', async () => { @@ -188,7 +229,7 @@ describe('', () => { type: 'locations', locations: [ { - target: 'url', + target: 'url-1', entities: [ { kind: 'component', diff --git a/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.tsx b/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.tsx index 179b7e7c2e..e7bedef9cc 100644 --- a/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.tsx +++ b/plugins/catalog-import/src/components/StepPrepareSelectLocations/StepPrepareSelectLocations.tsx @@ -22,11 +22,13 @@ import { ListItemText, Typography, } from '@material-ui/core'; +import LocationOnIcon from '@material-ui/icons/LocationOn'; import React, { useCallback, useState } from 'react'; import { AnalyzeResult } from '../../api'; import { BackButton, NextButton } from '../Buttons'; import { EntityListComponent } from '../EntityListComponent'; import { PrepareResult } from '../useImportState'; +import partition from 'lodash/partition'; type Props = { analyzeResult: Extract; @@ -53,14 +55,17 @@ export const StepPrepareSelectLocations = ({ prepareResult?.locations.map(l => l.target) || [], ); + const [existingLocations, locations] = partition( + analyzeResult?.locations, + l => l.exists, + ); + const handleResult = useCallback(async () => { onPrepare({ type: 'locations', - locations: analyzeResult.locations.filter((l: any) => - selectedUrls.includes(l.target), - ), + locations: locations.filter((l: any) => selectedUrls.includes(l.target)), }); - }, [analyzeResult.locations, onPrepare, selectedUrls]); + }, [locations, onPrepare, selectedUrls]); const onItemClick = (url: string) => { setSelectedUrls(urls => @@ -70,48 +75,62 @@ export const StepPrepareSelectLocations = ({ const onSelectAll = () => { setSelectedUrls(urls => - urls.length < analyzeResult.locations.length - ? analyzeResult.locations.map(l => l.target) - : [], + urls.length < locations.length ? locations.map(l => l.target) : [], ); }; return ( <> - - Select one or more locations that are present in your git repository: - - - - + {locations.length > 0 && ( + <> + + Select one or more locations that are present in your git + repository: + + + + 0 && + selectedUrls.length < locations.length + } + tabIndex={-1} + disableRipple + /> + + + + } + onItemClick={onItemClick} + locations={locations} + locationListItemIcon={target => ( 0 && - selectedUrls.length < analyzeResult.locations.length - } + checked={selectedUrls.includes(target)} tabIndex={-1} disableRipple /> - - - - } - onItemClick={onItemClick} - locations={analyzeResult.locations} - locationListItemIcon={target => ( - - )} - collapsed - /> + + )} + + {existingLocations.length > 0 && ( + <> + These locations already exist in the catalog: + } + withLinks + collapsed + /> + + )} {onGoBack && } diff --git a/plugins/catalog-import/src/components/StepReviewLocation/StepReviewLocation.tsx b/plugins/catalog-import/src/components/StepReviewLocation/StepReviewLocation.tsx index ff3276aa65..99dc972678 100644 --- a/plugins/catalog-import/src/components/StepReviewLocation/StepReviewLocation.tsx +++ b/plugins/catalog-import/src/components/StepReviewLocation/StepReviewLocation.tsx @@ -24,6 +24,7 @@ import { PrepareResult, ReviewResult } from '../useImportState'; import { configApiRef, useApi } from '@backstage/core-plugin-api'; import { Link } from '@backstage/core-components'; +import { stringifyEntityRef } from '@backstage/catalog-model'; type Props = { prepareResult: PrepareResult; @@ -43,27 +44,48 @@ export const StepReviewLocation = ({ const [submitted, setSubmitted] = useState(false); const [error, setError] = useState(); - - const handleImport = useCallback(async () => { + const exists = + prepareResult.type === 'locations' && + prepareResult.locations.some(l => l.exists) + ? true + : false; + const handleClick = useCallback(async () => { setSubmitted(true); try { - const result = await Promise.all( - prepareResult.locations.map(l => - catalogApi.addLocation({ - type: 'url', - target: l.target, - presence: - prepareResult.type === 'repository' ? 'optional' : 'required', + let refreshed = new Array<{ target: string }>(); + if (prepareResult.type === 'locations') { + refreshed = await Promise.all( + prepareResult.locations + .filter(l => l.exists) + .map(async l => { + const ref = stringifyEntityRef(l.entities[0] ?? l); + await catalogApi.refreshEntity(ref); + return { target: l.target }; + }), + ); + } + + const locations = await Promise.all( + prepareResult.locations + .filter((l: unknown) => !(l as { exists?: boolean }).exists) + .map(async l => { + const result = await catalogApi.addLocation({ + type: 'url', + target: l.target, + presence: + prepareResult.type === 'repository' ? 'optional' : 'required', + }); + return { + target: result.location.target, + entities: result.entities, + }; }), - ), ); onReview({ ...prepareResult, - locations: result.map(r => ({ - target: r.location.target, - entities: r.entities, - })), + ...{ refreshed }, + locations, }); } catch (e) { // TODO: this error should be handled differently. We add it as 'optional' and @@ -111,7 +133,9 @@ export const StepReviewLocation = ({ )} - The following entities will be added to the catalog: + {exists + ? 'The following locations already exist in the catalog:' + : 'The following entities will be added to the catalog:'} handleImport()} + onClick={() => handleClick()} > - Import + {exists ? 'Refresh' : 'Import'} diff --git a/plugins/catalog-import/src/components/useImportState.test.tsx b/plugins/catalog-import/src/components/useImportState.test.tsx index 260fb66119..1a01c61449 100644 --- a/plugins/catalog-import/src/components/useImportState.test.tsx +++ b/plugins/catalog-import/src/components/useImportState.test.tsx @@ -50,6 +50,7 @@ describe('useImportState', () => { entities: [] as Entity[], }, ], + refreshed: [], }; it('should use initial url', async () => { @@ -131,14 +132,16 @@ describe('useImportState', () => { }); act(() => result.current.onReset()); - expect(result.current).toMatchObject({ activeFlow: 'unknown', activeStepNumber: 0, analysisUrl: undefined, activeState: 'analyze', analyzeResult: undefined, - prepareResult: locationR, + prepareResult: { + type: 'locations', + locations: [{ target: 'https://0', entities: [] }], + }, reviewResult: undefined, }); }); diff --git a/plugins/catalog-import/src/components/useImportState.ts b/plugins/catalog-import/src/components/useImportState.ts index c823499cdf..b5fa007144 100644 --- a/plugins/catalog-import/src/components/useImportState.ts +++ b/plugins/catalog-import/src/components/useImportState.ts @@ -33,6 +33,7 @@ export type PrepareResult = | { type: 'locations'; locations: Array<{ + exists?: boolean; target: string; entities: EntityName[]; }>; @@ -58,6 +59,7 @@ export type ReviewResult = target: string; entities: Entity[]; }>; + refreshed: Array<{ target: string }>; } | { type: 'repository';