From 9266b80ab36a17aadd81673e830576beb50416b2 Mon Sep 17 00:00:00 2001 From: Jussi Hallila Date: Tue, 13 Jul 2021 09:07:18 +0200 Subject: [PATCH 01/29] Implement DefaultTechDocsCollator * Implements a collator for tech docs. * Retrieves mkdocs created search index for entities that have documentation configured * Registers collator to expose tech docs content to be searchable * Adds pagination to example search * Modifies example search to contain tech docs * Displays docs results with link to docs and the entity name as title. * Creates a reusable type filter to be located in the search package. * Add tests for type filter Signed-off-by: Jussi Hallila --- .changeset/funny-dragons-sneeze.md | 5 + .changeset/great-tips-hammer.md | 5 + .changeset/neat-wolves-cross.md | 5 + packages/app/package.json | 2 + .../app/src/components/search/SearchPage.tsx | 91 ++++--- packages/backend/src/plugins/search.ts | 6 + plugins/search/api-report.md | 11 + .../components/SearchType/SearchType.test.tsx | 231 ++++++++++++++++++ .../src/components/SearchType/SearchType.tsx | 110 +++++++++ .../search/src/components/SearchType/index.ts | 17 ++ plugins/search/src/components/index.tsx | 1 + plugins/search/src/index.ts | 1 + plugins/techdocs-backend/api-report.md | 51 ++++ plugins/techdocs-backend/package.json | 4 + plugins/techdocs-backend/src/index.ts | 1 + .../search/DefaultTechDocsCollator.test.ts | 149 +++++++++++ .../src/search/DefaultTechDocsCollator.ts | 149 +++++++++++ plugins/techdocs-backend/src/search/index.ts | 17 ++ plugins/techdocs/api-report.md | 11 + plugins/techdocs/package.json | 1 + .../DocsResultListItem.test.tsx | 50 ++++ .../DocsResultListItem/DocsResultListItem.tsx | 60 +++++ .../components/DocsResultListItem/index.ts | 17 ++ plugins/techdocs/src/index.ts | 1 + 24 files changed, 968 insertions(+), 28 deletions(-) create mode 100644 .changeset/funny-dragons-sneeze.md create mode 100644 .changeset/great-tips-hammer.md create mode 100644 .changeset/neat-wolves-cross.md create mode 100644 plugins/search/src/components/SearchType/SearchType.test.tsx create mode 100644 plugins/search/src/components/SearchType/SearchType.tsx create mode 100644 plugins/search/src/components/SearchType/index.ts create mode 100644 plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts create mode 100644 plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts create mode 100644 plugins/techdocs-backend/src/search/index.ts create mode 100644 plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.test.tsx create mode 100644 plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.tsx create mode 100644 plugins/techdocs/src/components/DocsResultListItem/index.ts diff --git a/.changeset/funny-dragons-sneeze.md b/.changeset/funny-dragons-sneeze.md new file mode 100644 index 0000000000..758603e811 --- /dev/null +++ b/.changeset/funny-dragons-sneeze.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Add search list item to display tech docs search results diff --git a/.changeset/great-tips-hammer.md b/.changeset/great-tips-hammer.md new file mode 100644 index 0000000000..2f31b59568 --- /dev/null +++ b/.changeset/great-tips-hammer.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-backend': patch +--- + +Implements tech docs collator to retrieve and expose search indexes for entities that have tech docs configured diff --git a/.changeset/neat-wolves-cross.md b/.changeset/neat-wolves-cross.md new file mode 100644 index 0000000000..555245d42d --- /dev/null +++ b/.changeset/neat-wolves-cross.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-search': patch +--- + +Adding a type filter to new search diff --git a/packages/app/package.json b/packages/app/package.json index d86e0f27e9..cc38248a55 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -39,9 +39,11 @@ "@backstage/plugin-techdocs": "^0.10.0", "@backstage/plugin-todo": "^0.1.5", "@backstage/plugin-user-settings": "^0.3.0", + "@backstage/search-common": "^0.1.2", "@backstage/theme": "^0.2.8", "@material-ui/core": "^4.11.0", "@material-ui/icons": "^4.9.1", + "@material-ui/lab": "4.0.0-alpha.45", "@octokit/rest": "^18.5.3", "@roadiehq/backstage-plugin-buildkite": "^1.0.4", "@roadiehq/backstage-plugin-github-insights": "^1.1.15", diff --git a/packages/app/src/components/search/SearchPage.tsx b/packages/app/src/components/search/SearchPage.tsx index 35c4a4ac1c..6fe02de837 100644 --- a/packages/app/src/components/search/SearchPage.tsx +++ b/packages/app/src/components/search/SearchPage.tsx @@ -14,17 +14,20 @@ * limitations under the License. */ -import React from 'react'; +import React, { useState } from 'react'; import { makeStyles, Theme, Grid, List, Paper } from '@material-ui/core'; - +import Pagination from '@material-ui/lab/Pagination'; import { CatalogResultListItem } from '@backstage/plugin-catalog'; import { SearchBar, SearchFilter, SearchResult, + SearchType, DefaultResultListItem, } from '@backstage/plugin-search'; import { Content, Header, Lifecycle, Page } from '@backstage/core-components'; +import { DocsResultListItem } from '@backstage/plugin-techdocs'; +import { SearchResultSet } from '@backstage/search-common'; const useStyles = makeStyles((theme: Theme) => ({ bar: { @@ -34,15 +37,63 @@ const useStyles = makeStyles((theme: Theme) => ({ padding: theme.spacing(2), }, filter: { - '& + &': { - marginTop: theme.spacing(2.5), - }, + marginTop: theme.spacing(2.5), }, })); +// TODO: Move this into the search plugin once pagination is natively supported. +// See: https://github.com/backstage/backstage/issues/6062 +const SearchResultList = ({ results }: SearchResultSet) => { + const pageSize = 10; + const [page, setPage] = useState(1); + const changePage = (_: any, pageIndex: number) => { + setPage(pageIndex); + }; + const pageAmount = Math.ceil((results.length || 0) / pageSize); + return ( + <> + + {results + .slice(pageSize * (page - 1), pageSize * page) + .map(({ type, document }) => { + switch (type) { + case 'software-catalog': + return ( + + ); + case 'techdocs': + return ( + + ); + default: + return ( + + ); + } + })} + + + + ); +}; + const SearchPage = () => { const classes = useStyles(); - return (
} /> @@ -55,6 +106,11 @@ const SearchPage = () => { + { - {({ results }) => ( - - {results.map(({ type, document }) => { - switch (type) { - case 'software-catalog': - return ( - - ); - default: - return ( - - ); - } - })} - - )} + {({ results }) => } diff --git a/packages/backend/src/plugins/search.ts b/packages/backend/src/plugins/search.ts index 4a1e415c74..7e7f6ae400 100644 --- a/packages/backend/src/plugins/search.ts +++ b/packages/backend/src/plugins/search.ts @@ -21,6 +21,7 @@ import { } from '@backstage/plugin-search-backend-node'; import { PluginEnvironment } from '../types'; import { DefaultCatalogCollator } from '@backstage/plugin-catalog-backend'; +import { DefaultTechDocsCollator } from '@backstage/plugin-techdocs-backend'; export default async function createPlugin({ logger, @@ -37,6 +38,11 @@ export default async function createPlugin({ collator: new DefaultCatalogCollator({ discovery }), }); + indexBuilder.addCollator({ + defaultRefreshIntervalSeconds: 600, + collator: new DefaultTechDocsCollator({ discovery, logger }), + }); + // The scheduler controls when documents are gathered from collators and sent // to the search engine for indexing. const { scheduler } = await indexBuilder.build(); diff --git a/plugins/search/api-report.md b/plugins/search/api-report.md index 505a8989b6..525136713d 100644 --- a/plugins/search/api-report.md +++ b/plugins/search/api-report.md @@ -143,6 +143,17 @@ export const SearchResult: ({ children: (results: { results: SearchResult_2[] }) => JSX.Element; }) => JSX.Element; +// Warning: (ae-forgotten-export) The symbol "SearchTypeProps" needs to be exported by the entry point index.d.ts +// Warning: (ae-missing-release-tag) "SearchType" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export const SearchType: ({ + values, + className, + name, + defaultValue, +}: SearchTypeProps) => JSX.Element; + // Warning: (ae-missing-release-tag) "SidebarSearch" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/plugins/search/src/components/SearchType/SearchType.test.tsx b/plugins/search/src/components/SearchType/SearchType.test.tsx new file mode 100644 index 0000000000..1ca032e41d --- /dev/null +++ b/plugins/search/src/components/SearchType/SearchType.test.tsx @@ -0,0 +1,231 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { screen, render, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { SearchType } from './SearchType'; + +import { SearchContextProvider } from '../SearchContext'; +import { useApi } from '@backstage/core-plugin-api'; + +jest.mock('@backstage/core-plugin-api', () => ({ + ...jest.requireActual('@backstage/core-plugin-api'), + useApi: jest.fn().mockReturnValue({}), +})); + +describe('SearchType', () => { + const initialState = { + term: '', + filters: {}, + types: [], + pageCursor: '', + }; + + const name = 'field'; + const values = ['value1', 'value2']; + const typeValues = ['preselected']; + + const query = jest.fn().mockResolvedValue({}); + (useApi as jest.Mock).mockReturnValue({ query: query }); + + afterAll(() => { + jest.resetAllMocks(); + }); + + describe('Type Filter', () => { + it('Renders field name and values when provided as props', async () => { + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('button')); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + expect( + screen.getByRole('option', { name: values[0] }), + ).toBeInTheDocument(); + expect( + screen.getByRole('option', { name: values[1] }), + ).toBeInTheDocument(); + }); + + it('Renders correctly based on type filter state', async () => { + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('button')); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + expect(screen.getByRole('option', { name: values[0] })).toHaveAttribute( + 'aria-selected', + 'true', + ); + expect( + screen.getByRole('option', { name: values[1] }), + ).not.toHaveAttribute('aria-selected'); + expect(screen.getByRole('option', { name: 'All' })).not.toHaveAttribute( + 'aria-selected', + ); + }); + + it('Renders correctly based on type filter defaultValue', async () => { + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('button')); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + expect(screen.getByRole('option', { name: values[0] })).toHaveAttribute( + 'aria-selected', + 'true', + ); + expect( + screen.getByRole('option', { name: values[1] }), + ).not.toHaveAttribute('aria-selected'); + expect(screen.getByRole('option', { name: 'All' })).not.toHaveAttribute( + 'aria-selected', + ); + }); + + it('Selecting a value sets type filter state', async () => { + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + const button = screen.getByRole('button'); + + userEvent.click(button); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('option', { name: values[0] })); + + await waitFor(() => { + expect(query).toHaveBeenLastCalledWith( + expect.objectContaining({ + types: [values[0]], + }), + ); + }); + + userEvent.click(button); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('option', { name: 'All' })); + + await waitFor(() => { + expect(query).toHaveBeenLastCalledWith( + expect.objectContaining({ + types: [], + }), + ); + }); + }); + + it('Selecting a value maintains unrelated filter state, selecting All defaults to default empty state', async () => { + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByText(name)).toBeInTheDocument(); + }); + + const button = screen.getByRole('button'); + + userEvent.click(button); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('option', { name: values[0] })); + + await waitFor(() => { + expect(query).toHaveBeenLastCalledWith( + expect.objectContaining({ + types: [...typeValues, values[0]], + }), + ); + }); + + userEvent.click(button); + + await waitFor(() => { + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + userEvent.click(screen.getByRole('option', { name: 'All' })); + + await waitFor(() => { + expect(query).toHaveBeenLastCalledWith(expect.objectContaining([])); + }); + }); + }); +}); diff --git a/plugins/search/src/components/SearchType/SearchType.tsx b/plugins/search/src/components/SearchType/SearchType.tsx new file mode 100644 index 0000000000..6e57879064 --- /dev/null +++ b/plugins/search/src/components/SearchType/SearchType.tsx @@ -0,0 +1,110 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { useSearch } from '../SearchContext'; +import { useEffectOnce } from 'react-use'; +import React, { ChangeEvent } from 'react'; +import { + Chip, + FormControl, + InputLabel, + makeStyles, + MenuItem, + Select, +} from '@material-ui/core'; + +const useStyles = makeStyles({ + label: { + textTransform: 'capitalize', + }, + chips: { + display: 'flex', + flexWrap: 'wrap', + }, + chip: { + margin: 2, + }, +}); + +export type SearchTypeProps = { + className?: string; + name: string; + values?: string[]; + defaultValue?: string[] | string | null; +}; + +const SearchType = ({ + values = [], + className, + name, + defaultValue, +}: SearchTypeProps) => { + const classes = useStyles(); + const { types, setTypes } = useSearch(); + + useEffectOnce(() => { + if (defaultValue && Array.isArray(defaultValue)) { + setTypes(defaultValue); + } else if (defaultValue) { + setTypes([defaultValue]); + } + }); + + const handleChange = (e: ChangeEvent<{ value: unknown }>) => { + const value = e.target.value as string[]; + if (!value || value.includes('*')) { + setTypes([]); + } else { + setTypes(value.filter(it => it !== 'All')); + } + }; + + return ( + + + {name} + + + + ); +}; + +export { SearchType }; diff --git a/plugins/search/src/components/SearchType/index.ts b/plugins/search/src/components/SearchType/index.ts new file mode 100644 index 0000000000..400f3d2a46 --- /dev/null +++ b/plugins/search/src/components/SearchType/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { SearchType } from './SearchType'; diff --git a/plugins/search/src/components/index.tsx b/plugins/search/src/components/index.tsx index 245c31598d..889eb36334 100644 --- a/plugins/search/src/components/index.tsx +++ b/plugins/search/src/components/index.tsx @@ -16,6 +16,7 @@ export * from './Filters'; export * from './SearchFilter'; +export * from './SearchType'; export * from './SearchBar'; export * from './SearchPage'; export * from './SearchResult'; diff --git a/plugins/search/src/index.ts b/plugins/search/src/index.ts index ecd6015b1c..f09ba739bd 100644 --- a/plugins/search/src/index.ts +++ b/plugins/search/src/index.ts @@ -32,6 +32,7 @@ export { useSearch, SearchPage as Router, SearchFilter, + SearchType, SearchFilterNext, SidebarSearch, } from './components'; diff --git a/plugins/techdocs-backend/api-report.md b/plugins/techdocs-backend/api-report.md index 3d1738cb12..f617b796aa 100644 --- a/plugins/techdocs-backend/api-report.md +++ b/plugins/techdocs-backend/api-report.md @@ -3,9 +3,12 @@ > Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/). ```ts +import { CatalogApi } from '@backstage/catalog-client'; import { Config } from '@backstage/config'; +import { DocumentCollator } from '@backstage/search-common'; import express from 'express'; import { GeneratorBuilder } from '@backstage/techdocs-common'; +import { IndexableDocument } from '@backstage/search-common'; import { Knex } from 'knex'; import { Logger as Logger_2 } from 'winston'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; @@ -18,6 +21,54 @@ import { PublisherBase } from '@backstage/techdocs-common'; // @public (undocumented) export function createRouter(options: RouterOptions): Promise; +// Warning: (ae-missing-release-tag) "DefaultTechDocsCollator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export class DefaultTechDocsCollator implements DocumentCollator { + constructor({ + discovery, + locationTemplate, + logger, + catalogClient, + parallelismLimit, + }: { + discovery: PluginEndpointDiscovery; + logger: Logger_2; + locationTemplate?: string; + catalogClient?: CatalogApi; + parallelismLimit?: number; + }); + // (undocumented) + protected applyArgsToFormat( + format: string, + args: Record, + ): string; + // (undocumented) + protected discovery: PluginEndpointDiscovery; + // (undocumented) + execute(): Promise; + // (undocumented) + protected locationTemplate: string; + // (undocumented) + readonly type: string; +} + +// Warning: (ae-missing-release-tag) "TechDocsDocument" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export interface TechDocsDocument extends IndexableDocument { + // (undocumented) + kind: string; + // (undocumented) + lifecycle: string; + // (undocumented) + name: string; + // (undocumented) + namespace: string; + // (undocumented) + owner: string; +} + export * from '@backstage/techdocs-common'; // (No @packageDocumentation comment for this package) diff --git a/plugins/techdocs-backend/package.json b/plugins/techdocs-backend/package.json index 76aadc79cf..24f3f754ca 100644 --- a/plugins/techdocs-backend/package.json +++ b/plugins/techdocs-backend/package.json @@ -35,6 +35,7 @@ "@backstage/catalog-model": "^0.9.0", "@backstage/config": "^0.1.5", "@backstage/errors": "^0.1.1", + "@backstage/search-common": "^0.1.2", "@backstage/techdocs-common": "^0.6.8", "@types/express": "^4.17.6", "cross-fetch": "^3.0.6", @@ -43,10 +44,13 @@ "express-promise-router": "^4.1.0", "fs-extra": "9.1.0", "knex": "^0.95.1", + "lodash": "^4.17.21", + "p-limit": "^3.1.0", "winston": "^3.2.1" }, "devDependencies": { "@backstage/cli": "^0.7.4", + "@backstage/test-utils": "^0.1.14", "@types/dockerode": "^3.2.1", "msw": "^0.29.0", "supertest": "^6.1.3" diff --git a/plugins/techdocs-backend/src/index.ts b/plugins/techdocs-backend/src/index.ts index 6d1e551629..e2a89237db 100644 --- a/plugins/techdocs-backend/src/index.ts +++ b/plugins/techdocs-backend/src/index.ts @@ -15,4 +15,5 @@ */ export { createRouter } from './service/router'; +export * from './search'; export * from '@backstage/techdocs-common'; diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts new file mode 100644 index 0000000000..618bb54d60 --- /dev/null +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.test.ts @@ -0,0 +1,149 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + PluginEndpointDiscovery, + getVoidLogger, +} from '@backstage/backend-common'; +import { Entity } from '@backstage/catalog-model'; +import { DefaultTechDocsCollator } from './DefaultTechDocsCollator'; +import { msw } from '@backstage/test-utils'; +import { setupServer } from 'msw/node'; +import { rest } from 'msw'; + +const logger = getVoidLogger(); + +const mockSearchDocIndex = { + config: { + lang: ['en'], + min_search_length: 3, + prebuild_index: false, + separator: '[\\s\\-]+', + }, + docs: [ + { + location: '', + text: 'docs docs docs', + title: 'Home', + }, + { + location: 'local-development/', + text: 'Docs for first subtitle', + title: 'Local development', + }, + { + location: 'local-development/#development', + text: 'Docs for sub-subtitle', + title: 'Development', + }, + ], +}; + +const expectedEntities: Entity[] = [ + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'test-entity-with-docs', + description: 'Documented description', + annotations: { + 'backstage.io/techdocs-ref': './', + }, + }, + spec: { + type: 'dog', + lifecycle: 'experimental', + owner: 'someone', + }, + }, + { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + name: 'test-entity', + description: 'The expected description', + }, + spec: { + type: 'some-type', + lifecycle: 'experimental', + }, + }, +]; + +describe('DefaultTechDocsCollator', () => { + let mockDiscoveryApi: jest.Mocked; + let collator: DefaultTechDocsCollator; + + const worker = setupServer(); + msw.setupDefaultHandlers(worker); + beforeEach(() => { + mockDiscoveryApi = { + getBaseUrl: jest.fn().mockResolvedValue('http://test-backend'), + getExternalBaseUrl: jest.fn(), + }; + collator = new DefaultTechDocsCollator({ + discovery: mockDiscoveryApi, + logger, + }); + + worker.use( + rest.get( + 'http://test-backend/static/docs/default/Component/test-entity-with-docs/search/search_index.json', + (_, res, ctx) => res(ctx.status(200), ctx.json(mockSearchDocIndex)), + ), + rest.get('http://test-backend/entities', (_, res, ctx) => + res(ctx.status(200), ctx.json(expectedEntities)), + ), + ); + }); + + it('fetches from the configured catalog and tech docs services', async () => { + const documents = await collator.execute(); + expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('catalog'); + expect(mockDiscoveryApi.getBaseUrl).toHaveBeenCalledWith('techdocs'); + expect(documents).toHaveLength(mockSearchDocIndex.docs.length); + }); + + it('should create documents for each tech docs search index', async () => { + const documents = await collator.execute(); + const entity = expectedEntities[0]; + documents.forEach((document, idx) => { + expect(document).toMatchObject({ + title: mockSearchDocIndex.docs[idx].title, + location: `/docs/default/Component/${entity.metadata.name}/${mockSearchDocIndex.docs[idx].location}`, + text: mockSearchDocIndex.docs[idx].text, + namespace: 'default', + componentType: entity!.spec!.type, + lifecycle: entity!.spec!.lifecycle, + owner: '', + }); + }); + }); + + it('maps a returned entity with a custom locationTemplate', async () => { + // Provide an alternate location template. + collator = new DefaultTechDocsCollator({ + discovery: mockDiscoveryApi, + locationTemplate: '/software/:name', + logger, + }); + + const documents = await collator.execute(); + expect(documents[0]).toMatchObject({ + location: '/software/test-entity-with-docs', + }); + }); +}); diff --git a/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts new file mode 100644 index 0000000000..7c176da9fb --- /dev/null +++ b/plugins/techdocs-backend/src/search/DefaultTechDocsCollator.ts @@ -0,0 +1,149 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { Entity, RELATION_OWNED_BY } from '@backstage/catalog-model'; +import { IndexableDocument, DocumentCollator } from '@backstage/search-common'; +import fetch from 'cross-fetch'; +import unescape from 'lodash/unescape'; +import { Logger } from 'winston'; +import pLimit from 'p-limit'; +import { CatalogApi, CatalogClient } from '@backstage/catalog-client'; + +interface MkSearchIndexDoc { + title: string; + text: string; + location: string; +} + +export interface TechDocsDocument extends IndexableDocument { + kind: string; + namespace: string; + name: string; + lifecycle: string; + owner: string; +} + +export class DefaultTechDocsCollator implements DocumentCollator { + protected discovery: PluginEndpointDiscovery; + protected locationTemplate: string; + private readonly logger: Logger; + private readonly catalogClient: CatalogApi; + private readonly parallelismLimit: number; + public readonly type: string = 'techdocs'; + + constructor({ + discovery, + locationTemplate, + logger, + catalogClient, + parallelismLimit = 10, + }: { + discovery: PluginEndpointDiscovery; + logger: Logger; + locationTemplate?: string; + catalogClient?: CatalogApi; + parallelismLimit?: number; + }) { + this.discovery = discovery; + this.locationTemplate = + locationTemplate || '/docs/:namespace/:kind/:name/:path'; + this.logger = logger; + this.catalogClient = + catalogClient || new CatalogClient({ discoveryApi: discovery }); + this.parallelismLimit = parallelismLimit; + } + + async execute() { + const limit = pLimit(this.parallelismLimit); + const techDocsBaseUrl = await this.discovery.getBaseUrl('techdocs'); + const entities = await this.catalogClient.getEntities({ + fields: [ + 'kind', + 'namespace', + 'metadata.annotations', + 'metadata.name', + 'metadata.namespace', + 'spec.type', + 'spec.lifecycle', + 'relations', + ], + }); + const docPromises = entities.items + .filter(it => it.metadata?.annotations?.['backstage.io/techdocs-ref']) + .map((entity: Entity) => + limit( + async (): Promise => { + const entityInfo = { + kind: entity.kind, + namespace: entity.metadata.namespace || 'default', + name: entity.metadata.name, + }; + + try { + const searchIndexResponse = await fetch( + DefaultTechDocsCollator.constructDocsIndexUrl( + techDocsBaseUrl, + entityInfo, + ), + ); + const searchIndex = await searchIndexResponse.json(); + + return searchIndex.docs.map((doc: MkSearchIndexDoc) => ({ + title: unescape(doc.title), + text: unescape(doc.text || ''), + location: this.applyArgsToFormat(this.locationTemplate, { + ...entityInfo, + path: doc.location, + }), + ...entityInfo, + componentType: entity.spec?.type?.toString() || 'other', + lifecycle: (entity.spec?.lifecycle as string) || '', + owner: + entity.relations?.find(r => r.type === RELATION_OWNED_BY) + ?.target?.name || '', + })); + } catch (e) { + this.logger.warn( + `Failed to retrieve tech docs search index for entity ${entityInfo.namespace}/${entityInfo.kind}/${entityInfo.name}`, + e, + ); + return []; + } + }, + ), + ); + return (await Promise.all(docPromises)).flat(); + } + + protected applyArgsToFormat( + format: string, + args: Record, + ): string { + let formatted = format; + for (const [key, value] of Object.entries(args)) { + formatted = formatted.replace(`:${key}`, value); + } + return formatted; + } + + private static constructDocsIndexUrl( + techDocsBaseUrl: string, + entityInfo: { kind: string; namespace: string; name: string }, + ) { + return `${techDocsBaseUrl}/static/docs/${entityInfo.namespace}/${entityInfo.kind}/${entityInfo.name}/search/search_index.json`; + } +} diff --git a/plugins/techdocs-backend/src/search/index.ts b/plugins/techdocs-backend/src/search/index.ts new file mode 100644 index 0000000000..74f348d769 --- /dev/null +++ b/plugins/techdocs-backend/src/search/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +export { DefaultTechDocsCollator } from './DefaultTechDocsCollator'; +export type { TechDocsDocument } from './DefaultTechDocsCollator'; diff --git a/plugins/techdocs/api-report.md b/plugins/techdocs/api-report.md index d63303c134..fdac1a8aab 100644 --- a/plugins/techdocs/api-report.md +++ b/plugins/techdocs/api-report.md @@ -25,6 +25,17 @@ export const DocsCardGrid: ({ entities: Entity[] | undefined; }) => JSX.Element | null; +// Warning: (ae-missing-release-tag) "DocsResultListItem" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export const DocsResultListItem: ({ + result, + lineClamp, +}: { + result: any; + lineClamp?: number | undefined; +}) => JSX.Element; + // Warning: (ae-missing-release-tag) "DocsTable" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 140be7d886..653fc99146 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -51,6 +51,7 @@ "react-router": "6.0.0-beta.0", "react-router-dom": "6.0.0-beta.0", "react-use": "^17.2.4", + "react-text-truncate": "^0.16.0", "sanitize-html": "^2.3.2" }, "devDependencies": { diff --git a/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.test.tsx b/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.test.tsx new file mode 100644 index 0000000000..41de2fefe4 --- /dev/null +++ b/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.test.tsx @@ -0,0 +1,50 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { render } from '@testing-library/react'; +import { DocsResultListItem } from './DocsResultListItem'; + +// Using canvas to render text.. +jest.mock('react-text-truncate', () => { + return ({ text }: { text: string }) => {text}; +}); + +const validResult = { + location: 'https://backstage.io/docs', + title: 'Documentation', + text: + 'Backstage is an open-source developer portal that puts the developer experience first.', + kind: 'library', + namespace: '', + name: 'Backstage', + lifecycle: 'production', +}; + +describe('DocsResultListItem test', () => { + it('should render search doc passed in', async () => { + const { findByText } = render(); + + expect( + await findByText('Documentation | Backstage docs'), + ).toBeInTheDocument(); + expect( + await findByText( + 'Backstage is an open-source developer portal that puts the developer experience first.', + ), + ).toBeInTheDocument(); + }); +}); diff --git a/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.tsx b/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.tsx new file mode 100644 index 0000000000..52aa45a97e --- /dev/null +++ b/plugins/techdocs/src/components/DocsResultListItem/DocsResultListItem.tsx @@ -0,0 +1,60 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Divider, ListItem, ListItemText, makeStyles } from '@material-ui/core'; +import { Link } from '@backstage/core-components'; +import TextTruncate from 'react-text-truncate'; + +const useStyles = makeStyles({ + flexContainer: { + flexWrap: 'wrap', + }, + itemText: { + width: '100%', + marginBottom: '1rem', + }, +}); + +export const DocsResultListItem = ({ + result, + lineClamp = 5, +}: { + result: any; + lineClamp?: number; +}) => { + const classes = useStyles(); + return ( + + + + } + /> + + + + ); +}; diff --git a/plugins/techdocs/src/components/DocsResultListItem/index.ts b/plugins/techdocs/src/components/DocsResultListItem/index.ts new file mode 100644 index 0000000000..4e1e511ea1 --- /dev/null +++ b/plugins/techdocs/src/components/DocsResultListItem/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { DocsResultListItem } from './DocsResultListItem'; diff --git a/plugins/techdocs/src/index.ts b/plugins/techdocs/src/index.ts index d645d128f7..66502a4585 100644 --- a/plugins/techdocs/src/index.ts +++ b/plugins/techdocs/src/index.ts @@ -19,6 +19,7 @@ export { techdocsApiRef, techdocsStorageApiRef } from './api'; export type { TechDocsApi, TechDocsStorageApi } from './api'; export { TechDocsClient, TechDocsStorageClient } from './client'; export type { PanelType } from './home/components/TechDocsCustomHome'; +export * from './components/DocsResultListItem'; export { DocsCardGrid, DocsTable, From 378cc6a54bdb3f4f149201c32b018d7f93f3b560 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 15 Jul 2021 11:22:31 +0200 Subject: [PATCH 02/29] Only update the path when the content is updated Signed-off-by: Dominik Henneke --- .changeset/techdocs-fifty-cameras-listen.md | 5 + .../techdocs/src/reader/components/Reader.tsx | 4 +- .../reader/components/useReaderState.test.tsx | 160 +++++++++++++++--- .../src/reader/components/useReaderState.ts | 26 ++- 4 files changed, 157 insertions(+), 38 deletions(-) create mode 100644 .changeset/techdocs-fifty-cameras-listen.md diff --git a/.changeset/techdocs-fifty-cameras-listen.md b/.changeset/techdocs-fifty-cameras-listen.md new file mode 100644 index 0000000000..9071004acd --- /dev/null +++ b/.changeset/techdocs-fifty-cameras-listen.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Only update the `path` when the content is updated. diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index d63ddf8125..76349784e7 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -47,17 +47,17 @@ type Props = { export const Reader = ({ entityId, onReady }: Props) => { const { kind, namespace, name } = entityId; - const { '*': path } = useParams(); const theme = useTheme(); const { state, + path, contentReload, content: rawPage, contentErrorMessage, syncErrorMessage, buildLog, - } = useReaderState(kind, namespace, name, path); + } = useReaderState(kind, namespace, name, useParams()['*']); const techdocsStorageApi = useApi(techdocsStorageApiRef); const [sidebars, setSidebars] = useState(); diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 9d9a8e2b1a..ebc9277096 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -86,7 +86,7 @@ describe('useReaderState', () => { }; it('should return a copy of the state', () => { - expect(reducer(oldState, { type: 'navigate', path: '/' })).toEqual({ + expect(reducer(oldState, { type: 'content', path: '/' })).toEqual({ activeSyncState: 'CHECKING', contentLoading: false, path: '/', @@ -102,13 +102,11 @@ describe('useReaderState', () => { }); it.each` - type | oldActiveSyncState | newActiveSyncState - ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'navigate'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'sync'} | ${'BUILD_READY'} | ${undefined} - ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} + type | oldActiveSyncState | newActiveSyncState + ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'sync'} | ${'BUILD_READY'} | ${undefined} + ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} `( 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', ({ type, oldActiveSyncState, newActiveSyncState }) => { @@ -164,6 +162,27 @@ describe('useReaderState', () => { }); }); + it('should set content and update path', () => { + expect( + reducer( + { + ...oldState, + contentLoading: true, + }, + { + type: 'content', + content: 'asdf', + path: '/new-path', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: false, + content: 'asdf', + path: '/new-path', + }); + }); + it('should set error', () => { expect( reducer( @@ -185,20 +204,6 @@ describe('useReaderState', () => { }); }); - describe('"navigate" action', () => { - it('should work', () => { - expect( - reducer(oldState, { - type: 'navigate', - path: '/', - }), - ).toEqual({ - ...oldState, - path: '/', - }); - }); - }); - describe('"sync" action', () => { it('should update state', () => { expect( @@ -256,6 +261,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -267,6 +273,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -313,6 +320,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -324,6 +332,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'INITIAL_BUILD', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Page Not Found', syncErrorMessage: undefined, @@ -335,6 +344,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -346,6 +356,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -394,6 +405,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -405,6 +417,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -416,6 +429,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_REFRESHING', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -427,6 +441,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_READY', + path: '/example', content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -441,6 +456,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -452,6 +468,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_FRESH', + path: '/example', content: 'my new content', contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -475,6 +492,103 @@ describe('useReaderState', () => { }); }); + it('should handle navigation', async () => { + techdocsStorageApi.getEntityDocs + .mockResolvedValueOnce('my content') + .mockImplementationOnce(async () => { + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'my new content'; + }) + .mockRejectedValueOnce(new NotFoundError('Some error description')); + techdocsStorageApi.syncEntityDocs.mockResolvedValue('cached'); + + await act(async () => { + const { result, waitForValueToChange, rerender } = await renderHook( + ({ path }: { path: string }) => + useReaderState('Component', 'default', 'backstage', path), + { initialProps: { path: '/example' }, wrapper: Wrapper as any }, + ); + + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: undefined, + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // show the content + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/example', + content: 'my content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/new' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CHECKING', + path: '/example', + content: undefined, + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + path: '/new', + content: 'my new content', + contentErrorMessage: undefined, + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + // navigate + rerender({ path: '/missing' }); + + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_NOT_FOUND', + path: '/missing', + content: undefined, + contentErrorMessage: 'NotFoundError: Some error description', + syncErrorMessage: undefined, + buildLog: [], + contentReload: expect.any(Function), + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/example', + ); + expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( + { kind: 'Component', namespace: 'default', name: 'backstage' }, + '/new', + ); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); + }); + }); + it('should handle content error', async () => { techdocsStorageApi.getEntityDocs.mockRejectedValue( new NotFoundError('Some error description'), @@ -489,6 +603,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', + path: '/example', content: undefined, contentErrorMessage: undefined, syncErrorMessage: undefined, @@ -500,6 +615,7 @@ describe('useReaderState', () => { await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_NOT_FOUND', + path: '/example', content: undefined, contentErrorMessage: 'NotFoundError: Some error description', syncErrorMessage: undefined, diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index 178c0746bf..fabcfa2687 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -15,7 +15,7 @@ */ import { useApi } from '@backstage/core-plugin-api'; -import { useEffect, useMemo, useReducer, useRef } from 'react'; +import { useMemo, useReducer, useRef } from 'react'; import { useAsync, useAsyncRetry } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; @@ -133,11 +133,11 @@ type ReducerActions = } | { type: 'content'; + path?: string; content?: string; contentLoading?: true; contentError?: Error; } - | { type: 'navigate'; path: string } | { type: 'buildLog'; log: string }; type ReducerState = { @@ -187,15 +187,15 @@ export function reducer( break; case 'content': + if (typeof action.path === 'string') { + newState.path = action.path; + } + newState.content = action.content; newState.contentLoading = action.contentLoading ?? false; newState.contentError = action.contentError; break; - case 'navigate': - newState.path = action.path; - break; - case 'buildLog': newState.buildLog = newState.buildLog.concat(action.log); break; @@ -207,7 +207,7 @@ export function reducer( // a navigation or a content update loads fresh content so the build is updated to being up-to-date if ( ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && - ['content', 'navigate'].includes(action.type) + ['content'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; newState.buildLog = []; @@ -223,6 +223,7 @@ export function useReaderState( path: string, ): { state: ContentStateTypes; + path: string; contentReload: () => void; content?: string; contentErrorMessage?: string; @@ -238,11 +239,6 @@ export function useReaderState( const techdocsStorageApi = useApi(techdocsStorageApiRef); - // convert all path changes into actions - useEffect(() => { - dispatch({ type: 'navigate', path }); - }, [path]); - // try to load the content. the function will fire events and we don't care for the return values const { retry: contentReload } = useAsyncRetry(async () => { dispatch({ type: 'content', contentLoading: true }); @@ -253,11 +249,12 @@ export function useReaderState( path, ); - dispatch({ type: 'content', content: entityDocs }); + // update content and path at the same time + dispatch({ type: 'content', content: entityDocs, path }); return entityDocs; } catch (e) { - dispatch({ type: 'content', contentError: e }); + dispatch({ type: 'content', contentError: e, path }); } return undefined; @@ -335,6 +332,7 @@ export function useReaderState( return { state: displayState, contentReload, + path: state.path, content: state.content, contentErrorMessage: state.contentError?.toString(), syncErrorMessage: state.syncError?.toString(), From 3d3c43f0a70337fa6db69da2387088f2acce4705 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Thu, 15 Jul 2021 14:05:51 +0200 Subject: [PATCH 03/29] Create a dedicated `contentLoading` action that keeps old content This change makes sure that the content is removed when an error occurs such as a link to a missing page. It also stills shows the old content while the new one is loaded. The (delayed) loading indicator still shows that new content is loaded. Signed-off-by: Dominik Henneke --- .../techdocs/src/reader/components/Reader.tsx | 4 ++ .../reader/components/useReaderState.test.tsx | 51 +++++++++++++++---- .../src/reader/components/useReaderState.ts | 18 +++++-- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index 76349784e7..f981394036 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -96,6 +96,10 @@ export const Reader = ({ entityId, onReady }: Props) => { useEffect(() => { if (!rawPage || !shadowDomRef.current) { + // clear the shadow dom if no content is available + if (shadowDomRef.current?.shadowRoot) { + shadowDomRef.current.shadowRoot.innerHTML = ''; + } return; } if (onReady) { diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index ebc9277096..b0d4e0326a 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -102,11 +102,13 @@ describe('useReaderState', () => { }); it.each` - type | oldActiveSyncState | newActiveSyncState - ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} - ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} - ${'sync'} | ${'BUILD_READY'} | ${undefined} - ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined} + type | oldActiveSyncState | newActiveSyncState + ${'contentLoading'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'contentLoading'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY'} | ${'UP_TO_DATE'} + ${'content'} | ${'BUILD_READY_RELOAD'} | ${'UP_TO_DATE'} + ${'sync'} | ${'BUILD_READY'} | ${undefined /* undefined, because we don't set an input */} + ${'sync'} | ${'BUILD_READY_RELOAD'} | ${undefined /* undefined, because we don't set an input */} `( 'should, when type=$type and activeSyncState=$oldActiveSyncState, set activeSyncState=$newActiveSyncState', ({ type, oldActiveSyncState, newActiveSyncState }) => { @@ -122,18 +124,45 @@ describe('useReaderState', () => { }, ); - describe('"content" action', () => { + describe('"contentLoading" action', () => { it('should set loading', () => { + expect( + reducer(oldState, { + type: 'contentLoading', + }), + ).toEqual({ + ...oldState, + contentLoading: true, + }); + }); + + it('should keep content', () => { expect( reducer( { ...oldState, content: 'some-old-content', + }, + { + type: 'contentLoading', + }, + ), + ).toEqual({ + ...oldState, + contentLoading: true, + content: 'some-old-content', + }); + }); + + it('should reset errors', () => { + expect( + reducer( + { + ...oldState, contentError: new Error(), }, { - type: 'content', - contentLoading: true, + type: 'contentLoading', }, ), ).toEqual({ @@ -141,7 +170,9 @@ describe('useReaderState', () => { contentLoading: true, }); }); + }); + describe('"content" action', () => { it('should set content', () => { expect( reducer( @@ -457,7 +488,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', path: '/example', - content: undefined, + content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, buildLog: [], @@ -538,7 +569,7 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', path: '/example', - content: undefined, + content: 'my content', contentErrorMessage: undefined, syncErrorMessage: undefined, buildLog: [], diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index fabcfa2687..07bd338de9 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -131,11 +131,11 @@ type ReducerActions = state: SyncStates; syncError?: Error; } + | { type: 'contentLoading' } | { type: 'content'; path?: string; content?: string; - contentLoading?: true; contentError?: Error; } | { type: 'buildLog'; log: string }; @@ -186,13 +186,21 @@ export function reducer( newState.syncError = action.syncError; break; + case 'contentLoading': + newState.contentLoading = true; + + // only reset errors but keep the old content until it is replaced by the 'content' action + newState.contentError = undefined; + break; + case 'content': + // only override the path if it is part of the action if (typeof action.path === 'string') { newState.path = action.path; } + newState.contentLoading = false; newState.content = action.content; - newState.contentLoading = action.contentLoading ?? false; newState.contentError = action.contentError; break; @@ -204,10 +212,10 @@ export function reducer( throw new Error(); } - // a navigation or a content update loads fresh content so the build is updated to being up-to-date + // a content update loads fresh content so the build is updated to being up-to-date if ( ['BUILD_READY', 'BUILD_READY_RELOAD'].includes(newState.activeSyncState) && - ['content'].includes(action.type) + ['contentLoading', 'content'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; newState.buildLog = []; @@ -241,7 +249,7 @@ export function useReaderState( // try to load the content. the function will fire events and we don't care for the return values const { retry: contentReload } = useAsyncRetry(async () => { - dispatch({ type: 'content', contentLoading: true }); + dispatch({ type: 'contentLoading' }); try { const entityDocs = await techdocsStorageApi.getEntityDocs( From 214e7c52d14c02c0dccd6e4f623066c103e74619 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 7 Jul 2021 10:43:54 +0200 Subject: [PATCH 04/29] Refactor the techdocs transformers to return `Promise`s and await all transformations Signed-off-by: Dominik Henneke --- .changeset/techdocs-spotty-tables-beam.md | 5 ++ .../techdocs/src/reader/components/Reader.tsx | 12 ++-- .../reader/transformers/addBaseUrl.test.ts | 35 ++++++------ .../src/reader/transformers/addBaseUrl.ts | 30 +++++----- .../transformers/addGitFeedbackLink.test.ts | 20 +++---- .../transformers/addLinkClickListener.test.ts | 8 +-- .../src/reader/transformers/index.test.ts | 4 +- .../src/reader/transformers/injectCss.test.ts | 6 +- .../reader/transformers/onCssReady.test.ts | 21 ++++--- .../src/reader/transformers/onCssReady.ts | 6 +- .../transformers/removeMkdocsHeader.test.ts | 26 +++++---- .../transformers/rewriteDocLinks.test.ts | 12 ++-- .../transformers/sanitizeDOM/index.test.ts | 55 +++++++++++-------- .../transformers/simplifyMkdocsFooter.test.ts | 26 +++++---- .../src/reader/transformers/transformer.ts | 12 ++-- plugins/techdocs/src/test-utils/shadowDom.ts | 8 +-- 16 files changed, 159 insertions(+), 127 deletions(-) create mode 100644 .changeset/techdocs-spotty-tables-beam.md diff --git a/.changeset/techdocs-spotty-tables-beam.md b/.changeset/techdocs-spotty-tables-beam.md new file mode 100644 index 0000000000..397817b196 --- /dev/null +++ b/.changeset/techdocs-spotty-tables-beam.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Refactor the techdocs transformers to return `Promise`s and await all transformations. diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index d63ddf8125..535419dc10 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -23,6 +23,7 @@ import { Button, CircularProgress, useTheme } from '@material-ui/core'; import { Alert } from '@material-ui/lab'; import React, { useCallback, useEffect, useRef, useState } from 'react'; import { useNavigate, useParams } from 'react-router-dom'; +import { useAsync } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; import { addBaseUrl, @@ -94,15 +95,16 @@ export const Reader = ({ entityId, onReady }: Props) => { // an update to "state" might lead to an updated UI so we include it as a trigger }, [updateSidebarPosition, state]); - useEffect(() => { + useAsync(async () => { if (!rawPage || !shadowDomRef.current) { return; } if (onReady) { onReady(); } + // Pre-render - const transformedElement = transformer(rawPage, [ + const transformedElement = await transformer(rawPage, [ sanitizeDOM(), addBaseUrl({ techdocsStorageApi, @@ -236,7 +238,7 @@ export const Reader = ({ entityId, onReady }: Props) => { }), ]); - if (!transformedElement) { + if (!transformedElement?.innerHTML) { return; // An unexpected error occurred } @@ -252,7 +254,7 @@ export const Reader = ({ entityId, onReady }: Props) => { window.scroll({ top: 0 }); // Post-render - transformer(shadowRoot.children[0], [ + await transformer(shadowRoot.children[0], [ dom => { setTimeout(() => { // Scoll to the desired anchor on initial navigation @@ -281,7 +283,7 @@ export const Reader = ({ entityId, onReady }: Props) => { }, }), onCssReady({ - docStorageUrl: techdocsStorageApi.getApiOrigin(), + docStorageUrl: await techdocsStorageApi.getApiOrigin(), onLoading: (dom: Element) => { (dom as HTMLElement).style.setProperty('opacity', '0'); }, diff --git a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts index f04c9963f0..11e8375420 100644 --- a/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts +++ b/plugins/techdocs/src/reader/transformers/addBaseUrl.test.ts @@ -15,9 +15,9 @@ */ import { waitFor } from '@testing-library/react'; -import { createTestShadowDom } from '../../test-utils'; -import { addBaseUrl } from '../transformers'; import { TechDocsStorageApi } from '../../api'; +import { createTestShadowDom } from '../../test-utils'; +import { addBaseUrl } from './addBaseUrl'; const DOC_STORAGE_URL = 'https://example-host.storage.googleapis.com'; const API_ORIGIN_URL = 'https://backstage.example.com/api/techdocs'; @@ -62,8 +62,8 @@ describe('addBaseUrl', () => { global.fetch = originalFetch; }); - it('contains relative paths', () => { - createTestShadowDom(fixture, { + it('contains relative paths', async () => { + await createTestShadowDom(fixture, { preTransformers: [ addBaseUrl({ techdocsStorageApi, @@ -110,7 +110,7 @@ describe('addBaseUrl', () => { text: jest.fn().mockResolvedValue(svgContent), }); - const root = createTestShadowDom('', { + const root = await createTestShadowDom('', { preTransformers: [ addBaseUrl({ techdocsStorageApi, @@ -137,7 +137,7 @@ describe('addBaseUrl', () => { text: jest.fn().mockResolvedValue(svgContent), }); - const root = createTestShadowDom( + const root = await createTestShadowDom( ``, { preTransformers: [ @@ -162,16 +162,19 @@ describe('addBaseUrl', () => { it('does not inline external svgs', async () => { const expectedSrc = 'https://example.com/test.svg'; - const root = createTestShadowDom(``, { - preTransformers: [ - addBaseUrl({ - techdocsStorageApi, - entityId: mockEntityId, - path: '', - }), - ], - postTransformers: [], - }); + const root = await createTestShadowDom( + ``, + { + preTransformers: [ + addBaseUrl({ + techdocsStorageApi, + entityId: mockEntityId, + path: '', + }), + ], + postTransformers: [], + }, + ); await new Promise(done => { process.nextTick(() => { diff --git a/plugins/techdocs/src/reader/transformers/addBaseUrl.ts b/plugins/techdocs/src/reader/transformers/addBaseUrl.ts index f21cb8c47a..8f08ec83a6 100644 --- a/plugins/techdocs/src/reader/transformers/addBaseUrl.ts +++ b/plugins/techdocs/src/reader/transformers/addBaseUrl.ts @@ -14,8 +14,8 @@ * limitations under the License. */ import { EntityName } from '@backstage/catalog-model'; -import type { Transformer } from './transformer'; import { TechDocsStorageApi } from '../../api'; +import type { Transformer } from './transformer'; type AddBaseUrlOptions = { techdocsStorageApi: TechDocsStorageApi; @@ -44,14 +44,15 @@ export const addBaseUrl = ({ entityId, path, }: AddBaseUrlOptions): Transformer => { - return dom => { - const updateDom = ( + return async dom => { + const apiOrigin = await techdocsStorageApi.getApiOrigin(); + + const updateDom = async ( list: HTMLCollectionOf | NodeListOf, attributeName: string, - ): void => { - Array.from(list) - .filter(elem => !!elem.getAttribute(attributeName)) - .forEach(async (elem: T) => { + ) => { + for (const elem of list) { + if (elem.hasAttribute(attributeName)) { const elemAttribute = elem.getAttribute(attributeName); if (!elemAttribute) return; @@ -61,7 +62,7 @@ export const addBaseUrl = ({ entityId, path, ); - const apiOrigin = await techdocsStorageApi.getApiOrigin(); + if (isSvgNeedingInlining(attributeName, elemAttribute, apiOrigin)) { try { const svg = await fetch(newValue, { credentials: 'include' }); @@ -76,13 +77,16 @@ export const addBaseUrl = ({ } else { elem.setAttribute(attributeName, newValue); } - }); + } + } }; - updateDom(dom.querySelectorAll('img'), 'src'); - updateDom(dom.querySelectorAll('script'), 'src'); - updateDom(dom.querySelectorAll('link'), 'href'); - updateDom(dom.querySelectorAll('a[download]'), 'href'); + await Promise.all([ + updateDom(dom.querySelectorAll('img'), 'src'), + updateDom(dom.querySelectorAll('script'), 'src'), + updateDom(dom.querySelectorAll('link'), 'href'), + updateDom(dom.querySelectorAll('a[download]'), 'href'), + ]); return dom; }; diff --git a/plugins/techdocs/src/reader/transformers/addGitFeedbackLink.test.ts b/plugins/techdocs/src/reader/transformers/addGitFeedbackLink.test.ts index 2165afac98..0b8e431781 100644 --- a/plugins/techdocs/src/reader/transformers/addGitFeedbackLink.test.ts +++ b/plugins/techdocs/src/reader/transformers/addGitFeedbackLink.test.ts @@ -28,8 +28,8 @@ const integrations = ScmIntegrations.fromConfig( ); describe('addGitFeedbackLink', () => { - it('adds a feedback link when a Gitlab source edit link is available', () => { - const shadowDom = createTestShadowDom( + it('adds a feedback link when a Gitlab source edit link is available', async () => { + const shadowDom = await createTestShadowDom( ` @@ -53,8 +53,8 @@ describe('addGitFeedbackLink', () => { ); }); - it('adds a feedback link when a Github source edit link is available', () => { - const shadowDom = createTestShadowDom( + it('adds a feedback link when a Github source edit link is available', async () => { + const shadowDom = await createTestShadowDom( ` @@ -78,8 +78,8 @@ describe('addGitFeedbackLink', () => { ); }); - it('does not add a feedback link when no source edit link is available', () => { - const shadowDom = createTestShadowDom( + it('does not add a feedback link when no source edit link is available', async () => { + const shadowDom = await createTestShadowDom( ` @@ -97,8 +97,8 @@ describe('addGitFeedbackLink', () => { expect(shadowDom.querySelector('#git-feedback-link')).toBeFalsy(); }); - it('does not add a feedback link when a Gitlab or Github source edit link is not available', () => { - const shadowDom = createTestShadowDom( + it('does not add a feedback link when a Gitlab or Github source edit link is not available', async () => { + const shadowDom = await createTestShadowDom( ` @@ -117,8 +117,8 @@ describe('addGitFeedbackLink', () => { expect(shadowDom.querySelector('#git-feedback-link')).toBeFalsy(); }); - it('adds a feedback link when a Gitlab or Github source edit link is not available but hostname matches an integrations host', () => { - const shadowDom = createTestShadowDom( + it('adds a feedback link when a Gitlab or Github source edit link is not available but hostname matches an integrations host', async () => { + const shadowDom = await createTestShadowDom( ` diff --git a/plugins/techdocs/src/reader/transformers/addLinkClickListener.test.ts b/plugins/techdocs/src/reader/transformers/addLinkClickListener.test.ts index 2d2d54a31c..fe3a6e557c 100644 --- a/plugins/techdocs/src/reader/transformers/addLinkClickListener.test.ts +++ b/plugins/techdocs/src/reader/transformers/addLinkClickListener.test.ts @@ -18,9 +18,9 @@ import { createTestShadowDom } from '../../test-utils'; import { addLinkClickListener } from './addLinkClickListener'; describe('addLinkClickListener', () => { - it('calls onClick when a link has been clicked', () => { + it('calls onClick when a link has been clicked', async () => { const fn = jest.fn(); - const shadowDom = createTestShadowDom( + const shadowDom = await createTestShadowDom( ` @@ -45,9 +45,9 @@ describe('addLinkClickListener', () => { expect(fn).toHaveBeenCalledTimes(1); }); - it('does not call onClick when a link links to another baseUrl', () => { + it('does not call onClick when a link links to another baseUrl', async () => { const fn = jest.fn(); - const shadowDom = createTestShadowDom( + const shadowDom = await createTestShadowDom( ` diff --git a/plugins/techdocs/src/reader/transformers/index.test.ts b/plugins/techdocs/src/reader/transformers/index.test.ts index 30607aedcd..16b42266df 100644 --- a/plugins/techdocs/src/reader/transformers/index.test.ts +++ b/plugins/techdocs/src/reader/transformers/index.test.ts @@ -17,14 +17,14 @@ import { Transformer, transform } from './transformer'; describe('transform', () => { - it('calls the transformers', () => { + it('calls the transformers', async () => { const fn = jest.fn(); const mockTransformer = (): Transformer => (dom: Element) => { fn(dom); return dom; }; - transform('', [mockTransformer()]); + await transform('', [mockTransformer()]); expect(fn).toHaveBeenCalledTimes(1); expect(fn).toHaveBeenCalledWith(expect.any(Element)); diff --git a/plugins/techdocs/src/reader/transformers/injectCss.test.ts b/plugins/techdocs/src/reader/transformers/injectCss.test.ts index 368d077b43..6d0eb8daa9 100644 --- a/plugins/techdocs/src/reader/transformers/injectCss.test.ts +++ b/plugins/techdocs/src/reader/transformers/injectCss.test.ts @@ -15,10 +15,10 @@ */ import { createTestShadowDom } from '../../test-utils'; -import { injectCss } from '../transformers'; +import { injectCss } from './injectCss'; describe('injectCss', () => { - it('should inject style with passed css in head', () => { + it('should inject style with passed css in head', async () => { const html = ` @@ -27,7 +27,7 @@ describe('injectCss', () => { `; const injectedCss = '* {background-color: #fff}'; - const shadowDom = createTestShadowDom(html, { + const shadowDom = await createTestShadowDom(html, { preTransformers: [injectCss({ css: injectedCss })], postTransformers: [], }); diff --git a/plugins/techdocs/src/reader/transformers/onCssReady.test.ts b/plugins/techdocs/src/reader/transformers/onCssReady.test.ts index 3174e23f5c..b2a5aa9760 100644 --- a/plugins/techdocs/src/reader/transformers/onCssReady.test.ts +++ b/plugins/techdocs/src/reader/transformers/onCssReady.test.ts @@ -15,16 +15,15 @@ */ import { - createTestShadowDom, - mockStylesheetEventListener, - executeStylesheetEventListeners, clearStylesheetEventListeners, + createTestShadowDom, + executeStylesheetEventListeners, + mockStylesheetEventListener, } from '../../test-utils'; -import { onCssReady } from '../transformers'; +import { onCssReady } from './onCssReady'; -const docStorageUrl: Promise = Promise.resolve( - 'https://techdocs-mock-sites.storage.googleapis.com', -); +const docStorageUrl: string = + 'https://techdocs-mock-sites.storage.googleapis.com'; const fixture = ` @@ -48,11 +47,11 @@ describe('onCssReady', () => { clearStylesheetEventListeners(); }); - it('does not call onLoading and onLoaded without the onCssReady transformer', () => { + it('does not call onLoading and onLoaded without the onCssReady transformer', async () => { const onLoading = jest.fn(); const onLoaded = jest.fn(); - createTestShadowDom(fixture, { + await createTestShadowDom(fixture, { preTransformers: [], postTransformers: [], }); @@ -62,11 +61,11 @@ describe('onCssReady', () => { expect(onLoaded).not.toHaveBeenCalled(); }); - it('calls the onLoading and onLoaded correctly', () => { + it('calls the onLoading and onLoaded correctly', async () => { const onLoading = jest.fn(); const onLoaded = jest.fn(); - createTestShadowDom(fixture, { + await createTestShadowDom(fixture, { preTransformers: [], postTransformers: [ onCssReady({ diff --git a/plugins/techdocs/src/reader/transformers/onCssReady.ts b/plugins/techdocs/src/reader/transformers/onCssReady.ts index 936c4306a7..e9406ba632 100644 --- a/plugins/techdocs/src/reader/transformers/onCssReady.ts +++ b/plugins/techdocs/src/reader/transformers/onCssReady.ts @@ -17,7 +17,7 @@ import type { Transformer } from './transformer'; type OnCssReadyOptions = { - docStorageUrl: Promise; + docStorageUrl: string; onLoading: (dom: Element) => void; onLoaded: (dom: Element) => void; }; @@ -30,9 +30,7 @@ export const onCssReady = ({ return dom => { const cssPages = Array.from( dom.querySelectorAll('head > link[rel="stylesheet"]'), - ).filter(async elem => - elem.getAttribute('href')?.startsWith(await docStorageUrl), - ); + ).filter(elem => elem.getAttribute('href')?.startsWith(docStorageUrl)); let count = cssPages.length; diff --git a/plugins/techdocs/src/reader/transformers/removeMkdocsHeader.test.ts b/plugins/techdocs/src/reader/transformers/removeMkdocsHeader.test.ts index 70d6b2fa4a..60f0b9ca30 100644 --- a/plugins/techdocs/src/reader/transformers/removeMkdocsHeader.test.ts +++ b/plugins/techdocs/src/reader/transformers/removeMkdocsHeader.test.ts @@ -18,20 +18,26 @@ import { createTestShadowDom, FIXTURES } from '../../test-utils'; import { removeMkdocsHeader } from '../transformers'; describe('removeMkdocsHeader', () => { - it('does not remove mkdocs header', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [], - postTransformers: [], - }); + it('does not remove mkdocs header', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [], + postTransformers: [], + }, + ); expect(shadowDom.querySelector('.md-header')).toBeTruthy(); }); - it('does remove mkdocs header', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [removeMkdocsHeader()], - postTransformers: [], - }); + it('does remove mkdocs header', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [removeMkdocsHeader()], + postTransformers: [], + }, + ); expect(shadowDom.querySelector('.md-header')).toBeFalsy(); }); diff --git a/plugins/techdocs/src/reader/transformers/rewriteDocLinks.test.ts b/plugins/techdocs/src/reader/transformers/rewriteDocLinks.test.ts index 62b80cd514..66fc5f29c1 100644 --- a/plugins/techdocs/src/reader/transformers/rewriteDocLinks.test.ts +++ b/plugins/techdocs/src/reader/transformers/rewriteDocLinks.test.ts @@ -19,8 +19,8 @@ import { rewriteDocLinks } from '../transformers'; import { normalizeUrl } from './rewriteDocLinks'; describe('rewriteDocLinks', () => { - it('should not do anything', () => { - const shadowDom = createTestShadowDom(` + it('should not do anything', async () => { + const shadowDom = await createTestShadowDom(` Test Test Test @@ -35,8 +35,8 @@ describe('rewriteDocLinks', () => { ]); }); - it('should transform a href with localhost as baseUrl', () => { - const shadowDom = createTestShadowDom( + it('should transform a href with localhost as baseUrl', async () => { + const shadowDom = await createTestShadowDom( ` Test Test @@ -57,9 +57,9 @@ describe('rewriteDocLinks', () => { ]); }); - it('should rewrite non-parseable URLs as text', () => { + it('should rewrite non-parseable URLs as text', async () => { const expectedText = `www.my-internet.[top-level-domain]/pathname/[URLkey]`; - const shadowDom = createTestShadowDom( + const shadowDom = await createTestShadowDom( `${expectedText}`, { preTransformers: [rewriteDocLinks()], diff --git a/plugins/techdocs/src/reader/transformers/sanitizeDOM/index.test.ts b/plugins/techdocs/src/reader/transformers/sanitizeDOM/index.test.ts index 8456cf7c23..6296793e2a 100644 --- a/plugins/techdocs/src/reader/transformers/sanitizeDOM/index.test.ts +++ b/plugins/techdocs/src/reader/transformers/sanitizeDOM/index.test.ts @@ -16,7 +16,7 @@ import { createTestShadowDom, FIXTURES } from '../../../test-utils'; import { Transformer } from '../index'; -import { sanitizeDOM } from '../sanitizeDOM'; +import { sanitizeDOM } from './index'; const injectMaliciousLink = (): Transformer => dom => { const link = document.createElement('a'); @@ -27,55 +27,64 @@ const injectMaliciousLink = (): Transformer => dom => { }; describe('sanitizeDOM', () => { - it('contains a script tag', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE); + it('contains a script tag', async () => { + const shadowDom = await createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE); expect(shadowDom.querySelectorAll('script').length).toBeGreaterThan(0); }); - it('does not contain a script tag', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [sanitizeDOM()], - postTransformers: [], - }); + it('does not contain a script tag', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [sanitizeDOM()], + postTransformers: [], + }, + ); expect(shadowDom.querySelectorAll('script').length).toBe(0); }); - it('contains link with a onClick attribute', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [injectMaliciousLink()], - postTransformers: [], - }); + it('contains link with a onClick attribute', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [injectMaliciousLink()], + postTransformers: [], + }, + ); expect( shadowDom.querySelector('#test-malicious-link')?.hasAttribute('onclick'), ).toBeTruthy(); }); - it('does not contain link with a onClick attribute', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [sanitizeDOM()], - postTransformers: [], - }); + it('does not contain link with a onClick attribute', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [sanitizeDOM()], + postTransformers: [], + }, + ); expect( shadowDom.querySelector('#test-malicious-link')?.hasAttribute('onclick'), ).toBeFalsy(); }); - it('removes style tags', () => { + it('removes style tags', async () => { const html = ` - `; - const shadowDom = createTestShadowDom(html, { + const shadowDom = await createTestShadowDom(html, { preTransformers: [sanitizeDOM()], postTransformers: [], }); @@ -83,7 +92,7 @@ describe('sanitizeDOM', () => { expect(shadowDom.querySelectorAll('style').length).toEqual(0); }); - it('does not remove link tags', () => { + it('does not remove link tags', async () => { const html = ` @@ -94,7 +103,7 @@ describe('sanitizeDOM', () => { `; - const shadowDom = createTestShadowDom(html, { + const shadowDom = await createTestShadowDom(html, { preTransformers: [sanitizeDOM()], postTransformers: [], }); diff --git a/plugins/techdocs/src/reader/transformers/simplifyMkdocsFooter.test.ts b/plugins/techdocs/src/reader/transformers/simplifyMkdocsFooter.test.ts index dbc3761e80..020befd522 100644 --- a/plugins/techdocs/src/reader/transformers/simplifyMkdocsFooter.test.ts +++ b/plugins/techdocs/src/reader/transformers/simplifyMkdocsFooter.test.ts @@ -18,20 +18,26 @@ import { createTestShadowDom, FIXTURES } from '../../test-utils'; import { simplifyMkdocsFooter } from './simplifyMkdocsFooter'; describe('simplifyMkdocsFooter', () => { - it('does not remove mkdocs copyright', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [], - postTransformers: [], - }); + it('does not remove mkdocs copyright', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [], + postTransformers: [], + }, + ); expect(shadowDom.querySelector('.md-footer-copyright')).toBeTruthy(); }); - it('does remove mkdocs copyright', () => { - const shadowDom = createTestShadowDom(FIXTURES.FIXTURE_STANDARD_PAGE, { - preTransformers: [simplifyMkdocsFooter()], - postTransformers: [], - }); + it('does remove mkdocs copyright', async () => { + const shadowDom = await createTestShadowDom( + FIXTURES.FIXTURE_STANDARD_PAGE, + { + preTransformers: [simplifyMkdocsFooter()], + postTransformers: [], + }, + ); expect(shadowDom.querySelector('.md-footer-copyright')).toBeFalsy(); }); diff --git a/plugins/techdocs/src/reader/transformers/transformer.ts b/plugins/techdocs/src/reader/transformers/transformer.ts index 7b440befbf..fc52f42b5c 100644 --- a/plugins/techdocs/src/reader/transformers/transformer.ts +++ b/plugins/techdocs/src/reader/transformers/transformer.ts @@ -14,12 +14,12 @@ * limitations under the License. */ -export type Transformer = (dom: Element) => Element; +export type Transformer = (dom: Element) => Element | Promise; -export const transform = ( +export const transform = async ( html: string | Element, transformers: Transformer[], -): Element => { +): Promise => { let dom: Element; if (typeof html === 'string') { @@ -30,9 +30,9 @@ export const transform = ( throw new Error('dom is not a recognized type'); } - transformers.forEach(transformer => { - dom = transformer(dom); - }); + for (const transformer of transformers) { + dom = await transformer(dom); + } return dom; }; diff --git a/plugins/techdocs/src/test-utils/shadowDom.ts b/plugins/techdocs/src/test-utils/shadowDom.ts index 61055a87bd..f8289bf6d9 100644 --- a/plugins/techdocs/src/test-utils/shadowDom.ts +++ b/plugins/techdocs/src/test-utils/shadowDom.ts @@ -22,13 +22,13 @@ export type CreateTestShadowDomOptions = { postTransformers: Transformer[]; }; -export const createTestShadowDom = ( +export const createTestShadowDom = async ( fixture: string, opts: CreateTestShadowDomOptions = { preTransformers: [], postTransformers: [], }, -): ShadowRoot => { +): Promise => { const divElement = document.createElement('div'); divElement.attachShadow({ mode: 'open' }); document.body.appendChild(divElement); @@ -39,7 +39,7 @@ export const createTestShadowDom = ( 'text/html', ).documentElement; if (opts.preTransformers) { - dom = transformer(dom, opts.preTransformers); + dom = await transformer(dom, opts.preTransformers); } // Mount the UI @@ -47,7 +47,7 @@ export const createTestShadowDom = ( // Transformers after the UI is rendered if (opts.postTransformers) { - transformer(dom, opts.postTransformers); + await transformer(dom, opts.postTransformers); } return divElement.shadowRoot!; From d32d01e5bcfb73e4c7bd56ef662d48a37e0e9636 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Fri, 9 Jul 2021 09:54:42 +0200 Subject: [PATCH 05/29] Introduce the annotation `backstage.io/techdocs-ref: ` as an alias for `backstage.io/techdocs-ref: dir:` Signed-off-by: Dominik Henneke --- .changeset/techdocs-twenty-donuts-eat.md | 89 +++++++ packages/techdocs-common/api-report.md | 39 ++- packages/techdocs-common/src/helpers.test.ts | 239 +++++++++++++++++- packages/techdocs-common/src/helpers.ts | 128 +++++++++- .../src/stages/prepare/dir.test.ts | 18 +- .../techdocs-common/src/stages/prepare/dir.ts | 93 ++----- .../src/stages/prepare/preparers.ts | 30 ++- plugins/techdocs-backend/package.json | 1 + .../src/DocsBuilder/builder.ts | 10 +- .../src/service/DocsSynchronizer.test.ts | 2 + .../src/service/DocsSynchronizer.ts | 6 + .../techdocs-backend/src/service/router.ts | 11 +- 12 files changed, 556 insertions(+), 110 deletions(-) create mode 100644 .changeset/techdocs-twenty-donuts-eat.md diff --git a/.changeset/techdocs-twenty-donuts-eat.md b/.changeset/techdocs-twenty-donuts-eat.md new file mode 100644 index 0000000000..b4f9a067c3 --- /dev/null +++ b/.changeset/techdocs-twenty-donuts-eat.md @@ -0,0 +1,89 @@ +--- +'@backstage/techdocs-common': minor +'@backstage/plugin-techdocs-backend': minor +--- + +Introduce the annotation `backstage.io/techdocs-ref: ` as an alias for `backstage.io/techdocs-ref: dir:`. +This annotation works with both the basic and the recommended flow, however, it will be most useful with the basic approach. + +In addition, this change removes the support of the deprecated `github`, `gitlab`, and `azure/api` locations from the `dir` reference preparer. + +#### Example Usage + +The new annotation is convenient if the documentation is stored in the same location, i.e. the same git repository, as the `catalog-info.yaml`. +While it is still supported to add full URLs such as `backstage.io/techdocs-ref: url:https://...` for custom setups, documentation is mostly stored in the same repository as the entity definition. +By automatically resolving the target relative to the registration location of the entity, the configuration overhead for this default setup is minimized. +Since it leverages the `@backstage/integrations` package for the URL resolution, this is compatible with every supported source. + +Consider the following examples: + +> Note that the short version `` is only an alias for the still supported `dir:`. + +1. "I have a repository with a single `catalog-info.yaml` and a TechDocs page in the root folder!" + +``` +https://github.com/backstage/example/tree/main/ + |- catalog-info.yaml + | > apiVersion: backstage.io/v1alpha1 + | > kind: Component + | > metadata: + | > name: example + | > annotations: + | > backstage.io/techdocs-ref: . # -> same folder + | > spec: {} + |- docs/ + |- mkdocs.yml +``` + +2. "I have a repository with a single `catalog-info.yaml` and my TechDocs page in located in a folder!" + +``` +https://bitbucket.org/my-owner/my-project/src/master/ + |- catalog-info.yaml + | > apiVersion: backstage.io/v1alpha1 + | > kind: Component + | > metadata: + | > name: example + | > annotations: + | > backstage.io/techdocs-ref: ./some-folder # -> subfolder + | > spec: {} + |- some-folder/ + |- docs/ + |- mkdocs.yml +``` + +3. "I have a mono repository that hosts multiple components!" + +``` +https://dev.azure.com/organization/project/_git/repository + |- my-1st-module/ + |- catalog-info.yaml + | > apiVersion: backstage.io/v1alpha1 + | > kind: Component + | > metadata: + | > name: my-1st-module + | > annotations: + | > backstage.io/techdocs-ref: . # -> same folder + | > spec: {} + |- docs/ + |- mkdocs.yml + |- my-2nd-module/ + |- catalog-info.yaml + | > apiVersion: backstage.io/v1alpha1 + | > kind: Component + | > metadata: + | > name: my-2nd-module + | > annotations: + | > backstage.io/techdocs-ref: . # -> same folder + | > spec: {} + |- docs/ + |- mkdocs.yml + |- catalog-info.yaml + | > apiVersion: backstage.io/v1alpha1 + | > kind: Location + | > metadata: + | > name: example + | > spec: + | > targets: + | > - ./*/catalog-info.yaml +``` diff --git a/packages/techdocs-common/api-report.md b/packages/techdocs-common/api-report.md index 7c44dc5484..9fc1795c13 100644 --- a/packages/techdocs-common/api-report.md +++ b/packages/techdocs-common/api-report.md @@ -15,6 +15,7 @@ import { GitHubIntegrationConfig } from '@backstage/integration'; import { GitLabIntegrationConfig } from '@backstage/integration'; import { Logger as Logger_2 } from 'winston'; import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { ScmIntegrationRegistry } from '@backstage/integration'; import { UrlReader } from '@backstage/backend-common'; import { Writable } from 'stream'; @@ -47,9 +48,15 @@ export class CommonGitPreparer implements PreparerBase { // // @public (undocumented) export class DirectoryPreparer implements PreparerBase { - constructor(config: Config, logger: Logger_2, reader: UrlReader); + constructor(config: Config, _logger: Logger_2, reader: UrlReader); // (undocumented) - prepare(entity: Entity): Promise; + prepare( + entity: Entity, + options?: { + logger?: Logger_2; + etag?: string; + }, + ): Promise; } // Warning: (ae-missing-release-tag) "GeneratorBase" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) @@ -130,6 +137,18 @@ export const getDefaultBranch: ( config: Config, ) => Promise; +// Warning: (ae-missing-release-tag) "getDirLocation" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export const getDirLocation: ( + entity: Entity, +) => + | { + type: 'dir'; + target: string; + } + | undefined; + // Warning: (ae-missing-release-tag) "getDocFilesFromRepository" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) @@ -189,7 +208,10 @@ export const getLastCommitTimestamp: ( // Warning: (ae-missing-release-tag) "getLocationForEntity" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) -export const getLocationForEntity: (entity: Entity) => ParsedLocationAnnotation; +export const getLocationForEntity: ( + entity: Entity, + scmIntegration: ScmIntegrationRegistry, +) => ParsedLocationAnnotation; // Warning: (ae-missing-release-tag) "getTokenForGitRepo" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // @@ -355,6 +377,17 @@ export type TechDocsMetadata = { etag: string; }; +// Warning: (ae-missing-release-tag) "transformDirLocation" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public +export const transformDirLocation: ( + entity: Entity, + scmIntegrations: ScmIntegrationRegistry, +) => { + type: 'dir' | 'url'; + target: string; +}; + // Warning: (ae-missing-release-tag) "UrlPreparer" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/packages/techdocs-common/src/helpers.test.ts b/packages/techdocs-common/src/helpers.test.ts index b94cbba572..6b677c6f5e 100644 --- a/packages/techdocs-common/src/helpers.test.ts +++ b/packages/techdocs-common/src/helpers.test.ts @@ -19,14 +19,27 @@ import { SearchResponse, UrlReader, } from '@backstage/backend-common'; -import { Entity } from '@backstage/catalog-model'; +import { Entity, getEntitySourceLocation } from '@backstage/catalog-model'; +import { ConfigReader } from '@backstage/config'; +import { ScmIntegrations } from '@backstage/integration'; +import os from 'os'; +import path from 'path'; import { Readable } from 'stream'; import { + getDirLocation, getDocFilesFromRepository, getLocationForEntity, parseReferenceAnnotation, + transformDirLocation, } from './helpers'; +jest.mock('@backstage/catalog-model', () => ({ + ...jest.requireActual('@backstage/catalog-model'), + getEntitySourceLocation: jest.fn(), +})); + +const rootDir = os.platform() === 'win32' ? 'C:\\rootDir' : '/rootDir'; + const entityBase: Entity = { metadata: { namespace: 'default', @@ -81,6 +94,10 @@ const mockEntityWithBadAnnotation: Entity = { }, }; +const scmIntegrations = ScmIntegrations.fromConfig(new ConfigReader({})); + +afterEach(() => jest.resetAllMocks()); + describe('parseReferenceAnnotation', () => { it('should parse annotation', () => { const parsedLocationAnnotation = parseReferenceAnnotation( @@ -109,10 +126,230 @@ describe('parseReferenceAnnotation', () => { }); }); +describe('getDirLocation', () => { + it.each` + techdocsRef | responseTarget + ${undefined} | ${undefined} + ${16} | ${undefined} + ${'.'} | ${'.'} + ${'dir:.'} | ${'.'} + ${'./relative'} | ${'./relative'} + ${'dir:./relative'} | ${'./relative'} + ${'dir:https://github.com...'} | ${'https://github.com...'} + ${'url:https://github.com...'} | ${undefined} + `( + 'should handle "backstage.io/techdocs-ref: $techdocsRef" correctly', + ({ techdocsRef, responseTarget }) => { + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': techdocsRef, + }, + }, + }; + + const result = getDirLocation(entity); + + expect(result).toEqual( + responseTarget && { type: 'dir', target: responseTarget }, + ); + }, + ); + + it('Reject https urls and hint to the url: location type', async () => { + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': 'https://github.com/backstage/backstage', + }, + }, + }; + + expect(() => getDirLocation(entity)).toThrow( + /please prefix it with 'url:'/, + ); + }); +}); + +describe('transformDirLocation', () => { + it('should reject missing annotation', () => { + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + }, + }; + + expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( + /No techdocs location annotation provided in entity: component:default\/test/, + ); + }); + + it.each` + techdocsRef | target + ${'.'} | ${'https://my-url/folder/'} + ${'./sub-folder'} | ${'https://my-url/folder/sub-folder'} + `( + 'should transform "$techdocsRef" for url type locations', + ({ techdocsRef, target }) => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'url', + target: 'https://my-url/folder/', + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': techdocsRef, + }, + }, + }; + + const result = transformDirLocation(entity, scmIntegrations); + + expect(result).toEqual({ type: 'url', target }); + }, + ); + + it.each` + techdocsRef | target + ${'.'} | ${path.join(rootDir, 'working-copy')} + ${'./sub-folder'} | ${path.join(rootDir, 'working-copy', 'sub-folder')} + `( + 'should transform "$techdocsRef" for file type locations', + ({ techdocsRef, target }) => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'file', + target: path.join(rootDir, 'working-copy', 'catalog-info.yaml'), + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': techdocsRef, + }, + }, + }; + + const result = transformDirLocation(entity, scmIntegrations); + + expect(result).toEqual({ type: 'dir', target }); + }, + ); + + it('should reject unsafe file location', () => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'file', + target: '/tmp/catalog-info.yaml', + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': '..', + }, + }, + }; + + expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( + /Relative path is not allowed to refer to a directory outside its parent/, + ); + }); + + it('should reject other location types', () => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'other', + target: '/', + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': '.', + }, + }, + }; + + expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( + /Unable to resolve location type other/, + ); + }); +}); + describe('getLocationForEntity', () => { + it('should handle implicit dir locations', () => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'url', + target: 'https://my-url/folder/', + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': '.', + }, + }, + }; + + const parsedLocationAnnotation = getLocationForEntity( + entity, + scmIntegrations, + ); + expect(parsedLocationAnnotation.type).toBe('url'); + expect(parsedLocationAnnotation.target).toBe('https://my-url/folder/'); + }); + + it('should handle dir locations', () => { + (getEntitySourceLocation as jest.Mock).mockReturnValue({ + type: 'url', + target: 'https://my-url/folder/', + }); + + const entity = { + apiVersion: '1', + kind: 'Component', + metadata: { + name: 'test', + annotations: { + 'backstage.io/techdocs-ref': 'dir:.', + }, + }, + }; + + const parsedLocationAnnotation = getLocationForEntity( + entity, + scmIntegrations, + ); + expect(parsedLocationAnnotation.type).toBe('url'); + expect(parsedLocationAnnotation.target).toBe('https://my-url/folder/'); + }); + it('should get location for entity', () => { const parsedLocationAnnotation = getLocationForEntity( mockEntityWithAnnotation, + scmIntegrations, ); expect(parsedLocationAnnotation.type).toBe('url'); expect(parsedLocationAnnotation.target).toBe( diff --git a/packages/techdocs-common/src/helpers.ts b/packages/techdocs-common/src/helpers.ts index cac8a7047f..bb640c1cce 100644 --- a/packages/techdocs-common/src/helpers.ts +++ b/packages/techdocs-common/src/helpers.ts @@ -14,10 +14,20 @@ * limitations under the License. */ -import { Git, UrlReader } from '@backstage/backend-common'; -import { InputError } from '@backstage/errors'; -import { Entity, parseLocationReference } from '@backstage/catalog-model'; +import { + Git, + resolveSafeChildPath, + UrlReader, +} from '@backstage/backend-common'; +import { + Entity, + getEntitySourceLocation, + parseLocationReference, + stringifyEntityRef, +} from '@backstage/catalog-model'; import { Config } from '@backstage/config'; +import { InputError } from '@backstage/errors'; +import { ScmIntegrationRegistry } from '@backstage/integration'; import fs from 'fs-extra'; import parseGitUrl from 'git-url-parse'; import os from 'os'; @@ -50,9 +60,113 @@ export const parseReferenceAnnotation = ( }; }; +/** + * Check if the entity provides a `backstage.io/techdocs-ref` annotation of type `dir` + * and return the annotation. It accepts the two variants `` and `dir:`. + * + * @param entity - the entity to check + */ +export const getDirLocation = ( + entity: Entity, +): { type: 'dir'; target: string } | undefined => { + const annotation = entity.metadata.annotations?.['backstage.io/techdocs-ref']; + + if (!annotation) { + return undefined; + } + + if (typeof annotation !== 'string') { + return undefined; + } + + // if the string doesn't contain `:`, interpret it as the target of a dir type + if (!annotation.includes(':')) { + return { type: 'dir', target: annotation }; + } + + // note that `backstage.io/techdocs-ref: https://...` is invalid and will throw + const reference = parseLocationReference(annotation); + + if (reference.type === 'dir') { + return { + type: 'dir', + target: reference.target, + }; + } + + // ignore any other types + return undefined; +}; + +/** + * TechDocs references of type `dir` are relative the source location of the entity. + * This function transforms relative references to absolute ones, based on the + * location the entity was ingested from. If the entity was registered by a `url` + * location, it returns a `url` location with a resolved target that points to the + * targeted subfolder. If the entity was registered by a `file` location, it returns + * an absolute `dir` location. + * + * @param entity - the entity with annotations + * @param scmIntegrations - access to the scmIntegrationt to do url transformations + * @throws if the entity doesn't specify a `dir` location or is ingested from an unsupported location. + * @returns the transformed location with an absolute target. + */ +export const transformDirLocation = ( + entity: Entity, + scmIntegrations: ScmIntegrationRegistry, +): { type: 'dir' | 'url'; target: string } => { + const dirLocation = getDirLocation(entity); + + if (!dirLocation) { + throw new InputError( + `No techdocs location annotation provided in entity: ${stringifyEntityRef( + entity, + )}`, + ); + } + + const location = getEntitySourceLocation(entity); + + switch (location.type) { + case 'url': { + const target = scmIntegrations.resolveUrl({ + url: dirLocation.target, + base: location.target, + }); + + return { + type: 'url', + target, + }; + } + + case 'file': { + // only permit targets in the same folder as the target of the `file` location! + const target = resolveSafeChildPath( + path.dirname(location.target), + dirLocation.target, + ); + + return { + type: 'dir', + target, + }; + } + + default: + throw new InputError(`Unable to resolve location type ${location.type}`); + } +}; + export const getLocationForEntity = ( entity: Entity, + scmIntegration: ScmIntegrationRegistry, ): ParsedLocationAnnotation => { + // try to resolve relative references first + if (getDirLocation(entity) !== undefined) { + return transformDirLocation(entity, scmIntegration); + } + const { type, target } = parseReferenceAnnotation( 'backstage.io/techdocs-ref', entity, @@ -64,14 +178,6 @@ export const getLocationForEntity = ( case 'azure/api': case 'url': return { type, target }; - case 'dir': - if (path.isAbsolute(target)) { - return { type, target }; - } - return parseReferenceAnnotation( - 'backstage.io/managed-by-location', - entity, - ); default: throw new Error(`Invalid reference annotation ${type}`); } diff --git a/packages/techdocs-common/src/stages/prepare/dir.test.ts b/packages/techdocs-common/src/stages/prepare/dir.test.ts index 6992a35c65..019bf51fe9 100644 --- a/packages/techdocs-common/src/stages/prepare/dir.test.ts +++ b/packages/techdocs-common/src/stages/prepare/dir.test.ts @@ -15,7 +15,6 @@ */ import { getVoidLogger, UrlReader } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; -import { checkoutGitRepository } from '../../helpers'; import { DirectoryPreparer } from './dir'; function normalizePath(path: string) { @@ -27,8 +26,6 @@ function normalizePath(path: string) { jest.mock('../../helpers', () => ({ ...jest.requireActual<{}>('../../helpers'), - checkoutGitRepository: jest.fn(() => '/tmp/backstage-repo/org/name/branch/'), - getLastCommitTimestamp: jest.fn(() => 12345678), })); const logger = getVoidLogger(); @@ -71,7 +68,7 @@ describe('directory preparer', () => { expect(normalizePath(preparedDir)).toEqual('/directory/our-documentation'); }); - it('should merge managed-by-location and techdocs-ref when techdocs-ref is absolute', async () => { + it('should reject when techdocs-ref is absolute', async () => { const directoryPreparer = new DirectoryPreparer( mockConfig, logger, @@ -84,11 +81,12 @@ describe('directory preparer', () => { 'backstage.io/techdocs-ref': 'dir:/our-documentation/techdocs', }); - const { preparedDir } = await directoryPreparer.prepare(mockEntity); - expect(normalizePath(preparedDir)).toEqual('/our-documentation/techdocs'); + await expect(directoryPreparer.prepare(mockEntity)).rejects.toThrow( + /Relative path is not allowed to refer to a directory outside its parent/, + ); }); - it('should merge managed-by-location and techdocs-ref when managed-by-location is a git repository', async () => { + it('should reject when managed-by-location is a git repository', async () => { const directoryPreparer = new DirectoryPreparer( mockConfig, logger, @@ -101,10 +99,8 @@ describe('directory preparer', () => { 'backstage.io/techdocs-ref': 'dir:./docs', }); - const { preparedDir } = await directoryPreparer.prepare(mockEntity); - expect(normalizePath(preparedDir)).toEqual( - '/tmp/backstage-repo/org/name/branch/docs', + await expect(directoryPreparer.prepare(mockEntity)).rejects.toThrow( + /Unable to resolve location type github/, ); - expect(checkoutGitRepository).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/techdocs-common/src/stages/prepare/dir.ts b/packages/techdocs-common/src/stages/prepare/dir.ts index 9ec4b4d37a..b0ba2a36b3 100644 --- a/packages/techdocs-common/src/stages/prepare/dir.ts +++ b/packages/techdocs-common/src/stages/prepare/dir.ts @@ -15,104 +15,57 @@ */ import { UrlReader } from '@backstage/backend-common'; -import { InputError, NotModifiedError } from '@backstage/errors'; import { Entity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import parseGitUrl from 'git-url-parse'; -import path from 'path'; -import { Logger } from 'winston'; +import { InputError } from '@backstage/errors'; import { - checkoutGitRepository, - getLastCommitTimestamp, - parseReferenceAnnotation, -} from '../../helpers'; + ScmIntegrationRegistry, + ScmIntegrations, +} from '@backstage/integration'; +import { Logger } from 'winston'; +import { transformDirLocation } from '../../helpers'; import { PreparerBase, PreparerResponse } from './types'; export class DirectoryPreparer implements PreparerBase { - constructor( - private readonly config: Config, - private readonly logger: Logger, - private readonly reader: UrlReader, - ) { - this.config = config; - this.logger = logger; + private readonly scmIntegrations: ScmIntegrationRegistry; + private readonly reader: UrlReader; + + constructor(config: Config, _logger: Logger, reader: UrlReader) { this.reader = reader; + this.scmIntegrations = ScmIntegrations.fromConfig(config); } - private async resolveManagedByLocationToDir( + async prepare( entity: Entity, - options?: { etag?: string }, + options?: { logger?: Logger; etag?: string }, ): Promise { - const { type, target } = parseReferenceAnnotation( - 'backstage.io/managed-by-location', - entity, - ); + const { type, target } = transformDirLocation(entity, this.scmIntegrations); - this.logger.debug( - `Building docs for entity with type 'dir' and managed-by-location '${type}'`, - ); switch (type) { case 'url': { + options?.logger?.info(`Download documentation from ${target}`); + // the target is an absolute url since it has already been transformed const response = await this.reader.readTree(target, { etag: options?.etag, }); - const preparedDir = await response.dir(); + return { - preparedDir, + preparedDir: await response.dir(), etag: response.etag, }; } - case 'github': - case 'gitlab': - case 'azure/api': { - const parsedGitLocation = parseGitUrl(target); - const repoLocation = await checkoutGitRepository( - target, - this.config, - this.logger, - ); - // Check if etag has changed for cache invalidation. - const etag = await getLastCommitTimestamp(repoLocation, this.logger); - if (options?.etag === etag.toString()) { - throw new NotModifiedError(); - } + case 'dir': { return { - preparedDir: path.dirname( - path.join(repoLocation, parsedGitLocation.filepath), - ), - etag: etag.toString(), - }; - } - case 'file': - return { - preparedDir: path.dirname(target), + // the transformation already validated that the target is in a safe location + preparedDir: target, // Instead of supporting caching on local sources, use techdocs-cli for local development and debugging. etag: '', }; + } + default: throw new InputError(`Unable to resolve location type ${type}`); } } - - async prepare(entity: Entity): Promise { - this.logger.warn( - 'You are using the legacy dir preparer in TechDocs which will be removed in near future (March 2021). ' + - 'Migrate to URL reader by updating `backstage.io/techdocs-ref` annotation in `catalog-info.yaml` ' + - 'to be prefixed with `url:`. Read the migration guide and benefits at https://github.com/backstage/backstage/issues/4409 ', - ); - - const { target } = parseReferenceAnnotation( - 'backstage.io/techdocs-ref', - entity, - ); - - // This will throw NotModified error if etag has not changed. - const response = await this.resolveManagedByLocationToDir(entity); - - return { - preparedDir: path.resolve(response.preparedDir, target), - etag: response.etag, - }; - } } diff --git a/packages/techdocs-common/src/stages/prepare/preparers.ts b/packages/techdocs-common/src/stages/prepare/preparers.ts index 5c78d96f15..48e99d48f5 100644 --- a/packages/techdocs-common/src/stages/prepare/preparers.ts +++ b/packages/techdocs-common/src/stages/prepare/preparers.ts @@ -13,15 +13,17 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { UrlReader } from '@backstage/backend-common'; -import { Entity } from '@backstage/catalog-model'; +import { Entity, parseLocationReference } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; +import { InputError } from '@backstage/errors'; import { Logger } from 'winston'; -import { parseReferenceAnnotation } from '../../helpers'; -import { DirectoryPreparer } from './dir'; +import { getDirLocation } from '../../helpers'; import { CommonGitPreparer } from './commonGit'; -import { UrlPreparer } from './url'; +import { DirectoryPreparer } from './dir'; import { PreparerBase, PreparerBuilder, RemoteProtocol } from './types'; +import { UrlPreparer } from './url'; type factoryOptions = { logger: Logger; @@ -61,11 +63,21 @@ export class Preparers implements PreparerBuilder { } get(entity: Entity): PreparerBase { - const { type } = parseReferenceAnnotation( - 'backstage.io/techdocs-ref', - entity, - ); - const preparer = this.preparerMap.get(type); + const annotation = + entity.metadata.annotations?.['backstage.io/techdocs-ref']; + if (!annotation) { + throw new InputError( + `No location annotation provided in entity: ${entity.metadata.name}`, + ); + } + + // the dir processor handles both `` and `dir:` + if (getDirLocation(entity) !== undefined) { + return this.preparerMap.get('dir')!; + } + + const { type } = parseLocationReference(annotation); + const preparer = this.preparerMap.get(type as RemoteProtocol); if (!preparer) { throw new Error(`No preparer registered for type: "${type}"`); diff --git a/plugins/techdocs-backend/package.json b/plugins/techdocs-backend/package.json index 76aadc79cf..4ae17d307c 100644 --- a/plugins/techdocs-backend/package.json +++ b/plugins/techdocs-backend/package.json @@ -35,6 +35,7 @@ "@backstage/catalog-model": "^0.9.0", "@backstage/config": "^0.1.5", "@backstage/errors": "^0.1.1", + "@backstage/integration": "^0.5.8", "@backstage/techdocs-common": "^0.6.8", "@types/express": "^4.17.6", "cross-fetch": "^3.0.6", diff --git a/plugins/techdocs-backend/src/DocsBuilder/builder.ts b/plugins/techdocs-backend/src/DocsBuilder/builder.ts index 2cd6f57154..5327062233 100644 --- a/plugins/techdocs-backend/src/DocsBuilder/builder.ts +++ b/plugins/techdocs-backend/src/DocsBuilder/builder.ts @@ -20,6 +20,7 @@ import { } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { NotModifiedError } from '@backstage/errors'; +import { ScmIntegrationRegistry } from '@backstage/integration'; import { GeneratorBase, GeneratorBuilder, @@ -43,6 +44,7 @@ type DocsBuilderArguments = { entity: Entity; logger: Logger; config: Config; + scmIntegrations: ScmIntegrationRegistry; logStream?: Writable; }; @@ -53,6 +55,7 @@ export class DocsBuilder { private entity: Entity; private logger: Logger; private config: Config; + private scmIntegrations: ScmIntegrationRegistry; private logStream: Writable | undefined; constructor({ @@ -62,6 +65,7 @@ export class DocsBuilder { entity, logger, config, + scmIntegrations, logStream, }: DocsBuilderArguments) { this.preparer = preparers.get(entity); @@ -70,6 +74,7 @@ export class DocsBuilder { this.entity = entity; this.logger = logger; this.config = config; + this.scmIntegrations = scmIntegrations; this.logStream = logStream; } @@ -166,7 +171,10 @@ export class DocsBuilder { path.join(tmpdirResolvedPath, 'techdocs-tmp-'), ); - const parsedLocationAnnotation = getLocationForEntity(this.entity); + const parsedLocationAnnotation = getLocationForEntity( + this.entity, + this.scmIntegrations, + ); await this.generator.run({ inputDir: preparedDir, outputDir, diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts index 409dc6e128..ac60f3da3a 100644 --- a/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts @@ -19,6 +19,7 @@ import { PluginEndpointDiscovery, } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; +import { ScmIntegrations } from '@backstage/integration'; import { GeneratorBuilder, PreparerBuilder, @@ -69,6 +70,7 @@ describe('DocsSynchronizer', () => { publisher, config: new ConfigReader({}), logger: getVoidLogger(), + scmIntegrations: ScmIntegrations.fromConfig(new ConfigReader({})), }); }); diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts index 9b51381748..5e5d91b625 100644 --- a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts @@ -17,6 +17,7 @@ import { Entity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { NotFoundError } from '@backstage/errors'; +import { ScmIntegrationRegistry } from '@backstage/integration'; import { GeneratorBuilder, PreparerBuilder, @@ -36,19 +37,23 @@ export class DocsSynchronizer { private readonly publisher: PublisherBase; private readonly logger: winston.Logger; private readonly config: Config; + private readonly scmIntegrations: ScmIntegrationRegistry; constructor({ publisher, logger, config, + scmIntegrations, }: { publisher: PublisherBase; logger: winston.Logger; config: Config; + scmIntegrations: ScmIntegrationRegistry; }) { this.config = config; this.logger = logger; this.publisher = publisher; + this.scmIntegrations = scmIntegrations; } async doSync({ @@ -94,6 +99,7 @@ export class DocsSynchronizer { logger: taskLogger, entity, config: this.config, + scmIntegrations: this.scmIntegrations, logStream, }); diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index b889312abd..e560a895b6 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -29,6 +29,7 @@ import express, { Response } from 'express'; import Router from 'express-promise-router'; import { Knex } from 'knex'; import { Logger } from 'winston'; +import { ScmIntegrations } from '@backstage/integration'; import { DocsSynchronizer, DocsSynchronizerSyncOpts } from './DocsSynchronizer'; /** @@ -79,10 +80,12 @@ export async function createRouter( const router = Router(); const { publisher, config, logger, discovery } = options; const catalogClient = new CatalogClient({ discoveryApi: discovery }); + const scmIntegrations = ScmIntegrations.fromConfig(config); const docsSynchronizer = new DocsSynchronizer({ - publisher: publisher, - logger: logger, - config: config, + publisher, + logger, + config, + scmIntegrations, }); router.get('/metadata/techdocs/:namespace/:kind/:name', async (req, res) => { @@ -126,7 +129,7 @@ export async function createRouter( ) ).json()) as Entity; - const locationMetadata = getLocationForEntity(entity); + const locationMetadata = getLocationForEntity(entity, scmIntegrations); res.json({ ...entity, locationMetadata }); } catch (err) { logger.info( From f3d5a811b0ea067cb631825c5a154ef72551792a Mon Sep 17 00:00:00 2001 From: Alexander Klein Date: Mon, 19 Jul 2021 11:09:31 +0200 Subject: [PATCH 06/29] style: increase sidebar search input hitbox Increase the padding of the sidebar search input. Fixes linked issue. refs #6529 Signed-off-by: Alexander Klein Signed-off-by: Alexander Klein --- packages/core-components/src/layout/Sidebar/Items.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core-components/src/layout/Sidebar/Items.tsx b/packages/core-components/src/layout/Sidebar/Items.tsx index 28abbce53c..6fdf3d56dd 100644 --- a/packages/core-components/src/layout/Sidebar/Items.tsx +++ b/packages/core-components/src/layout/Sidebar/Items.tsx @@ -97,6 +97,9 @@ const useStyles = makeStyles(theme => { fontWeight: 'bold', fontSize: theme.typography.fontSize, }, + searchFieldHTMLInput: { + padding: '15px 0 17px', + }, searchContainer: { width: drawerWidthOpen - iconContainerWidth, }, @@ -271,6 +274,9 @@ export const SidebarSearchField = (props: SidebarSearchFieldProps) => { disableUnderline: true, className: classes.searchField, }} + inputProps={{ + className: classes.searchFieldHTMLInput, + }} /> From 9a751bb28fb6c477f3909f2eebd135f5ef244d3d Mon Sep 17 00:00:00 2001 From: Alexander Klein Date: Mon, 19 Jul 2021 11:29:50 +0200 Subject: [PATCH 07/29] docs: add changeset Signed-off-by: Alexander Klein Signed-off-by: Alexander Klein --- .changeset/chilled-cars-cough.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/chilled-cars-cough.md diff --git a/.changeset/chilled-cars-cough.md b/.changeset/chilled-cars-cough.md new file mode 100644 index 0000000000..b6406fcab1 --- /dev/null +++ b/.changeset/chilled-cars-cough.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-components': patch +--- + +Increase the vertical padding of the sidebar search input field to match the height of the parent anchor tag. This prevents users from accidentally navigating to the search page when they actually wanted to use the search input directly. From f25357273165e618e94a1ffb89396f4dd3be4533 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 19 Jul 2021 14:58:04 +0200 Subject: [PATCH 08/29] Implement the etag functionality in the `readUrl` method of `FetchUrlReader` Signed-off-by: Dominik Henneke --- .changeset/friendly-tips-jam.md | 5 ++ .../src/reading/FetchUrlReader.test.ts | 88 +++++++++++++++++++ .../src/reading/FetchUrlReader.ts | 34 ++++--- 3 files changed, 115 insertions(+), 12 deletions(-) create mode 100644 .changeset/friendly-tips-jam.md diff --git a/.changeset/friendly-tips-jam.md b/.changeset/friendly-tips-jam.md new file mode 100644 index 0000000000..806b632ba1 --- /dev/null +++ b/.changeset/friendly-tips-jam.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Implement the etag functionality in the `readUrl` method of `FetchUrlReader`. diff --git a/packages/backend-common/src/reading/FetchUrlReader.test.ts b/packages/backend-common/src/reading/FetchUrlReader.test.ts index 169cbfbf66..e8c16c5f0a 100644 --- a/packages/backend-common/src/reading/FetchUrlReader.test.ts +++ b/packages/backend-common/src/reading/FetchUrlReader.test.ts @@ -15,12 +15,16 @@ */ import { ConfigReader } from '@backstage/config'; +import { NotFoundError, NotModifiedError } from '@backstage/errors'; import { msw } from '@backstage/test-utils'; +import { rest } from 'msw'; import { setupServer } from 'msw/node'; import { getVoidLogger } from '../logging'; import { FetchUrlReader } from './FetchUrlReader'; import { DefaultReadTreeResponseFactory } from './tree'; +const fetchUrlReader = new FetchUrlReader(); + describe('FetchUrlReader', () => { const worker = setupServer(); @@ -30,6 +34,39 @@ describe('FetchUrlReader', () => { jest.clearAllMocks(); }); + beforeEach(() => { + worker.use( + rest.get('https://backstage.io/some-resource', (req, res, ctx) => { + if (req.headers.get('if-none-match') === 'foo') { + return res( + ctx.status(304), + ctx.set('Content-Type', 'text/plain'), + ctx.set('etag', 'foo'), + ); + } + + return res( + ctx.status(200), + ctx.set('Content-Type', 'text/plain'), + ctx.set('etag', 'foo'), + ctx.body('content foo'), + ); + }), + ); + + worker.use( + rest.get('https://backstage.io/not-exists', (_req, res, ctx) => { + return res(ctx.status(404)); + }), + ); + + worker.use( + rest.get('https://backstage.io/error', (_req, res, ctx) => { + return res(ctx.status(500), ctx.body('An internal error occured')); + }), + ); + }); + it('factory should create a single entry with a predicate that matches config', async () => { const entries = FetchUrlReader.factory({ config: new ConfigReader({ @@ -70,4 +107,55 @@ describe('FetchUrlReader', () => { expect(predicate(new URL('https://a.examples.org:700/test'))).toBe(true); expect(predicate(new URL('https://a.b.examples.org:700/test'))).toBe(true); }); + + describe('read', () => { + it('should return etag from the response', async () => { + const buffer = await fetchUrlReader.read( + 'https://backstage.io/some-resource', + ); + expect(buffer.toString()).toBe('content foo'); + }); + + it('should throw NotFound if server responds with 404', async () => { + await expect( + fetchUrlReader.read('https://backstage.io/not-exists'), + ).rejects.toThrow(NotFoundError); + }); + + it('should throw Error if server responds with 500', async () => { + await expect( + fetchUrlReader.read('https://backstage.io/error'), + ).rejects.toThrow(Error); + }); + }); + + describe('readUrl', () => { + it('should throw NotModified if server responds with 304', async () => { + await expect( + fetchUrlReader.readUrl('https://backstage.io/some-resource', { + etag: 'foo', + }), + ).rejects.toThrow(NotModifiedError); + }); + + it('should return etag from the response', async () => { + const response = await fetchUrlReader.readUrl( + 'https://backstage.io/some-resource', + ); + expect(response.etag).toBe('foo'); + expect((await response.buffer()).toString()).toEqual('content foo'); + }); + + it('should throw NotFound if server responds with 404', async () => { + await expect( + fetchUrlReader.readUrl('https://backstage.io/not-exists'), + ).rejects.toThrow(NotFoundError); + }); + + it('should throw Error if server responds with 500', async () => { + await expect( + fetchUrlReader.readUrl('https://backstage.io/error'), + ).rejects.toThrow(Error); + }); + }); }); diff --git a/packages/backend-common/src/reading/FetchUrlReader.ts b/packages/backend-common/src/reading/FetchUrlReader.ts index 30468158aa..3177ee1f8e 100644 --- a/packages/backend-common/src/reading/FetchUrlReader.ts +++ b/packages/backend-common/src/reading/FetchUrlReader.ts @@ -14,8 +14,8 @@ * limitations under the License. */ +import { NotFoundError, NotModifiedError } from '@backstage/errors'; import fetch from 'cross-fetch'; -import { NotFoundError } from '@backstage/errors'; import { ReaderFactory, ReadTreeResponse, @@ -57,15 +57,34 @@ export class FetchUrlReader implements UrlReader { }; async read(url: string): Promise { + const response = await this.readUrl(url); + return response.buffer(); + } + + async readUrl( + url: string, + options?: ReadUrlOptions, + ): Promise { let response: Response; try { - response = await fetch(url); + response = await fetch(url, { + headers: { + ...(options?.etag && { 'If-None-Match': options.etag }), + }, + }); } catch (e) { throw new Error(`Unable to read ${url}, ${e}`); } + if (response.status === 304) { + throw new NotModifiedError(); + } + if (response.ok) { - return Buffer.from(await response.text()); + return { + buffer: async () => Buffer.from(await response.text()), + etag: response.headers.get('ETag') ?? undefined, + }; } const message = `could not read ${url}, ${response.status} ${response.statusText}`; @@ -75,15 +94,6 @@ export class FetchUrlReader implements UrlReader { throw new Error(message); } - async readUrl( - url: string, - _options?: ReadUrlOptions, - ): Promise { - // TODO etag is not implemented yet. - const buffer = await this.read(url); - return { buffer: async () => buffer }; - } - async readTree(): Promise { throw new Error('FetchUrlReader does not implement readTree'); } From 6d59d17a7fbb2dc91629c9ed3c380878841966eb Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Wed, 14 Jul 2021 10:20:23 +0100 Subject: [PATCH 09/29] Add functionality to the kubernetes plugin that allows users to assume role This change adds a new field to the kubernetes plugin configuration that allows a user to configure an assumed role so that they can access other kubernetes cluster. Signed-off-by: Nicolas Arnold --- plugins/kubernetes-backend/package.json | 4 +- .../ConfigClusterLocator.test.ts | 6 ++ .../cluster-locator/ConfigClusterLocator.ts | 1 + .../src/cluster-locator/index.test.ts | 3 + .../AwsIamKubernetesAuthTranslator.test.ts | 80 ++++++++++++---- .../AwsIamKubernetesAuthTranslator.ts | 91 ++++++++++++++----- .../GoogleKubernetesAuthTranslator.ts | 3 +- .../KubernetesAuthTranslatorGenerator.test.ts | 15 ++- .../ServiceAccountKubernetesAuthTranslator.ts | 3 +- .../src/service/KubernetesFanOutHandler.ts | 12 +-- .../src/service/KubernetesFetcher.ts | 38 ++++---- .../kubernetes-backend/src/service/router.ts | 5 +- plugins/kubernetes-backend/src/types/types.ts | 1 + 13 files changed, 184 insertions(+), 78 deletions(-) diff --git a/plugins/kubernetes-backend/package.json b/plugins/kubernetes-backend/package.json index 6bc7382f54..720321a180 100644 --- a/plugins/kubernetes-backend/package.json +++ b/plugins/kubernetes-backend/package.json @@ -55,7 +55,9 @@ "devDependencies": { "@backstage/cli": "^0.7.4", "@types/aws4": "^1.5.1", - "supertest": "^6.1.3" + "supertest": "^6.1.3", + "aws-sdk-mock": "^5.2.1", + "bdd-lazy-var": "^2.6.0" }, "files": [ "dist", diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts index ac4a742828..eb5e6ff605 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts @@ -35,6 +35,7 @@ describe('ConfigClusterLocator', () => { const config: Config = new ConfigReader({ clusters: [ { + assumeRole: 'SomeRole', name: 'cluster1', url: 'http://localhost:8080', authProvider: 'serviceAccount', @@ -48,6 +49,7 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { + assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: undefined, url: 'http://localhost:8080', @@ -61,6 +63,7 @@ describe('ConfigClusterLocator', () => { const config: Config = new ConfigReader({ clusters: [ { + assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -68,6 +71,7 @@ describe('ConfigClusterLocator', () => { skipTLSVerify: false, }, { + assumeRole: undefined, name: 'cluster2', url: 'http://localhost:8081', authProvider: 'google', @@ -82,6 +86,7 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { + assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -89,6 +94,7 @@ describe('ConfigClusterLocator', () => { skipTLSVerify: false, }, { + assumeRole: undefined, name: 'cluster2', serviceAccountToken: undefined, url: 'http://localhost:8081', diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts index 268cca0193..d2fdbe4211 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts @@ -35,6 +35,7 @@ export class ConfigClusterLocator implements KubernetesClustersSupplier { serviceAccountToken: c.getOptionalString('serviceAccountToken'), skipTLSVerify: c.getOptionalBoolean('skipTLSVerify') ?? false, authProvider: c.getString('authProvider'), + assumeRole: c.getOptionalString('assumeRole'), }; }), ); diff --git a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts index 95be99a8a5..9725d294a9 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts @@ -27,6 +27,7 @@ describe('getCombinedClusterDetails', () => { type: 'config', clusters: [ { + assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -49,6 +50,7 @@ describe('getCombinedClusterDetails', () => { expect(result).toStrictEqual([ { + assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -56,6 +58,7 @@ describe('getCombinedClusterDetails', () => { skipTLSVerify: false, }, { + assumeRole: undefined, name: 'cluster2', serviceAccountToken: undefined, url: 'http://localhost:8081', diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts index 348ad541d5..d5179fc07a 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts @@ -14,15 +14,49 @@ * limitations under the License. */ import AWS from 'aws-sdk'; +import AWSMock from 'aws-sdk-mock'; import { AwsIamKubernetesAuthTranslator } from './AwsIamKubernetesAuthTranslator'; +import { get, def } from 'bdd-lazy-var'; describe('AwsIamKubernetesAuthTranslator tests', () => { + let valid: boolean = true; + let role: any = undefined; + let response: any = { + Credentials: { + AccessKeyId: 'bloop', + SecretAccessKey: 'omg-so-secret', + SessionToken: 'token', + }, + }; + + AWSMock.setSDKInstance(AWS); + beforeEach(() => { jest.resetAllMocks(); }); - it('returns a signed url for aws credentials', async () => { - const authTranslator = new AwsIamKubernetesAuthTranslator(); + afterAll(() => { + jest.resetAllMocks(); + }); + + def('subject', () => { + AWSMock.mock('STS', 'assumeRole', (_params: any, callback: Function) => { + callback(null, response); + }); + + const authTranslator = new AwsIamKubernetesAuthTranslator(); + jest + .spyOn(authTranslator, 'validCredentials') + .mockImplementation(() => valid); + return authTranslator.decorateClusterDetailsWithAuth({ + assumeRole: role, + name: 'test-cluster', + url: '', + authProvider: 'aws', + }); + }); + + it('returns a signed url for aws credentials', async () => { // These credentials are not real. // Pulled from example in docs: https://docs.aws.amazon.com/general/latest/gr/aws-sec-cred-types.html AWS.config.credentials = new AWS.Credentials( @@ -30,24 +64,38 @@ describe('AwsIamKubernetesAuthTranslator tests', () => { 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY', ); - const clusterDetails = await authTranslator.decorateClusterDetailsWithAuth({ - name: 'test-cluster', - url: '', - authProvider: 'aws', + const subject = await get('subject'); + expect(subject.serviceAccountToken).toBeDefined(); + }); + + describe('When the role is assumed', () => { + // These credentials are not real. + // Pulled from example in docs: https://docs.aws.amazon.com/general/latest/gr/aws-sec-cred-types.html + AWS.config.credentials = new AWS.Credentials( + 'AKIAIOSFODNN7EXAMPLE', + 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY', + ); + role = 'SomeRole'; + + describe('When the role is valid', () => { + it('returns a signed url for aws credentials', async () => { + const subject = await get('subject'); + expect(subject.serviceAccountToken).toBeDefined(); + }); + }); + + describe('When the role is invalid', () => { + it('returns the original AWS credentials', async () => { + response = undefined; + + await expect(get('subject')).rejects.toThrow(/Unable to assume role:/); + }); }); - expect(clusterDetails.serviceAccountToken).toBeDefined(); }); it('throws when unable to get aws credentials', async () => { + valid = false; AWS.config.credentials = undefined; - const authTranslator = new AwsIamKubernetesAuthTranslator(); - const promise = authTranslator.decorateClusterDetailsWithAuth({ - name: 'test-cluster', - url: '', - authProvider: 'aws', - }); - await expect(promise).rejects.toThrow( - 'Could not load credentials from any providers', - ); + await expect(get('subject')).rejects.toThrow('No AWS credentials found'); }); }); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts index 0909926d0d..15dfa9e14e 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import AWS, { Credentials } from 'aws-sdk'; +import AWS from 'aws-sdk'; import { sign } from 'aws4'; import { ClusterDetails } from '../types/types'; import { KubernetesAuthTranslator } from './types'; @@ -21,31 +21,80 @@ import { KubernetesAuthTranslator } from './types'; const base64 = (str: string) => Buffer.from(str.toString(), 'binary').toString('base64'); const prepend = (prep: string) => (str: string) => prep + str; -const replace = (search: string | RegExp, substitution: string) => ( - str: string, -) => str.replace(search, substitution); -const pipe = (fns: ReadonlyArray) => (thing: string): string => - fns.reduce((val, fn) => fn(val), thing); +const replace = + (search: string | RegExp, substitution: string) => (str: string) => + str.replace(search, substitution); +const pipe = + (fns: ReadonlyArray) => + (thing: string): string => + fns.reduce((val, fn) => fn(val), thing); const removePadding = replace(/=+$/, ''); const makeUrlSafe = pipe([replace('+', '-'), replace('/', '_')]); +type SigningCreds = { + accessKeyId: string | undefined; + secretAccessKey: string | undefined; + sessionToken: string | undefined; +}; + export class AwsIamKubernetesAuthTranslator - implements KubernetesAuthTranslator { - async getBearerToken(clusterName: string): Promise { - const credentials = await new Promise((resolve, reject) => { - AWS.config.getCredentials(err => { + implements KubernetesAuthTranslator +{ + validCredentials(creds: SigningCreds): boolean { + if (!creds.accessKeyId || !creds.secretAccessKey || !creds.sessionToken) { + return false; + } + return true; + } + + async getCredentials(assumeRole: string | undefined): Promise { + return new Promise(async (resolve, reject) => { + await AWS.config.getCredentials(err => { if (err) { + console.error('Unable to load aws config.'); reject(err); - } else { - resolve(AWS.config.credentials); } }); - }); - if (!(credentials instanceof Credentials)) { - throw new Error('no AWS credentials found.'); - } - await credentials.getPromise(); + let creds: SigningCreds = { + accessKeyId: AWS.config.credentials?.accessKeyId, + secretAccessKey: AWS.config.credentials?.secretAccessKey, + sessionToken: AWS.config.credentials?.sessionToken, + }; + + if (!this.validCredentials(creds)) + return reject(Error('No AWS credentials found.')); + if (!assumeRole) return resolve(creds); + + try { + const params = { + RoleArn: assumeRole, + RoleSessionName: 'backstage-login', + }; + const assumedRole = await new AWS.STS().assumeRole(params).promise(); + + if (!assumedRole.Credentials) { + throw new Error(`No credentials returned for role ${assumeRole}`); + } + + creds = { + accessKeyId: assumedRole.Credentials.AccessKeyId, + secretAccessKey: assumedRole.Credentials.SecretAccessKey, + sessionToken: assumedRole.Credentials.SessionToken, + }; + } catch (e) { + console.warn(`There was an error assuming the role: ${e}`); + return reject(Error(`Unable to assume role: ${e}`)); + } + return resolve(creds); + }); + } + async getBearerToken( + clusterName: string, + assumeRole: string | undefined, + ): Promise { + const credentials = await this.getCredentials(assumeRole); + const request = { host: `sts.amazonaws.com`, path: `/?Action=GetCallerIdentity&Version=2011-06-15&X-Amz-Expires=60`, @@ -54,11 +103,8 @@ export class AwsIamKubernetesAuthTranslator }, signQuery: true, }; - const signedRequest = sign(request, { - accessKeyId: credentials.accessKeyId, - secretAccessKey: credentials.secretAccessKey, - sessionToken: credentials.sessionToken, - }); + + const signedRequest = sign(request, credentials); return pipe([ (signed: any) => `https://${signed.host}${signed.path}`, @@ -79,6 +125,7 @@ export class AwsIamKubernetesAuthTranslator clusterDetailsWithAuthToken.serviceAccountToken = await this.getBearerToken( clusterDetails.name, + clusterDetails.assumeRole, ); return clusterDetailsWithAuthToken; } diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts index 02a7f314af..62ffd823cf 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts @@ -19,7 +19,8 @@ import { ClusterDetails } from '../types/types'; import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; export class GoogleKubernetesAuthTranslator - implements KubernetesAuthTranslator { + implements KubernetesAuthTranslator +{ async decorateClusterDetailsWithAuth( clusterDetails: ClusterDetails, requestBody: KubernetesRequestBody, diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts index 2d900b0bd1..33592b1c41 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts @@ -24,23 +24,20 @@ describe('getKubernetesAuthTranslatorInstance', () => { const sut = KubernetesAuthTranslatorGenerator; it('can return an auth translator for google auth', () => { - const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( - 'google', - ); + const authTranslator: KubernetesAuthTranslator = + sut.getKubernetesAuthTranslatorInstance('google'); expect(authTranslator instanceof GoogleKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for aws auth', () => { - const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( - 'aws', - ); + const authTranslator: KubernetesAuthTranslator = + sut.getKubernetesAuthTranslatorInstance('aws'); expect(authTranslator instanceof AwsIamKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for serviceAccount auth', () => { - const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( - 'serviceAccount', - ); + const authTranslator: KubernetesAuthTranslator = + sut.getKubernetesAuthTranslatorInstance('serviceAccount'); expect( authTranslator instanceof ServiceAccountKubernetesAuthTranslator, ).toBe(true); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts index c433abf4de..098ffe3330 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts @@ -19,7 +19,8 @@ import { ClusterDetails } from '../types/types'; import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; export class ServiceAccountKubernetesAuthTranslator - implements KubernetesAuthTranslator { + implements KubernetesAuthTranslator +{ async decorateClusterDetailsWithAuth( clusterDetails: ClusterDetails, // To ignore TS6133 linting error where it detects 'requestBody' is declared but its value is never read. diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index 3039560aad..e088fddd41 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -63,15 +63,15 @@ export class KubernetesFanOutHandler { 'backstage.io/kubernetes-id' ] || requestBody.entity?.metadata?.name; - const clusterDetails: ClusterDetails[] = await this.serviceLocator.getClustersByServiceId( - entityName, - ); + const clusterDetails: ClusterDetails[] = + await this.serviceLocator.getClustersByServiceId(entityName); // Execute all of these async actions simultaneously/without blocking sequentially as no common object is modified by them const promises: Promise[] = clusterDetails.map(cd => { - const kubernetesAuthTranslator: KubernetesAuthTranslator = KubernetesAuthTranslatorGenerator.getKubernetesAuthTranslatorInstance( - cd.authProvider, - ); + const kubernetesAuthTranslator: KubernetesAuthTranslator = + KubernetesAuthTranslatorGenerator.getKubernetesAuthTranslatorInstance( + cd.authProvider, + ); return kubernetesAuthTranslator.decorateClusterDetailsWithAuth( cd, requestBody, diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index 76c3b0aac0..e17b47e17a 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -167,10 +167,9 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { labelSelector, ).then(r => ({ type: type, resources: r })); case 'services': - return this.fetchServicesForService( - clusterDetails, - labelSelector, - ).then(r => ({ type: type, resources: r })); + return this.fetchServicesForService(clusterDetails, labelSelector).then( + r => ({ type: type, resources: r }), + ); case 'horizontalpodautoscalers': return this.fetchHorizontalPodAutoscalersForService( clusterDetails, @@ -192,9 +191,8 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { customResource: CustomResource, labelSelector: string, ): Promise { - const customObjects = this.kubernetesClientProvider.getCustomObjectsClient( - clusterDetails, - ); + const customObjects = + this.kubernetesClientProvider.getCustomObjectsClient(clusterDetails); return customObjects .listClusterCustomObject( @@ -217,18 +215,20 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { client: Clients, ) => Promise<{ body: { items: Array }; response: http.IncomingMessage }>, ): Promise> { - const core = this.kubernetesClientProvider.getCoreClientByClusterDetails( - clusterDetails, - ); - const apps = this.kubernetesClientProvider.getAppsClientByClusterDetails( - clusterDetails, - ); - const autoscaling = this.kubernetesClientProvider.getAutoscalingClientByClusterDetails( - clusterDetails, - ); - const networkingBeta1 = this.kubernetesClientProvider.getNetworkingBeta1Client( - clusterDetails, - ); + const core = + this.kubernetesClientProvider.getCoreClientByClusterDetails( + clusterDetails, + ); + const apps = + this.kubernetesClientProvider.getAppsClientByClusterDetails( + clusterDetails, + ); + const autoscaling = + this.kubernetesClientProvider.getAutoscalingClientByClusterDetails( + clusterDetails, + ); + const networkingBeta1 = + this.kubernetesClientProvider.getNetworkingBeta1Client(clusterDetails); this.logger.debug(`calling cluster=${clusterDetails.name}`); return fn({ core, apps, autoscaling, networkingBeta1 }).then(({ body }) => { diff --git a/plugins/kubernetes-backend/src/service/router.ts b/plugins/kubernetes-backend/src/service/router.ts index 984b3f14a0..beebd6fa65 100644 --- a/plugins/kubernetes-backend/src/service/router.ts +++ b/plugins/kubernetes-backend/src/service/router.ts @@ -70,9 +70,8 @@ export const makeRouter = ( const serviceId = req.params.serviceId; const requestBody: KubernetesRequestBody = req.body; try { - const response = await kubernetesFanOutHandler.getKubernetesObjectsByEntity( - requestBody, - ); + const response = + await kubernetesFanOutHandler.getKubernetesObjectsByEntity(requestBody); res.json(response); } catch (e) { logger.error( diff --git a/plugins/kubernetes-backend/src/types/types.ts b/plugins/kubernetes-backend/src/types/types.ts index c6e1668a5a..5150eba8cf 100644 --- a/plugins/kubernetes-backend/src/types/types.ts +++ b/plugins/kubernetes-backend/src/types/types.ts @@ -76,4 +76,5 @@ export interface ClusterDetails { authProvider: string; serviceAccountToken?: string | undefined; skipTLSVerify?: boolean; + assumeRole?: string; } From 5bd57f8f5dfced87a104f2f104f90b6732e89de1 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Wed, 14 Jul 2021 10:27:42 +0100 Subject: [PATCH 10/29] Adding changesets Signed-off-by: Nicolas Arnold --- .changeset/blue-feet-poke.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/blue-feet-poke.md diff --git a/.changeset/blue-feet-poke.md b/.changeset/blue-feet-poke.md new file mode 100644 index 0000000000..3be886c687 --- /dev/null +++ b/.changeset/blue-feet-poke.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-kubernetes-backend': minor +--- + +Support assume role on kubernetes api configuration for AWS. From f69d4085834203b2c78fb951fdeb8ccb0ffbaac2 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Wed, 14 Jul 2021 10:42:56 +0100 Subject: [PATCH 11/29] Add API.md for plugin Signed-off-by: Nicolas Arnold --- plugins/kubernetes-backend/api-report.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index 1592f20880..b7ece06ed5 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -14,6 +14,8 @@ import { Logger as Logger_2 } from 'winston'; // // @public (undocumented) export interface ClusterDetails { + // (undocumented) + assumeRole?: string; // (undocumented) authProvider: string; // (undocumented) From ec98274a7ef5fbca1baaec20606433d9dee9b1e6 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Wed, 14 Jul 2021 12:50:24 +0100 Subject: [PATCH 12/29] Creating a Cluster details object per k8s provider Signed-off-by: Nicolas Arnold --- plugins/kubernetes-backend/api-report.md | 17 ++++++- .../ConfigClusterLocator.test.ts | 50 ++++++++++++++++--- .../cluster-locator/ConfigClusterLocator.ts | 24 +++++++-- .../src/cluster-locator/GkeClusterLocator.ts | 4 +- .../src/cluster-locator/index.test.ts | 3 -- .../AwsIamKubernetesAuthTranslator.ts | 23 ++++----- .../GoogleKubernetesAuthTranslator.ts | 11 ++-- .../KubernetesAuthTranslatorGenerator.test.ts | 15 +++--- .../ServiceAccountKubernetesAuthTranslator.ts | 9 ++-- .../src/service/KubernetesFanOutHandler.ts | 12 ++--- .../src/service/KubernetesFetcher.ts | 38 +++++++------- .../kubernetes-backend/src/service/router.ts | 5 +- plugins/kubernetes-backend/src/types/types.ts | 10 +++- 13 files changed, 147 insertions(+), 74 deletions(-) diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index b7ece06ed5..0de29bd2f5 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -13,9 +13,13 @@ import { Logger as Logger_2 } from 'winston'; // Warning: (ae-missing-release-tag) "ClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) -export interface ClusterDetails { +export interface AWSClusterDetails extends ClusterDetails { // (undocumented) assumeRole?: string; +} + +// @public (undocumented) +export interface ClusterDetails { // (undocumented) authProvider: string; // (undocumented) @@ -57,6 +61,9 @@ export interface FetchResponseWrapper { // Warning: (ae-missing-release-tag) "KubernetesClustersSupplier" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // +// @public (undocumented) +export interface GKEClusterDetails extends ClusterDetails {} + // @public (undocumented) export interface KubernetesClustersSupplier { // (undocumented) @@ -109,7 +116,10 @@ export const makeRouter: ( // @public (undocumented) export interface ObjectFetchParams { // (undocumented) - clusterDetails: ClusterDetails; + clusterDetails: + | AWSClusterDetails + | GKEClusterDetails + | ServiceAccountClusterDetails; // (undocumented) customResources: CustomResource[]; // (undocumented) @@ -134,6 +144,9 @@ export interface RouterOptions { // Warning: (ae-missing-release-tag) "ServiceLocatorMethod" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // +// @public (undocumented) +export interface ServiceAccountClusterDetails extends ClusterDetails {} + // @public (undocumented) export type ServiceLocatorMethod = 'multiTenant' | 'http'; diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts index eb5e6ff605..1642ca5580 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts @@ -35,7 +35,6 @@ describe('ConfigClusterLocator', () => { const config: Config = new ConfigReader({ clusters: [ { - assumeRole: 'SomeRole', name: 'cluster1', url: 'http://localhost:8080', authProvider: 'serviceAccount', @@ -49,7 +48,6 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { - assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: undefined, url: 'http://localhost:8080', @@ -63,7 +61,6 @@ describe('ConfigClusterLocator', () => { const config: Config = new ConfigReader({ clusters: [ { - assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -71,7 +68,6 @@ describe('ConfigClusterLocator', () => { skipTLSVerify: false, }, { - assumeRole: undefined, name: 'cluster2', url: 'http://localhost:8081', authProvider: 'google', @@ -86,7 +82,6 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { - assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -94,7 +89,6 @@ describe('ConfigClusterLocator', () => { skipTLSVerify: false, }, { - assumeRole: undefined, name: 'cluster2', serviceAccountToken: undefined, url: 'http://localhost:8081', @@ -103,4 +97,48 @@ describe('ConfigClusterLocator', () => { }, ]); }); + + it('one aws cluster with assumeRole and one without', async () => { + const config: Config = new ConfigReader({ + clusters: [ + { + name: 'cluster1', + serviceAccountToken: 'token', + url: 'http://localhost:8080', + authProvider: 'aws', + skipTLSVerify: false, + }, + { + assumeRole: 'SomeRole', + name: 'cluster2', + url: 'http://localhost:8081', + authProvider: 'aws', + skipTLSVerify: true, + }, + ], + }); + + const sut = ConfigClusterLocator.fromConfig(config); + + const result = await sut.getClusters(); + + expect(result).toStrictEqual([ + { + assumeRole: undefined, + name: 'cluster1', + serviceAccountToken: 'token', + url: 'http://localhost:8080', + authProvider: 'aws', + skipTLSVerify: false, + }, + { + assumeRole: 'SomeRole', + name: 'cluster2', + serviceAccountToken: undefined, + url: 'http://localhost:8081', + authProvider: 'aws', + skipTLSVerify: true, + }, + ]); + }); }); diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts index d2fdbe4211..2899445b2a 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts @@ -29,14 +29,32 @@ export class ConfigClusterLocator implements KubernetesClustersSupplier { // is required if authProvider is serviceAccount return new ConfigClusterLocator( config.getConfigArray('clusters').map(c => { - return { + const authProvider = c.getString('authProvider'); + const clusterDetails = { name: c.getString('name'), url: c.getString('url'), serviceAccountToken: c.getOptionalString('serviceAccountToken'), skipTLSVerify: c.getOptionalBoolean('skipTLSVerify') ?? false, - authProvider: c.getString('authProvider'), - assumeRole: c.getOptionalString('assumeRole'), + authProvider: authProvider, }; + + switch (authProvider) { + case 'google': { + return clusterDetails; + } + case 'aws': { + const assumeRole = c.getOptionalString('assumeRole'); + return { assumeRole, ...clusterDetails }; + } + case 'serviceAccount': { + return clusterDetails; + } + default: { + throw new Error( + `authProvider "${authProvider}" has no config associated with it`, + ); + } + } }), ); } diff --git a/plugins/kubernetes-backend/src/cluster-locator/GkeClusterLocator.ts b/plugins/kubernetes-backend/src/cluster-locator/GkeClusterLocator.ts index 92212d0224..3cc6c216ae 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/GkeClusterLocator.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/GkeClusterLocator.ts @@ -16,7 +16,7 @@ import { Config } from '@backstage/config'; import * as container from '@google-cloud/container'; -import { ClusterDetails, KubernetesClustersSupplier } from '../types/types'; +import { GKEClusterDetails, KubernetesClustersSupplier } from '../types/types'; type GkeClusterLocatorOptions = { projectId: string; @@ -49,7 +49,7 @@ export class GkeClusterLocator implements KubernetesClustersSupplier { ); } - async getClusters(): Promise { + async getClusters(): Promise { const { projectId, region, skipTLSVerify } = this.options; const request = { parent: `projects/${projectId}/locations/${region}`, diff --git a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts index 9725d294a9..95be99a8a5 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts @@ -27,7 +27,6 @@ describe('getCombinedClusterDetails', () => { type: 'config', clusters: [ { - assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -50,7 +49,6 @@ describe('getCombinedClusterDetails', () => { expect(result).toStrictEqual([ { - assumeRole: 'SomeRole', name: 'cluster1', serviceAccountToken: 'token', url: 'http://localhost:8080', @@ -58,7 +56,6 @@ describe('getCombinedClusterDetails', () => { skipTLSVerify: false, }, { - assumeRole: undefined, name: 'cluster2', serviceAccountToken: undefined, url: 'http://localhost:8081', diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts index 15dfa9e14e..e9004818a2 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts @@ -15,19 +15,17 @@ */ import AWS from 'aws-sdk'; import { sign } from 'aws4'; -import { ClusterDetails } from '../types/types'; +import { AWSClusterDetails } from '../types/types'; import { KubernetesAuthTranslator } from './types'; const base64 = (str: string) => Buffer.from(str.toString(), 'binary').toString('base64'); const prepend = (prep: string) => (str: string) => prep + str; -const replace = - (search: string | RegExp, substitution: string) => (str: string) => - str.replace(search, substitution); -const pipe = - (fns: ReadonlyArray) => - (thing: string): string => - fns.reduce((val, fn) => fn(val), thing); +const replace = (search: string | RegExp, substitution: string) => ( + str: string, +) => str.replace(search, substitution); +const pipe = (fns: ReadonlyArray) => (thing: string): string => + fns.reduce((val, fn) => fn(val), thing); const removePadding = replace(/=+$/, ''); const makeUrlSafe = pipe([replace('+', '-'), replace('/', '_')]); @@ -38,8 +36,7 @@ type SigningCreds = { }; export class AwsIamKubernetesAuthTranslator - implements KubernetesAuthTranslator -{ + implements KubernetesAuthTranslator { validCredentials(creds: SigningCreds): boolean { if (!creds.accessKeyId || !creds.secretAccessKey || !creds.sessionToken) { return false; @@ -116,9 +113,9 @@ export class AwsIamKubernetesAuthTranslator } async decorateClusterDetailsWithAuth( - clusterDetails: ClusterDetails, - ): Promise { - const clusterDetailsWithAuthToken: ClusterDetails = Object.assign( + clusterDetails: AWSClusterDetails, + ): Promise { + const clusterDetailsWithAuthToken: AWSClusterDetails = Object.assign( {}, clusterDetails, ); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts index 62ffd823cf..eacba4d3e1 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/GoogleKubernetesAuthTranslator.ts @@ -15,17 +15,16 @@ */ import { KubernetesAuthTranslator } from './types'; -import { ClusterDetails } from '../types/types'; +import { GKEClusterDetails } from '../types/types'; import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; export class GoogleKubernetesAuthTranslator - implements KubernetesAuthTranslator -{ + implements KubernetesAuthTranslator { async decorateClusterDetailsWithAuth( - clusterDetails: ClusterDetails, + clusterDetails: GKEClusterDetails, requestBody: KubernetesRequestBody, - ): Promise { - const clusterDetailsWithAuthToken: ClusterDetails = Object.assign( + ): Promise { + const clusterDetailsWithAuthToken: GKEClusterDetails = Object.assign( {}, clusterDetails, ); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts index 33592b1c41..2d900b0bd1 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/KubernetesAuthTranslatorGenerator.test.ts @@ -24,20 +24,23 @@ describe('getKubernetesAuthTranslatorInstance', () => { const sut = KubernetesAuthTranslatorGenerator; it('can return an auth translator for google auth', () => { - const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance('google'); + const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( + 'google', + ); expect(authTranslator instanceof GoogleKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for aws auth', () => { - const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance('aws'); + const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( + 'aws', + ); expect(authTranslator instanceof AwsIamKubernetesAuthTranslator).toBe(true); }); it('can return an auth translator for serviceAccount auth', () => { - const authTranslator: KubernetesAuthTranslator = - sut.getKubernetesAuthTranslatorInstance('serviceAccount'); + const authTranslator: KubernetesAuthTranslator = sut.getKubernetesAuthTranslatorInstance( + 'serviceAccount', + ); expect( authTranslator instanceof ServiceAccountKubernetesAuthTranslator, ).toBe(true); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts index 098ffe3330..1c3add3c0d 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/ServiceAccountKubernetesAuthTranslator.ts @@ -15,19 +15,18 @@ */ import { KubernetesAuthTranslator } from './types'; -import { ClusterDetails } from '../types/types'; +import { ServiceAccountClusterDetails } from '../types/types'; import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; export class ServiceAccountKubernetesAuthTranslator - implements KubernetesAuthTranslator -{ + implements KubernetesAuthTranslator { async decorateClusterDetailsWithAuth( - clusterDetails: ClusterDetails, + clusterDetails: ServiceAccountClusterDetails, // To ignore TS6133 linting error where it detects 'requestBody' is declared but its value is never read. // @ts-ignore-start requestBody: KubernetesRequestBody, // eslint-disable-line @typescript-eslint/no-unused-vars // @ts-ignore-end - ): Promise { + ): Promise { return clusterDetails; } } diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index e088fddd41..3039560aad 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -63,15 +63,15 @@ export class KubernetesFanOutHandler { 'backstage.io/kubernetes-id' ] || requestBody.entity?.metadata?.name; - const clusterDetails: ClusterDetails[] = - await this.serviceLocator.getClustersByServiceId(entityName); + const clusterDetails: ClusterDetails[] = await this.serviceLocator.getClustersByServiceId( + entityName, + ); // Execute all of these async actions simultaneously/without blocking sequentially as no common object is modified by them const promises: Promise[] = clusterDetails.map(cd => { - const kubernetesAuthTranslator: KubernetesAuthTranslator = - KubernetesAuthTranslatorGenerator.getKubernetesAuthTranslatorInstance( - cd.authProvider, - ); + const kubernetesAuthTranslator: KubernetesAuthTranslator = KubernetesAuthTranslatorGenerator.getKubernetesAuthTranslatorInstance( + cd.authProvider, + ); return kubernetesAuthTranslator.decorateClusterDetailsWithAuth( cd, requestBody, diff --git a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts index e17b47e17a..76c3b0aac0 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFetcher.ts @@ -167,9 +167,10 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { labelSelector, ).then(r => ({ type: type, resources: r })); case 'services': - return this.fetchServicesForService(clusterDetails, labelSelector).then( - r => ({ type: type, resources: r }), - ); + return this.fetchServicesForService( + clusterDetails, + labelSelector, + ).then(r => ({ type: type, resources: r })); case 'horizontalpodautoscalers': return this.fetchHorizontalPodAutoscalersForService( clusterDetails, @@ -191,8 +192,9 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { customResource: CustomResource, labelSelector: string, ): Promise { - const customObjects = - this.kubernetesClientProvider.getCustomObjectsClient(clusterDetails); + const customObjects = this.kubernetesClientProvider.getCustomObjectsClient( + clusterDetails, + ); return customObjects .listClusterCustomObject( @@ -215,20 +217,18 @@ export class KubernetesClientBasedFetcher implements KubernetesFetcher { client: Clients, ) => Promise<{ body: { items: Array }; response: http.IncomingMessage }>, ): Promise> { - const core = - this.kubernetesClientProvider.getCoreClientByClusterDetails( - clusterDetails, - ); - const apps = - this.kubernetesClientProvider.getAppsClientByClusterDetails( - clusterDetails, - ); - const autoscaling = - this.kubernetesClientProvider.getAutoscalingClientByClusterDetails( - clusterDetails, - ); - const networkingBeta1 = - this.kubernetesClientProvider.getNetworkingBeta1Client(clusterDetails); + const core = this.kubernetesClientProvider.getCoreClientByClusterDetails( + clusterDetails, + ); + const apps = this.kubernetesClientProvider.getAppsClientByClusterDetails( + clusterDetails, + ); + const autoscaling = this.kubernetesClientProvider.getAutoscalingClientByClusterDetails( + clusterDetails, + ); + const networkingBeta1 = this.kubernetesClientProvider.getNetworkingBeta1Client( + clusterDetails, + ); this.logger.debug(`calling cluster=${clusterDetails.name}`); return fn({ core, apps, autoscaling, networkingBeta1 }).then(({ body }) => { diff --git a/plugins/kubernetes-backend/src/service/router.ts b/plugins/kubernetes-backend/src/service/router.ts index beebd6fa65..984b3f14a0 100644 --- a/plugins/kubernetes-backend/src/service/router.ts +++ b/plugins/kubernetes-backend/src/service/router.ts @@ -70,8 +70,9 @@ export const makeRouter = ( const serviceId = req.params.serviceId; const requestBody: KubernetesRequestBody = req.body; try { - const response = - await kubernetesFanOutHandler.getKubernetesObjectsByEntity(requestBody); + const response = await kubernetesFanOutHandler.getKubernetesObjectsByEntity( + requestBody, + ); res.json(response); } catch (e) { logger.error( diff --git a/plugins/kubernetes-backend/src/types/types.ts b/plugins/kubernetes-backend/src/types/types.ts index 5150eba8cf..c5f555d588 100644 --- a/plugins/kubernetes-backend/src/types/types.ts +++ b/plugins/kubernetes-backend/src/types/types.ts @@ -27,7 +27,10 @@ export interface CustomResource { export interface ObjectFetchParams { serviceId: string; - clusterDetails: ClusterDetails; + clusterDetails: + | AWSClusterDetails + | GKEClusterDetails + | ServiceAccountClusterDetails; objectTypesToFetch: Set; labelSelector: string; customResources: CustomResource[]; @@ -76,5 +79,10 @@ export interface ClusterDetails { authProvider: string; serviceAccountToken?: string | undefined; skipTLSVerify?: boolean; +} + +export interface GKEClusterDetails extends ClusterDetails {} +export interface ServiceAccountClusterDetails extends ClusterDetails {} +export interface AWSClusterDetails extends ClusterDetails { assumeRole?: string; } From 9b1524ad3348c3f2a69ef1ba8b08d7853710ee75 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 19 Jul 2021 16:55:44 +0200 Subject: [PATCH 13/29] Update changeset Signed-off-by: Dominik Henneke --- .changeset/techdocs-fifty-cameras-listen.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/techdocs-fifty-cameras-listen.md b/.changeset/techdocs-fifty-cameras-listen.md index 9071004acd..fb6be9d709 100644 --- a/.changeset/techdocs-fifty-cameras-listen.md +++ b/.changeset/techdocs-fifty-cameras-listen.md @@ -3,3 +3,5 @@ --- Only update the `path` when the content is updated. +If content and path are updated independently, the frontend rendering is triggered twice on each navigation: Once for the `path` change (with the old content) and once for the new content. +This might result in a flickering rendering that is caused by the async frontend preprocessing, and the fact that replacing the shadow dom content is expensive. From 76fbdbc322e54825b623894928ea04b510351867 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Mon, 19 Jul 2021 17:47:53 +0100 Subject: [PATCH 14/29] Modifying changesets and explicitly returning a promise for the AWS Creds This changes modifies the changeset to only include a patch (as per comments above). I have also changed the flow of retrieving the AWS creds. Previously, they were being returned after they were "resolved". Now we await and resolve a promise to guarantee that the credentials have been loaded. Signed-off-by: Nicolas Arnold --- .changeset/blue-feet-poke.md | 2 +- plugins/kubernetes-backend/api-report.md | 12 +++-- .../AwsIamKubernetesAuthTranslator.test.ts | 46 +++++++++++++------ .../AwsIamKubernetesAuthTranslator.ts | 38 ++++++++++----- 4 files changed, 68 insertions(+), 30 deletions(-) diff --git a/.changeset/blue-feet-poke.md b/.changeset/blue-feet-poke.md index 3be886c687..1a9e32a11a 100644 --- a/.changeset/blue-feet-poke.md +++ b/.changeset/blue-feet-poke.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-kubernetes-backend': minor +'@backstage/plugin-kubernetes-backend': patch --- Support assume role on kubernetes api configuration for AWS. diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index 0de29bd2f5..9f35fda79f 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -10,7 +10,7 @@ import { KubernetesFetchError } from '@backstage/plugin-kubernetes-common'; import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; import { Logger as Logger_2 } from 'winston'; -// Warning: (ae-missing-release-tag) "ClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// Warning: (ae-missing-release-tag) "AWSClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) export interface AWSClusterDetails extends ClusterDetails { @@ -18,6 +18,8 @@ export interface AWSClusterDetails extends ClusterDetails { assumeRole?: string; } +// Warning: (ae-missing-release-tag) "ClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public (undocumented) export interface ClusterDetails { // (undocumented) @@ -59,11 +61,13 @@ export interface FetchResponseWrapper { responses: FetchResponse[]; } -// Warning: (ae-missing-release-tag) "KubernetesClustersSupplier" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// Warning: (ae-missing-release-tag) "GKEClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) export interface GKEClusterDetails extends ClusterDetails {} +// Warning: (ae-missing-release-tag) "KubernetesClustersSupplier" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public (undocumented) export interface KubernetesClustersSupplier { // (undocumented) @@ -142,11 +146,13 @@ export interface RouterOptions { logger: Logger_2; } -// Warning: (ae-missing-release-tag) "ServiceLocatorMethod" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// Warning: (ae-missing-release-tag) "ServiceAccountClusterDetails" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) export interface ServiceAccountClusterDetails extends ClusterDetails {} +// Warning: (ae-missing-release-tag) "ServiceLocatorMethod" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public (undocumented) export type ServiceLocatorMethod = 'multiTenant' | 'http'; diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts index d5179fc07a..9060ce9239 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.test.ts @@ -19,16 +19,23 @@ import { AwsIamKubernetesAuthTranslator } from './AwsIamKubernetesAuthTranslator import { get, def } from 'bdd-lazy-var'; describe('AwsIamKubernetesAuthTranslator tests', () => { - let valid: boolean = true; let role: any = undefined; - let response: any = { + const credentials: any = { + accessKeyId: 'bloop', + secretAccessKey: 'omg-so-secret', + sessionToken: 'token', + }; + + let assumeResponse: any = { Credentials: { - AccessKeyId: 'bloop', - SecretAccessKey: 'omg-so-secret', - SessionToken: 'token', + AccessKeyId: credentials.accessKeyId, + SecretAccessKey: credentials.secretAccessKey, + SessionToken: credentials.sessionToken, }, }; + let credentialsResponse: any = new AWS.Credentials(credentials); + AWSMock.setSDKInstance(AWS); beforeEach(() => { @@ -41,13 +48,15 @@ describe('AwsIamKubernetesAuthTranslator tests', () => { def('subject', () => { AWSMock.mock('STS', 'assumeRole', (_params: any, callback: Function) => { - callback(null, response); + callback(null, assumeResponse); }); const authTranslator = new AwsIamKubernetesAuthTranslator(); + jest - .spyOn(authTranslator, 'validCredentials') - .mockImplementation(() => valid); + .spyOn(authTranslator, 'awsGetCredentials') + .mockImplementation(async () => credentialsResponse); + return authTranslator.decorateClusterDetailsWithAuth({ assumeRole: role, name: 'test-cluster', @@ -86,16 +95,25 @@ describe('AwsIamKubernetesAuthTranslator tests', () => { describe('When the role is invalid', () => { it('returns the original AWS credentials', async () => { - response = undefined; - + assumeResponse = undefined; await expect(get('subject')).rejects.toThrow(/Unable to assume role:/); }); }); }); - it('throws when unable to get aws credentials', async () => { - valid = false; - AWS.config.credentials = undefined; - await expect(get('subject')).rejects.toThrow('No AWS credentials found'); + describe('When no creds are returned from AWS', () => { + it('throws unable to get aws credentials', async () => { + credentialsResponse = new Error(); + await expect(get('subject')).rejects.toThrow('No AWS credentials found.'); + }); + }); + + describe('When invalid creds are returned from AWS', () => { + it('throws credentials are invalid to get aws credentials', async () => { + credentialsResponse = new AWS.Credentials(credentialsResponse); + await expect(get('subject')).rejects.toThrow( + 'Invalid AWS credentials found.', + ); + }); }); }); diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts index e9004818a2..c15022d504 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import AWS from 'aws-sdk'; +import AWS, { Credentials } from 'aws-sdk'; import { sign } from 'aws4'; import { AWSClusterDetails } from '../types/types'; import { KubernetesAuthTranslator } from './types'; @@ -38,29 +38,43 @@ type SigningCreds = { export class AwsIamKubernetesAuthTranslator implements KubernetesAuthTranslator { validCredentials(creds: SigningCreds): boolean { - if (!creds.accessKeyId || !creds.secretAccessKey || !creds.sessionToken) { + if ( + !creds?.accessKeyId || + !creds?.secretAccessKey || + !creds?.sessionToken + ) { return false; } return true; } + awsGetCredentials = async (): Promise => { + return new Promise((resolve, reject) => { + AWS.config.getCredentials(err => { + if (err) { + return reject(err); + } + + return resolve(AWS.config.credentials as Credentials); + }); + }); + }; + async getCredentials(assumeRole: string | undefined): Promise { return new Promise(async (resolve, reject) => { - await AWS.config.getCredentials(err => { - if (err) { - console.error('Unable to load aws config.'); - reject(err); - } - }); + const awsCreds = await this.awsGetCredentials(); + + if (!(awsCreds instanceof Credentials)) + return reject(Error('No AWS credentials found.')); let creds: SigningCreds = { - accessKeyId: AWS.config.credentials?.accessKeyId, - secretAccessKey: AWS.config.credentials?.secretAccessKey, - sessionToken: AWS.config.credentials?.sessionToken, + accessKeyId: awsCreds.accessKeyId, + secretAccessKey: awsCreds.secretAccessKey, + sessionToken: awsCreds.sessionToken, }; if (!this.validCredentials(creds)) - return reject(Error('No AWS credentials found.')); + return reject(Error('Invalid AWS credentials found.')); if (!assumeRole) return resolve(creds); try { From 65a4d898318e9e078199def138f6cc44b258f85b Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Tue, 20 Jul 2021 09:15:03 +0100 Subject: [PATCH 15/29] Refactoring the valid credentials method Signed-off-by: Nicolas Arnold --- .../AwsIamKubernetesAuthTranslator.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts index c15022d504..101bed32a3 100644 --- a/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts +++ b/plugins/kubernetes-backend/src/kubernetes-auth-translator/AwsIamKubernetesAuthTranslator.ts @@ -38,14 +38,9 @@ type SigningCreds = { export class AwsIamKubernetesAuthTranslator implements KubernetesAuthTranslator { validCredentials(creds: SigningCreds): boolean { - if ( - !creds?.accessKeyId || - !creds?.secretAccessKey || - !creds?.sessionToken - ) { - return false; - } - return true; + return ((creds?.accessKeyId && + creds?.secretAccessKey && + creds?.sessionToken) as unknown) as boolean; } awsGetCredentials = async (): Promise => { From 7e84b1bd9f4fab234ff15b513fdf3c26afcade10 Mon Sep 17 00:00:00 2001 From: Nicolas Arnold Date: Tue, 20 Jul 2021 14:20:06 +0100 Subject: [PATCH 16/29] Making changes backwards compatible by keeping the ClusterDetails object as an accepted type. Signed-off-by: Nicolas Arnold --- plugins/kubernetes-backend/api-report.md | 3 ++- plugins/kubernetes-backend/src/types/types.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/kubernetes-backend/api-report.md b/plugins/kubernetes-backend/api-report.md index 9f35fda79f..72d80050d8 100644 --- a/plugins/kubernetes-backend/api-report.md +++ b/plugins/kubernetes-backend/api-report.md @@ -123,7 +123,8 @@ export interface ObjectFetchParams { clusterDetails: | AWSClusterDetails | GKEClusterDetails - | ServiceAccountClusterDetails; + | ServiceAccountClusterDetails + | ClusterDetails; // (undocumented) customResources: CustomResource[]; // (undocumented) diff --git a/plugins/kubernetes-backend/src/types/types.ts b/plugins/kubernetes-backend/src/types/types.ts index c5f555d588..6409229968 100644 --- a/plugins/kubernetes-backend/src/types/types.ts +++ b/plugins/kubernetes-backend/src/types/types.ts @@ -30,7 +30,8 @@ export interface ObjectFetchParams { clusterDetails: | AWSClusterDetails | GKEClusterDetails - | ServiceAccountClusterDetails; + | ServiceAccountClusterDetails + | ClusterDetails; objectTypesToFetch: Set; labelSelector: string; customResources: CustomResource[]; From ed6092f76a606d0d367788c96f4fe6eee8500110 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 21 Jul 2021 12:42:24 +0200 Subject: [PATCH 17/29] Handle and forward errors from the DocsBuilder instantiation to the user Signed-off-by: Dominik Henneke --- .../src/service/DocsSynchronizer.ts | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts index 5e5d91b625..463b895564 100644 --- a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts @@ -92,20 +92,20 @@ export class DocsSynchronizer { return; } - const docsBuilder = new DocsBuilder({ - preparers, - generators, - publisher: this.publisher, - logger: taskLogger, - entity, - config: this.config, - scmIntegrations: this.scmIntegrations, - logStream, - }); - let foundDocs = false; try { + const docsBuilder = new DocsBuilder({ + preparers, + generators, + publisher: this.publisher, + logger: taskLogger, + entity, + config: this.config, + scmIntegrations: this.scmIntegrations, + logStream, + }); + const updated = await docsBuilder.build(); if (!updated) { From 445440a4084fd11739b52ba64871ed422b88f971 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 21 Jul 2021 13:52:29 +0200 Subject: [PATCH 18/29] Make sure all old transformations are cancelled when the hook dependencies changed Signed-off-by: Dominik Henneke --- .../techdocs/src/reader/components/Reader.tsx | 287 ++++++++++-------- 1 file changed, 154 insertions(+), 133 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index d253070827..f861641708 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -28,7 +28,6 @@ import { import { Alert } from '@material-ui/lab'; import React, { useCallback, useEffect, useRef, useState } from 'react'; import { useNavigate, useParams } from 'react-router-dom'; -import { useAsync } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; import { addBaseUrl, @@ -110,32 +109,26 @@ export const Reader = ({ entityId, onReady }: Props) => { // an update to "state" might lead to an updated UI so we include it as a trigger }, [updateSidebarPosition, state]); - useAsync(async () => { - if (!rawPage || !shadowDomRef.current) { - return; - } - if (onReady) { - onReady(); - } - - // Pre-render - const transformedElement = await transformer(rawPage, [ - sanitizeDOM(), - addBaseUrl({ - techdocsStorageApi, - entityId: { - kind, - name, - namespace, - }, - path, - }), - rewriteDocLinks(), - removeMkdocsHeader(), - simplifyMkdocsFooter(), - addGitFeedbackLink(scmIntegrationsApi), - injectCss({ - css: ` + // a function that performs transformations that are executed prior to adding it to the DOM + const preRender = useCallback( + (rawContent: string, contentPath: string) => + transformer(rawContent, [ + sanitizeDOM(), + addBaseUrl({ + techdocsStorageApi, + entityId: { + kind, + name, + namespace, + }, + path: contentPath, + }), + rewriteDocLinks(), + removeMkdocsHeader(), + simplifyMkdocsFooter(), + addGitFeedbackLink(scmIntegrationsApi), + injectCss({ + css: ` body { font-family: ${theme.typography.fontFamily}; --md-text-color: ${theme.palette.text.primary}; @@ -192,21 +185,21 @@ export const Reader = ({ entityId, onReady }: Props) => { } } `, - }), - injectCss({ - // Disable CSS animations on link colors as they lead to issues in dark - // mode. The dark mode color theme is applied later and theirfore there - // is always an animation from light to dark mode when navigation - // between pages. - css: ` + }), + injectCss({ + // Disable CSS animations on link colors as they lead to issues in dark + // mode. The dark mode color theme is applied later and theirfore there + // is always an animation from light to dark mode when navigation + // between pages. + css: ` .md-nav__link, .md-typeset a, .md-typeset a::before, .md-typeset .headerlink { transition: none; } `, - }), - injectCss({ - // Properly style code blocks. - css: ` + }), + injectCss({ + // Properly style code blocks. + css: ` .md-typeset pre > code::-webkit-scrollbar-thumb { background-color: hsla(0, 0%, 0%, 0.32); } @@ -214,17 +207,17 @@ export const Reader = ({ entityId, onReady }: Props) => { background-color: hsla(0, 0%, 0%, 0.87); } `, - }), - injectCss({ - // Admonitions and others are using SVG masks to define icons. These - // masks are defined as CSS variables. - // As the MkDocs output is rendered in shadow DOM, the CSS variable - // definitions on the root selector are not applied. Instead, the have - // to be applied on :host. - // As there is no way to transform the served main*.css yet (for - // example in the backend), we have to copy from main*.css and modify - // them. - css: ` + }), + injectCss({ + // Admonitions and others are using SVG masks to define icons. These + // masks are defined as CSS variables. + // As the MkDocs output is rendered in shadow DOM, the CSS variable + // definitions on the root selector are not applied. Instead, the have + // to be applied on :host. + // As there is no way to transform the served main*.css yet (for + // example in the backend), we have to copy from main*.css and modify + // them. + css: ` :host { --md-admonition-icon--note: url('data:image/svg+xml;charset=utf-8,'); --md-admonition-icon--abstract: url('data:image/svg+xml;charset=utf-8,'); @@ -250,97 +243,125 @@ export const Reader = ({ entityId, onReady }: Props) => { --md-tasklist-icon--checked: url('data:image/svg+xml;charset=utf-8,'); } `, - }), - ]); + }), + ]), + [ + kind, + name, + namespace, + scmIntegrationsApi, + techdocsStorageApi, + theme.palette.background.default, + theme.palette.background.paper, + theme.palette.primary.main, + theme.palette.text.primary, + theme.typography.fontFamily, + ], + ); - if (!transformedElement?.innerHTML) { - return; // An unexpected error occurred + // a function that performs transformations that are executed after adding it to the DOM + const postRender = useCallback( + async (shadowRoot: ShadowRoot) => + transformer(shadowRoot.children[0], [ + dom => { + setTimeout(() => { + // Scoll to the desired anchor on initial navigation + if (window.location.hash) { + const hash = window.location.hash.slice(1); + shadowRoot?.getElementById(hash)?.scrollIntoView(); + } + }, 200); + return dom; + }, + addLinkClickListener({ + baseUrl: window.location.origin, + onClick: (_: MouseEvent, url: string) => { + const parsedUrl = new URL(url); + + if (parsedUrl.hash) { + navigate(`${parsedUrl.pathname}${parsedUrl.hash}`); + + // Scroll to hash if it's on the current page + shadowRoot + ?.getElementById(parsedUrl.hash.slice(1)) + ?.scrollIntoView(); + } else { + navigate(parsedUrl.pathname); + } + }, + }), + onCssReady({ + docStorageUrl: await techdocsStorageApi.getApiOrigin(), + onLoading: (dom: Element) => { + (dom as HTMLElement).style.setProperty('opacity', '0'); + }, + onLoaded: (dom: Element) => { + (dom as HTMLElement).style.removeProperty('opacity'); + // disable MkDocs drawer toggling ('for' attribute => checkbox mechanism) + (dom as HTMLElement) + .querySelector('.md-nav__title') + ?.removeAttribute('for'); + const sideDivs: HTMLElement[] = Array.from( + shadowRoot!.querySelectorAll('.md-sidebar'), + ); + setSidebars(sideDivs); + // set sidebar height so they don't initially render in wrong position + const docTopPosition = (dom as HTMLElement).getBoundingClientRect() + .top; + const mdTabs = dom.querySelector('.md-container > .md-tabs'); + sideDivs!.forEach(sidebar => { + sidebar.style.top = mdTabs + ? `${docTopPosition + mdTabs.getBoundingClientRect().height}px` + : `${docTopPosition}px`; + }); + }, + }), + ]), + [navigate, techdocsStorageApi], + ); + + useEffect(() => { + if (!rawPage || !shadowDomRef.current) { + return () => {}; + } + if (onReady) { + onReady(); } - const shadowDiv: HTMLElement = shadowDomRef.current!; - const shadowRoot = - shadowDiv.shadowRoot || shadowDiv.attachShadow({ mode: 'open' }); - Array.from(shadowRoot.children).forEach(child => - shadowRoot.removeChild(child), - ); - shadowRoot.appendChild(transformedElement); + // if false, there is already a newer execution of this effect + let shouldReplaceContent = true; - // Scroll to top after render - window.scroll({ top: 0 }); + // Pre-render + preRender(rawPage, path).then(async transformedElement => { + if (!transformedElement?.innerHTML) { + return; // An unexpected error occurred + } - // Post-render - await transformer(shadowRoot.children[0], [ - dom => { - setTimeout(() => { - // Scoll to the desired anchor on initial navigation - if (window.location.hash) { - const hash = window.location.hash.slice(1); - shadowRoot?.getElementById(hash)?.scrollIntoView(); - } - }, 200); - return dom; - }, - addLinkClickListener({ - baseUrl: window.location.origin, - onClick: (_: MouseEvent, url: string) => { - const parsedUrl = new URL(url); + // don't manipulate the shadow dom if this isn't the latest effect execution + if (!shouldReplaceContent) { + return; + } - if (parsedUrl.hash) { - navigate(`${parsedUrl.pathname}${parsedUrl.hash}`); + const shadowDiv: HTMLElement = shadowDomRef.current!; + const shadowRoot = + shadowDiv.shadowRoot || shadowDiv.attachShadow({ mode: 'open' }); + Array.from(shadowRoot.children).forEach(child => + shadowRoot.removeChild(child), + ); + shadowRoot.appendChild(transformedElement); - // Scroll to hash if it's on the current page - shadowRoot - ?.getElementById(parsedUrl.hash.slice(1)) - ?.scrollIntoView(); - } else { - navigate(parsedUrl.pathname); - } - }, - }), - onCssReady({ - docStorageUrl: await techdocsStorageApi.getApiOrigin(), - onLoading: (dom: Element) => { - (dom as HTMLElement).style.setProperty('opacity', '0'); - }, - onLoaded: (dom: Element) => { - (dom as HTMLElement).style.removeProperty('opacity'); - // disable MkDocs drawer toggling ('for' attribute => checkbox mechanism) - (dom as HTMLElement) - .querySelector('.md-nav__title') - ?.removeAttribute('for'); - const sideDivs: HTMLElement[] = Array.from( - shadowRoot!.querySelectorAll('.md-sidebar'), - ); - setSidebars(sideDivs); - // set sidebar height so they don't initially render in wrong position - const docTopPosition = (dom as HTMLElement).getBoundingClientRect() - .top; - const mdTabs = dom.querySelector('.md-container > .md-tabs'); - sideDivs!.forEach(sidebar => { - sidebar.style.top = mdTabs - ? `${docTopPosition + mdTabs.getBoundingClientRect().height}px` - : `${docTopPosition}px`; - }); - }, - }), - ]); - }, [ - path, - kind, - namespace, - name, - rawPage, - navigate, - onReady, - shadowDomRef, - techdocsStorageApi, - theme.typography.fontFamily, - theme.palette.text.primary, - theme.palette.primary.main, - theme.palette.background.paper, - theme.palette.background.default, - scmIntegrationsApi, - ]); + // Scroll to top after render + window.scroll({ top: 0 }); + + // Post-render + await postRender(shadowRoot); + }); + + // cancel this execution + return () => { + shouldReplaceContent = false; + }; + }, [onReady, path, postRender, preRender, rawPage]); return ( <> From 8359313600330aa1ac171b13552b742a59547c84 Mon Sep 17 00:00:00 2001 From: blam Date: Wed, 21 Jul 2021 14:32:55 +0200 Subject: [PATCH 19/29] chore: refactoring to use `theme.spacing` Signed-off-by: blam --- packages/core-components/src/layout/Sidebar/Items.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/core-components/src/layout/Sidebar/Items.tsx b/packages/core-components/src/layout/Sidebar/Items.tsx index 6fdf3d56dd..3944df0b50 100644 --- a/packages/core-components/src/layout/Sidebar/Items.tsx +++ b/packages/core-components/src/layout/Sidebar/Items.tsx @@ -44,7 +44,6 @@ const useStyles = makeStyles(theme => { drawerWidthOpen, iconContainerWidth, } = sidebarConfig; - return { root: { color: theme.palette.navigation.color, @@ -98,7 +97,7 @@ const useStyles = makeStyles(theme => { fontSize: theme.typography.fontSize, }, searchFieldHTMLInput: { - padding: '15px 0 17px', + padding: `${theme.spacing(2)} 0 ${theme.spacing(2)}`, }, searchContainer: { width: drawerWidthOpen - iconContainerWidth, From e8dc2b893130e7017fec14892f0eccb200c1b7ac Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 21 Jul 2021 12:48:56 +0000 Subject: [PATCH 20/29] chore(deps-dev): bump @types/jenkins from 0.23.1 to 0.23.2 Bumps [@types/jenkins](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jenkins) from 0.23.1 to 0.23.2. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jenkins) --- updated-dependencies: - dependency-name: "@types/jenkins" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 9431895914..3921f057bc 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5846,9 +5846,9 @@ "@types/istanbul-lib-report" "*" "@types/jenkins@^0.23.1": - version "0.23.1" - resolved "https://registry.npmjs.org/@types/jenkins/-/jenkins-0.23.1.tgz#d0f0ef5511beff975c91cbd2365e580d700ca7f9" - integrity sha512-3oGxVCq+5esbjb0BQXUv0Iz0/7ogJxmzaxKtxwwMik5vGtRvfjWf/sXGA1RzkVAG0+rJUZNKStjKRdtqJfEyRg== + version "0.23.2" + resolved "https://registry.npmjs.org/@types/jenkins/-/jenkins-0.23.2.tgz#96736a2be4904efdfe7fe2650569479fd77dc48e" + integrity sha512-BELmIZ6brxwGFqBHfLjLTjYRWAUqcT1d2BydH1CcRTZEjCYw3DRVfZkXU7BVlyIsKXm2ZMIKVkEjKKtVDvPiXA== dependencies: "@types/node" "*" From d591b764d0f9e2de01b6caadccd046a93d901b7e Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 21 Jul 2021 15:13:16 +0200 Subject: [PATCH 21/29] Remove the short-form annotation and only support the full `dir:.` option Signed-off-by: Dominik Henneke --- .changeset/techdocs-twenty-donuts-eat.md | 17 +-- packages/techdocs-common/api-report.md | 13 +- packages/techdocs-common/src/helpers.test.ts | 137 ++++-------------- packages/techdocs-common/src/helpers.ts | 72 ++------- .../techdocs-common/src/stages/prepare/dir.ts | 19 ++- .../src/stages/prepare/preparers.ts | 23 +-- 6 files changed, 73 insertions(+), 208 deletions(-) diff --git a/.changeset/techdocs-twenty-donuts-eat.md b/.changeset/techdocs-twenty-donuts-eat.md index b4f9a067c3..a95ec5a752 100644 --- a/.changeset/techdocs-twenty-donuts-eat.md +++ b/.changeset/techdocs-twenty-donuts-eat.md @@ -3,22 +3,21 @@ '@backstage/plugin-techdocs-backend': minor --- -Introduce the annotation `backstage.io/techdocs-ref: ` as an alias for `backstage.io/techdocs-ref: dir:`. -This annotation works with both the basic and the recommended flow, however, it will be most useful with the basic approach. +Improve the annotation `backstage.io/techdocs-ref: dir:` that links to a path that is relative to the source of the annotated entity. +This annotation works with the basic and the recommended flow, however, it will be most useful with the basic approach. +This change remove the deprecation of the `dir` reference and provides first-class support for it. In addition, this change removes the support of the deprecated `github`, `gitlab`, and `azure/api` locations from the `dir` reference preparer. #### Example Usage -The new annotation is convenient if the documentation is stored in the same location, i.e. the same git repository, as the `catalog-info.yaml`. +The annotation is convenient if the documentation is stored in the same location, i.e. the same git repository, as the `catalog-info.yaml`. While it is still supported to add full URLs such as `backstage.io/techdocs-ref: url:https://...` for custom setups, documentation is mostly stored in the same repository as the entity definition. By automatically resolving the target relative to the registration location of the entity, the configuration overhead for this default setup is minimized. Since it leverages the `@backstage/integrations` package for the URL resolution, this is compatible with every supported source. Consider the following examples: -> Note that the short version `` is only an alias for the still supported `dir:`. - 1. "I have a repository with a single `catalog-info.yaml` and a TechDocs page in the root folder!" ``` @@ -29,7 +28,7 @@ https://github.com/backstage/example/tree/main/ | > metadata: | > name: example | > annotations: - | > backstage.io/techdocs-ref: . # -> same folder + | > backstage.io/techdocs-ref: dir:. # -> same folder | > spec: {} |- docs/ |- mkdocs.yml @@ -45,7 +44,7 @@ https://bitbucket.org/my-owner/my-project/src/master/ | > metadata: | > name: example | > annotations: - | > backstage.io/techdocs-ref: ./some-folder # -> subfolder + | > backstage.io/techdocs-ref: dir:./some-folder # -> subfolder | > spec: {} |- some-folder/ |- docs/ @@ -63,7 +62,7 @@ https://dev.azure.com/organization/project/_git/repository | > metadata: | > name: my-1st-module | > annotations: - | > backstage.io/techdocs-ref: . # -> same folder + | > backstage.io/techdocs-ref: dir:. # -> same folder | > spec: {} |- docs/ |- mkdocs.yml @@ -74,7 +73,7 @@ https://dev.azure.com/organization/project/_git/repository | > metadata: | > name: my-2nd-module | > annotations: - | > backstage.io/techdocs-ref: . # -> same folder + | > backstage.io/techdocs-ref: dir:. # -> same folder | > spec: {} |- docs/ |- mkdocs.yml diff --git a/packages/techdocs-common/api-report.md b/packages/techdocs-common/api-report.md index 9fc1795c13..6e845c9797 100644 --- a/packages/techdocs-common/api-report.md +++ b/packages/techdocs-common/api-report.md @@ -137,18 +137,6 @@ export const getDefaultBranch: ( config: Config, ) => Promise; -// Warning: (ae-missing-release-tag) "getDirLocation" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) -// -// @public -export const getDirLocation: ( - entity: Entity, -) => - | { - type: 'dir'; - target: string; - } - | undefined; - // Warning: (ae-missing-release-tag) "getDocFilesFromRepository" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) @@ -382,6 +370,7 @@ export type TechDocsMetadata = { // @public export const transformDirLocation: ( entity: Entity, + dirAnnotation: ParsedLocationAnnotation, scmIntegrations: ScmIntegrationRegistry, ) => { type: 'dir' | 'url'; diff --git a/packages/techdocs-common/src/helpers.test.ts b/packages/techdocs-common/src/helpers.test.ts index 6b677c6f5e..69f09d92ed 100644 --- a/packages/techdocs-common/src/helpers.test.ts +++ b/packages/techdocs-common/src/helpers.test.ts @@ -26,7 +26,6 @@ import os from 'os'; import path from 'path'; import { Readable } from 'stream'; import { - getDirLocation, getDocFilesFromRepository, getLocationForEntity, parseReferenceAnnotation, @@ -126,76 +125,11 @@ describe('parseReferenceAnnotation', () => { }); }); -describe('getDirLocation', () => { - it.each` - techdocsRef | responseTarget - ${undefined} | ${undefined} - ${16} | ${undefined} - ${'.'} | ${'.'} - ${'dir:.'} | ${'.'} - ${'./relative'} | ${'./relative'} - ${'dir:./relative'} | ${'./relative'} - ${'dir:https://github.com...'} | ${'https://github.com...'} - ${'url:https://github.com...'} | ${undefined} - `( - 'should handle "backstage.io/techdocs-ref: $techdocsRef" correctly', - ({ techdocsRef, responseTarget }) => { - const entity = { - apiVersion: '1', - kind: 'Component', - metadata: { - name: 'test', - annotations: { - 'backstage.io/techdocs-ref': techdocsRef, - }, - }, - }; - - const result = getDirLocation(entity); - - expect(result).toEqual( - responseTarget && { type: 'dir', target: responseTarget }, - ); - }, - ); - - it('Reject https urls and hint to the url: location type', async () => { - const entity = { - apiVersion: '1', - kind: 'Component', - metadata: { - name: 'test', - annotations: { - 'backstage.io/techdocs-ref': 'https://github.com/backstage/backstage', - }, - }, - }; - - expect(() => getDirLocation(entity)).toThrow( - /please prefix it with 'url:'/, - ); - }); -}); - describe('transformDirLocation', () => { - it('should reject missing annotation', () => { - const entity = { - apiVersion: '1', - kind: 'Component', - metadata: { - name: 'test', - }, - }; - - expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( - /No techdocs location annotation provided in entity: component:default\/test/, - ); - }); - it.each` - techdocsRef | target - ${'.'} | ${'https://my-url/folder/'} - ${'./sub-folder'} | ${'https://my-url/folder/sub-folder'} + techdocsRef | target + ${'dir:.'} | ${'https://my-url/folder/'} + ${'dir:./sub-folder'} | ${'https://my-url/folder/sub-folder'} `( 'should transform "$techdocsRef" for url type locations', ({ techdocsRef, target }) => { @@ -215,16 +149,20 @@ describe('transformDirLocation', () => { }, }; - const result = transformDirLocation(entity, scmIntegrations); + const result = transformDirLocation( + entity, + parseReferenceAnnotation('backstage.io/techdocs-ref', entity), + scmIntegrations, + ); expect(result).toEqual({ type: 'url', target }); }, ); it.each` - techdocsRef | target - ${'.'} | ${path.join(rootDir, 'working-copy')} - ${'./sub-folder'} | ${path.join(rootDir, 'working-copy', 'sub-folder')} + techdocsRef | target + ${'dir:.'} | ${path.join(rootDir, 'working-copy')} + ${'dir:./sub-folder'} | ${path.join(rootDir, 'working-copy', 'sub-folder')} `( 'should transform "$techdocsRef" for file type locations', ({ techdocsRef, target }) => { @@ -244,7 +182,11 @@ describe('transformDirLocation', () => { }, }; - const result = transformDirLocation(entity, scmIntegrations); + const result = transformDirLocation( + entity, + parseReferenceAnnotation('backstage.io/techdocs-ref', entity), + scmIntegrations, + ); expect(result).toEqual({ type: 'dir', target }); }, @@ -262,12 +204,18 @@ describe('transformDirLocation', () => { metadata: { name: 'test', annotations: { - 'backstage.io/techdocs-ref': '..', + 'backstage.io/techdocs-ref': 'dir:..', }, }, }; - expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( + expect(() => + transformDirLocation( + entity, + parseReferenceAnnotation('backstage.io/techdocs-ref', entity), + scmIntegrations, + ), + ).toThrow( /Relative path is not allowed to refer to a directory outside its parent/, ); }); @@ -284,43 +232,22 @@ describe('transformDirLocation', () => { metadata: { name: 'test', annotations: { - 'backstage.io/techdocs-ref': '.', + 'backstage.io/techdocs-ref': 'dir:.', }, }, }; - expect(() => transformDirLocation(entity, scmIntegrations)).toThrow( - /Unable to resolve location type other/, - ); + expect(() => + transformDirLocation( + entity, + parseReferenceAnnotation('backstage.io/techdocs-ref', entity), + scmIntegrations, + ), + ).toThrow(/Unable to resolve location type other/); }); }); describe('getLocationForEntity', () => { - it('should handle implicit dir locations', () => { - (getEntitySourceLocation as jest.Mock).mockReturnValue({ - type: 'url', - target: 'https://my-url/folder/', - }); - - const entity = { - apiVersion: '1', - kind: 'Component', - metadata: { - name: 'test', - annotations: { - 'backstage.io/techdocs-ref': '.', - }, - }, - }; - - const parsedLocationAnnotation = getLocationForEntity( - entity, - scmIntegrations, - ); - expect(parsedLocationAnnotation.type).toBe('url'); - expect(parsedLocationAnnotation.target).toBe('https://my-url/folder/'); - }); - it('should handle dir locations', () => { (getEntitySourceLocation as jest.Mock).mockReturnValue({ type: 'url', diff --git a/packages/techdocs-common/src/helpers.ts b/packages/techdocs-common/src/helpers.ts index bb640c1cce..bb6461a37c 100644 --- a/packages/techdocs-common/src/helpers.ts +++ b/packages/techdocs-common/src/helpers.ts @@ -23,7 +23,6 @@ import { Entity, getEntitySourceLocation, parseLocationReference, - stringifyEntityRef, } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { InputError } from '@backstage/errors'; @@ -60,44 +59,6 @@ export const parseReferenceAnnotation = ( }; }; -/** - * Check if the entity provides a `backstage.io/techdocs-ref` annotation of type `dir` - * and return the annotation. It accepts the two variants `` and `dir:`. - * - * @param entity - the entity to check - */ -export const getDirLocation = ( - entity: Entity, -): { type: 'dir'; target: string } | undefined => { - const annotation = entity.metadata.annotations?.['backstage.io/techdocs-ref']; - - if (!annotation) { - return undefined; - } - - if (typeof annotation !== 'string') { - return undefined; - } - - // if the string doesn't contain `:`, interpret it as the target of a dir type - if (!annotation.includes(':')) { - return { type: 'dir', target: annotation }; - } - - // note that `backstage.io/techdocs-ref: https://...` is invalid and will throw - const reference = parseLocationReference(annotation); - - if (reference.type === 'dir') { - return { - type: 'dir', - target: reference.target, - }; - } - - // ignore any other types - return undefined; -}; - /** * TechDocs references of type `dir` are relative the source location of the entity. * This function transforms relative references to absolute ones, based on the @@ -107,30 +68,22 @@ export const getDirLocation = ( * an absolute `dir` location. * * @param entity - the entity with annotations - * @param scmIntegrations - access to the scmIntegrationt to do url transformations + * @param dirAnnotation - the parsed techdocs-ref annotation of type 'dir' + * @param scmIntegrations - access to the scmIntegration to do url transformations * @throws if the entity doesn't specify a `dir` location or is ingested from an unsupported location. * @returns the transformed location with an absolute target. */ export const transformDirLocation = ( entity: Entity, + dirAnnotation: ParsedLocationAnnotation, scmIntegrations: ScmIntegrationRegistry, ): { type: 'dir' | 'url'; target: string } => { - const dirLocation = getDirLocation(entity); - - if (!dirLocation) { - throw new InputError( - `No techdocs location annotation provided in entity: ${stringifyEntityRef( - entity, - )}`, - ); - } - const location = getEntitySourceLocation(entity); switch (location.type) { case 'url': { const target = scmIntegrations.resolveUrl({ - url: dirLocation.target, + url: dirAnnotation.target, base: location.target, }); @@ -144,7 +97,7 @@ export const transformDirLocation = ( // only permit targets in the same folder as the target of the `file` location! const target = resolveSafeChildPath( path.dirname(location.target), - dirLocation.target, + dirAnnotation.target, ); return { @@ -162,24 +115,21 @@ export const getLocationForEntity = ( entity: Entity, scmIntegration: ScmIntegrationRegistry, ): ParsedLocationAnnotation => { - // try to resolve relative references first - if (getDirLocation(entity) !== undefined) { - return transformDirLocation(entity, scmIntegration); - } - - const { type, target } = parseReferenceAnnotation( + const annotation = parseReferenceAnnotation( 'backstage.io/techdocs-ref', entity, ); - switch (type) { + switch (annotation.type) { case 'github': case 'gitlab': case 'azure/api': case 'url': - return { type, target }; + return annotation; + case 'dir': + return transformDirLocation(entity, annotation, scmIntegration); default: - throw new Error(`Invalid reference annotation ${type}`); + throw new Error(`Invalid reference annotation ${annotation.type}`); } }; diff --git a/packages/techdocs-common/src/stages/prepare/dir.ts b/packages/techdocs-common/src/stages/prepare/dir.ts index b0ba2a36b3..ade0b00162 100644 --- a/packages/techdocs-common/src/stages/prepare/dir.ts +++ b/packages/techdocs-common/src/stages/prepare/dir.ts @@ -23,7 +23,7 @@ import { ScmIntegrations, } from '@backstage/integration'; import { Logger } from 'winston'; -import { transformDirLocation } from '../../helpers'; +import { parseReferenceAnnotation, transformDirLocation } from '../../helpers'; import { PreparerBase, PreparerResponse } from './types'; export class DirectoryPreparer implements PreparerBase { @@ -39,18 +39,29 @@ export class DirectoryPreparer implements PreparerBase { entity: Entity, options?: { logger?: Logger; etag?: string }, ): Promise { - const { type, target } = transformDirLocation(entity, this.scmIntegrations); + const annotation = parseReferenceAnnotation( + 'backstage.io/techdocs-ref', + entity, + ); + const { type, target } = transformDirLocation( + entity, + annotation, + this.scmIntegrations, + ); switch (type) { case 'url': { - options?.logger?.info(`Download documentation from ${target}`); + options?.logger?.debug(`Reading files from ${target}`); // the target is an absolute url since it has already been transformed const response = await this.reader.readTree(target, { etag: options?.etag, }); + const preparedDir = await response.dir(); + + options?.logger?.debug(`Tree downloaded and stored at ${preparedDir}`); return { - preparedDir: await response.dir(), + preparedDir, etag: response.etag, }; } diff --git a/packages/techdocs-common/src/stages/prepare/preparers.ts b/packages/techdocs-common/src/stages/prepare/preparers.ts index 48e99d48f5..12fcb01013 100644 --- a/packages/techdocs-common/src/stages/prepare/preparers.ts +++ b/packages/techdocs-common/src/stages/prepare/preparers.ts @@ -15,11 +15,10 @@ */ import { UrlReader } from '@backstage/backend-common'; -import { Entity, parseLocationReference } from '@backstage/catalog-model'; +import { Entity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import { InputError } from '@backstage/errors'; import { Logger } from 'winston'; -import { getDirLocation } from '../../helpers'; +import { parseReferenceAnnotation } from '../../helpers'; import { CommonGitPreparer } from './commonGit'; import { DirectoryPreparer } from './dir'; import { PreparerBase, PreparerBuilder, RemoteProtocol } from './types'; @@ -63,20 +62,10 @@ export class Preparers implements PreparerBuilder { } get(entity: Entity): PreparerBase { - const annotation = - entity.metadata.annotations?.['backstage.io/techdocs-ref']; - if (!annotation) { - throw new InputError( - `No location annotation provided in entity: ${entity.metadata.name}`, - ); - } - - // the dir processor handles both `` and `dir:` - if (getDirLocation(entity) !== undefined) { - return this.preparerMap.get('dir')!; - } - - const { type } = parseLocationReference(annotation); + const { type } = parseReferenceAnnotation( + 'backstage.io/techdocs-ref', + entity, + ); const preparer = this.preparerMap.get(type as RemoteProtocol); if (!preparer) { From 296d8e3832774ad92629f65dd208e00d24b157db Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 21 Jul 2021 15:18:22 +0200 Subject: [PATCH 22/29] Delete obsolete changes Signed-off-by: Dominik Henneke --- packages/techdocs-common/src/stages/prepare/preparers.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/techdocs-common/src/stages/prepare/preparers.ts b/packages/techdocs-common/src/stages/prepare/preparers.ts index 12fcb01013..5c78d96f15 100644 --- a/packages/techdocs-common/src/stages/prepare/preparers.ts +++ b/packages/techdocs-common/src/stages/prepare/preparers.ts @@ -13,16 +13,15 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - import { UrlReader } from '@backstage/backend-common'; import { Entity } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; import { Logger } from 'winston'; import { parseReferenceAnnotation } from '../../helpers'; -import { CommonGitPreparer } from './commonGit'; import { DirectoryPreparer } from './dir'; -import { PreparerBase, PreparerBuilder, RemoteProtocol } from './types'; +import { CommonGitPreparer } from './commonGit'; import { UrlPreparer } from './url'; +import { PreparerBase, PreparerBuilder, RemoteProtocol } from './types'; type factoryOptions = { logger: Logger; @@ -66,7 +65,7 @@ export class Preparers implements PreparerBuilder { 'backstage.io/techdocs-ref', entity, ); - const preparer = this.preparerMap.get(type as RemoteProtocol); + const preparer = this.preparerMap.get(type); if (!preparer) { throw new Error(`No preparer registered for type: "${type}"`); From c914db284f948c69d675a884b69a1c2c6663804f Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Wed, 14 Jul 2021 17:17:39 +0100 Subject: [PATCH 23/29] feat(scaffolder): add gha dispatch workflow action Signed-off-by: Andrea Falzetti --- .../builtin/ci/githubActionsDispatch.test.ts | 134 ++++++++++++++++++ .../builtin/ci/githubActionsDispatch.ts | 124 ++++++++++++++++ .../scaffolder/actions/builtin/ci/index.ts | 17 +++ .../actions/builtin/createBuiltinActions.ts | 5 + 4 files changed, 280 insertions(+) create mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts create mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts create mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts new file mode 100644 index 0000000000..606881e47a --- /dev/null +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts @@ -0,0 +1,134 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +jest.mock('@octokit/rest'); +jest.mock('@backstage/integration'); + +import { createGithubActionsDispatchAction } from './githubActionsDispatch'; +import { ScmIntegrations } from '@backstage/integration'; +import { ConfigReader } from '@backstage/config'; +import { getVoidLogger } from '@backstage/backend-common'; +import { PassThrough } from 'stream'; +// import { when } from 'jest-when'; + +describe('ci:github-actions-dispatch', () => { + const config = new ConfigReader({ + integrations: { + github: [ + { host: 'github.com', token: 'tokenlols' }, + { host: 'ghe.github.com' }, + ], + }, + }); + + const integrations = ScmIntegrations.fromConfig(config); + const action = createGithubActionsDispatchAction({ integrations, config }); + + const mockContext = { + input: { + owner: 'a-owner', + repoUrl: 'github.com?repo=repo&owner=owner', + repoName: 'repository-name', + workflowId: 'a-workflow-id', + branchOrTagName: 'main', + }, + workspacePath: 'lol', + logger: getVoidLogger(), + logStream: new PassThrough(), + output: jest.fn(), + createTemporaryDirectory: jest.fn(), + }; + + const { mockGithubClient } = require('@octokit/rest'); + mockGithubClient.rest = { + actions: { + createWorkflowDispatch: jest.fn(), + }, + }; + + beforeEach(() => { + jest.resetAllMocks(); + }); + + // it('should throw if there is no integration config provided', async () => { + // const actionWithNoIntegrations = createGithubActionsDispatchAction({ + // integrations: { + // ...integrations, + // github: { + // list: () => [], + // byHost: jest.fn(), + // byUrl: jest.fn(), + // }, + // }, + // config, + // }); + // await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( + // /No matching integration configuration/, + // ); + // }); + + // it('should throw if there is no token in the integration config that is returned', async () => { + // const mockedProvider = jest.fn(); + // const mockedIntegrationConfig = jest.fn(); + // GithubCredentialsProvider.create.mockReturnOnce(mockedProvider); + // const mockedArray = [ + // { + // config: { + // host: 'github.com', + // }, + // }, + // ] as GitHubIntegration[]; + // const integrations1 = { + // github: { + // list: () => mockedArray, + // byHost: mockedIntegrationConfig, + // byUrl: jest.fn(), + // }, + // } as ScmIntegrationRegistry; + // const actionWithNoIntegrations = createGithubActionsDispatchAction({ + // integrations: integrations1, + // config, + // }); + // mockedProvider.mockResolvedValue(undefined); + + // await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( + // /No token available for host/, + // ); + // }); + + it('should call the githubApis for creating WorkflowDispatch', async () => { + mockGithubClient.rest.actions.createWorkflowDispatch.mockResolvedValue({ + message: 'Success', + }); + const owner = 'dx'; + const repoName = 'test1'; + const workflowId = 'dispatch_workflow'; + const branchOrTagName = 'main'; + const ctx = Object.assign({}, mockContext, { + input: { owner, repoName, workflowId, branchOrTagName }, + }); + await action.handler(ctx); + + expect( + mockGithubClient.rest.actions.createWorkflowDispatch, + ).toHaveBeenCalledWith({ + owner, + repo: repoName, + workflow_id: workflowId, + ref: branchOrTagName, + }); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts new file mode 100644 index 0000000000..fbeeb4c9ac --- /dev/null +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts @@ -0,0 +1,124 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { InputError } from '@backstage/errors'; +import { + GithubCredentialsProvider, + ScmIntegrationRegistry, +} from '@backstage/integration'; +import { Octokit } from '@octokit/rest'; +import { createTemplateAction } from '../../createTemplateAction'; +import { Config } from '@backstage/config'; + +const host = 'github.com'; + +export function createGithubActionsDispatchAction(options: { + integrations: ScmIntegrationRegistry; + config: Config; +}) { + const { integrations } = options; + + const credentialsProviders = new Map( + integrations.github.list().map(integration => { + const provider = GithubCredentialsProvider.create(integration.config); + return [integration.config.host, provider]; + }), + ); + + return createTemplateAction<{ + owner: string; + repoName: string; + workflowId: string; + branchOrTagName: string; + }>({ + id: 'ci:github-actions-dispatch', + description: + 'Dispatches a GitHub Action workflow for a given branch or tag', + schema: { + input: { + type: 'object', + required: ['owner', 'repoName', 'workflowId', 'branchOrTagName'], + properties: { + owner: { + title: 'Repository Owner', + description: 'GitHub Org or User name owner of the repository', + type: 'string', + }, + repoName: { + title: 'Repository Name', + description: `Repo`, + type: 'string', + }, + workflowId: { + title: 'Workflow ID', + description: 'The GitHub Action Workflow filename', + type: 'string', + }, + branchOrTagName: { + title: 'Branch or Tag name', + description: + 'The git branch or tag name used to dispatch the workflow', + type: 'string', + }, + }, + }, + }, + async handler(ctx) { + const { owner, repoName, workflowId, branchOrTagName } = ctx.input; + + ctx.logger.info( + `Dispatching workflow ${workflowId} for repo ${owner}/${repoName} on ${branchOrTagName}`, + ); + + const credentialsProvider = credentialsProviders.get(host); + const integrationConfig = integrations.github.byHost(host); + + if (!credentialsProvider || !integrationConfig) { + throw new InputError( + `No matching integration configuration for host ${host}, please check your integrations config`, + ); + } + + // TODO(blam): Consider changing this API to have owner, repo interface instead of URL as the it's + // needless to create URL and then parse again the other side. + const { token } = await credentialsProvider.getCredentials({ + url: `https://${host}/${encodeURIComponent(owner)}/${encodeURIComponent( + repoName, + )}`, + }); + + if (!token) { + throw new InputError( + `No token available for host: ${host}, with owner ${owner}, and repo ${repoName}`, + ); + } + + const client = new Octokit({ + auth: ctx.token ?? token, + baseUrl: integrationConfig.config.apiBaseUrl, + previews: ['nebula-preview'], + }); + + await client.rest.actions.createWorkflowDispatch({ + owner, + repo, + workflow_id: workflowId, + ref: branchOrTagName, + }); + + ctx.logger.info(`Workflow ${workflowId} dispatched successfully`); + }, + }); +} diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts new file mode 100644 index 0000000000..50abd2c3dc --- /dev/null +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts @@ -0,0 +1,17 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export { createGithubActionsDispatchAction } from './githubActionsDispatch'; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts index ae5a7d0697..48c13436fa 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts @@ -37,6 +37,7 @@ import { createPublishGithubPullRequestAction, createPublishGitlabAction, } from './publish'; +import { createGithubActionsDispatchAction } from './ci'; export const createBuiltinActions = (options: { reader: UrlReader; @@ -91,5 +92,9 @@ export const createBuiltinActions = (options: { createCatalogWriteAction(), createFilesystemDeleteAction(), createFilesystemRenameAction(), + createGithubActionsDispatchAction({ + integrations, + config, + }), ]; }; From a370ecab04a59c4a50ba6e92b9d321532a53e240 Mon Sep 17 00:00:00 2001 From: Yogesh Lonkar Date: Thu, 15 Jul 2021 13:17:05 +0200 Subject: [PATCH 24/29] fix unit test Signed-off-by: Yogesh Lonkar Signed-off-by: Andrea Falzetti --- .../builtin/ci/githubActionsDispatch.test.ts | 157 +++++++++++------- .../builtin/ci/githubActionsDispatch.ts | 3 +- 2 files changed, 97 insertions(+), 63 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts index 606881e47a..6564671664 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts @@ -13,16 +13,21 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +// @ts-nocheck -jest.mock('@octokit/rest'); jest.mock('@backstage/integration'); +jest.mock('@octokit/rest', () => ({ Octokit: jest.fn() })); import { createGithubActionsDispatchAction } from './githubActionsDispatch'; -import { ScmIntegrations } from '@backstage/integration'; +import { + ScmIntegrations, + GithubCredentialsProvider, +} from '@backstage/integration'; import { ConfigReader } from '@backstage/config'; import { getVoidLogger } from '@backstage/backend-common'; import { PassThrough } from 'stream'; -// import { when } from 'jest-when'; + +import { Octokit } from '@octokit/rest'; describe('ci:github-actions-dispatch', () => { const config = new ConfigReader({ @@ -34,9 +39,6 @@ describe('ci:github-actions-dispatch', () => { }, }); - const integrations = ScmIntegrations.fromConfig(config); - const action = createGithubActionsDispatchAction({ integrations, config }); - const mockContext = { input: { owner: 'a-owner', @@ -52,67 +54,101 @@ describe('ci:github-actions-dispatch', () => { createTemporaryDirectory: jest.fn(), }; - const { mockGithubClient } = require('@octokit/rest'); - mockGithubClient.rest = { - actions: { - createWorkflowDispatch: jest.fn(), - }, - }; - beforeEach(() => { jest.resetAllMocks(); }); - // it('should throw if there is no integration config provided', async () => { - // const actionWithNoIntegrations = createGithubActionsDispatchAction({ - // integrations: { - // ...integrations, - // github: { - // list: () => [], - // byHost: jest.fn(), - // byUrl: jest.fn(), - // }, - // }, - // config, - // }); - // await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( - // /No matching integration configuration/, - // ); - // }); + it('should throw if there is no integration config provided', async () => { + const integrations = { + github: { + list: () => [], + byHost: jest.fn(), + byUrl: jest.fn(), + }, + } as ScmIntegrations; + const actionWithNoIntegrations = createGithubActionsDispatchAction({ + integrations, + config, + }); + await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( + /No matching integration configuration/, + ); + }); - // it('should throw if there is no token in the integration config that is returned', async () => { - // const mockedProvider = jest.fn(); - // const mockedIntegrationConfig = jest.fn(); - // GithubCredentialsProvider.create.mockReturnOnce(mockedProvider); - // const mockedArray = [ - // { - // config: { - // host: 'github.com', - // }, - // }, - // ] as GitHubIntegration[]; - // const integrations1 = { - // github: { - // list: () => mockedArray, - // byHost: mockedIntegrationConfig, - // byUrl: jest.fn(), - // }, - // } as ScmIntegrationRegistry; - // const actionWithNoIntegrations = createGithubActionsDispatchAction({ - // integrations: integrations1, - // config, - // }); - // mockedProvider.mockResolvedValue(undefined); - - // await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( - // /No token available for host/, - // ); - // }); + it('should throw if no token is provided', async () => { + const integrations = { + github: { + list: () => [ + { + config: { + host: 'github.com', + }, + }, + ], + byHost: () => ({ + config: { + apiBaseUrl: 'https://api.github.com', + }, + }), + byUrl: jest.fn(), + }, + } as ScmIntegrations; + const mockedCredentialsProvider = { + getCredentials: jest.fn(), + }; + mockedCredentialsProvider.getCredentials.mockResolvedValue({}); + GithubCredentialsProvider.create.mockReturnValueOnce( + mockedCredentialsProvider, + ); + const actionWithNoIntegrations = createGithubActionsDispatchAction({ + integrations, + config, + }); + await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( + /No token available for host/, + ); + }); it('should call the githubApis for creating WorkflowDispatch', async () => { - mockGithubClient.rest.actions.createWorkflowDispatch.mockResolvedValue({ + const integrations = { + github: { + list: () => [ + { + config: { + host: 'github.com', + }, + }, + ], + byHost: () => ({ + config: { + apiBaseUrl: 'https://api.github.com', + }, + }), + byUrl: jest.fn(), + }, + } as ScmIntegrations; + const mockedCredentialsProvider = { + getCredentials: jest.fn(), + }; + mockedCredentialsProvider.getCredentials.mockResolvedValue({ + token: 'test-token', + }); + GithubCredentialsProvider.create.mockReturnValueOnce( + mockedCredentialsProvider, + ); + const mockedCreateWorkflowDispatch = jest.fn(); + mockedCreateWorkflowDispatch.mockResolvedValue({ message: 'Success', }); + Octokit.mockImplementation(() => { + return { + rest: { + actions: { + createWorkflowDispatch: mockedCreateWorkflowDispatch, + }, + }, + }; + }); const owner = 'dx'; const repoName = 'test1'; const workflowId = 'dispatch_workflow'; @@ -120,11 +156,10 @@ describe('ci:github-actions-dispatch', () => { const ctx = Object.assign({}, mockContext, { input: { owner, repoName, workflowId, branchOrTagName }, }); + const action = createGithubActionsDispatchAction({ integrations, config }); await action.handler(ctx); - expect( - mockGithubClient.rest.actions.createWorkflowDispatch, - ).toHaveBeenCalledWith({ + expect(mockedCreateWorkflowDispatch).toHaveBeenCalledWith({ owner, repo: repoName, workflow_id: workflowId, diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts index fbeeb4c9ac..e720695b13 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts @@ -104,7 +104,6 @@ export function createGithubActionsDispatchAction(options: { `No token available for host: ${host}, with owner ${owner}, and repo ${repoName}`, ); } - const client = new Octokit({ auth: ctx.token ?? token, baseUrl: integrationConfig.config.apiBaseUrl, @@ -113,7 +112,7 @@ export function createGithubActionsDispatchAction(options: { await client.rest.actions.createWorkflowDispatch({ owner, - repo, + repo: repoName, workflow_id: workflowId, ref: branchOrTagName, }); From c73f53bc2f0eeec75e28c7633ce1b38421e3242f Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Thu, 15 Jul 2021 15:29:30 +0100 Subject: [PATCH 25/29] docs: add changeset Signed-off-by: Andrea Falzetti --- .changeset/spotty-actors-invite.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/spotty-actors-invite.md diff --git a/.changeset/spotty-actors-invite.md b/.changeset/spotty-actors-invite.md new file mode 100644 index 0000000000..96f179c475 --- /dev/null +++ b/.changeset/spotty-actors-invite.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Add new built-in action ci:github-actions-dispatch From 2ca8855b0d5257f903e63ed527f889f50cb69633 Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Fri, 16 Jul 2021 09:31:36 +0100 Subject: [PATCH 26/29] fix: remove ctx.token Signed-off-by: Andrea Falzetti --- .../src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts index e720695b13..99e745f852 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts @@ -105,7 +105,7 @@ export function createGithubActionsDispatchAction(options: { ); } const client = new Octokit({ - auth: ctx.token ?? token, + auth: token, baseUrl: integrationConfig.config.apiBaseUrl, previews: ['nebula-preview'], }); From f928a8f9bfcf1ecd84404ca2c0d62ab7b5be8fc4 Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Fri, 16 Jul 2021 09:44:53 +0100 Subject: [PATCH 27/29] chore: remove comment Signed-off-by: Andrea Falzetti --- .../src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts index 99e745f852..d7a5a26ccb 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts @@ -91,8 +91,6 @@ export function createGithubActionsDispatchAction(options: { ); } - // TODO(blam): Consider changing this API to have owner, repo interface instead of URL as the it's - // needless to create URL and then parse again the other side. const { token } = await credentialsProvider.getCredentials({ url: `https://${host}/${encodeURIComponent(owner)}/${encodeURIComponent( repoName, @@ -104,6 +102,7 @@ export function createGithubActionsDispatchAction(options: { `No token available for host: ${host}, with owner ${owner}, and repo ${repoName}`, ); } + const client = new Octokit({ auth: token, baseUrl: integrationConfig.config.apiBaseUrl, From 8fb0874dd44c1098d0120fb9e93bb26e9995f250 Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Wed, 21 Jul 2021 16:50:01 +0100 Subject: [PATCH 28/29] refactor: use repoUrl Signed-off-by: Andrea Falzetti --- .../__mocks__/@octokit/rest/index.ts | 5 + .../builtin/ci/githubActionsDispatch.test.ts | 169 ------------------ .../actions/builtin/createBuiltinActions.ts | 3 +- .../github/githubActionsDispatch.test.ts | 117 ++++++++++++ .../{ci => github}/githubActionsDispatch.ts | 35 ++-- .../actions/builtin/{ci => github}/index.ts | 0 .../src/scaffolder/actions/builtin/index.ts | 2 +- 7 files changed, 138 insertions(+), 193 deletions(-) delete mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts create mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.test.ts rename plugins/scaffolder-backend/src/scaffolder/actions/builtin/{ci => github}/githubActionsDispatch.ts (80%) rename plugins/scaffolder-backend/src/scaffolder/actions/builtin/{ci => github}/index.ts (100%) diff --git a/plugins/scaffolder-backend/src/scaffolder/__mocks__/@octokit/rest/index.ts b/plugins/scaffolder-backend/src/scaffolder/__mocks__/@octokit/rest/index.ts index 75cdfc208f..7505c3a30e 100644 --- a/plugins/scaffolder-backend/src/scaffolder/__mocks__/@octokit/rest/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/__mocks__/@octokit/rest/index.ts @@ -15,6 +15,11 @@ */ export const mockGithubClient = { + rest: { + actions: { + createWorkflowDispatch: jest.fn(), + }, + }, repos: { createInOrg: jest.fn(), createForAuthenticatedUser: jest.fn(), diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts deleted file mode 100644 index 6564671664..0000000000 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.test.ts +++ /dev/null @@ -1,169 +0,0 @@ -/* - * Copyright 2021 The Backstage Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -// @ts-nocheck - -jest.mock('@backstage/integration'); -jest.mock('@octokit/rest', () => ({ Octokit: jest.fn() })); - -import { createGithubActionsDispatchAction } from './githubActionsDispatch'; -import { - ScmIntegrations, - GithubCredentialsProvider, -} from '@backstage/integration'; -import { ConfigReader } from '@backstage/config'; -import { getVoidLogger } from '@backstage/backend-common'; -import { PassThrough } from 'stream'; - -import { Octokit } from '@octokit/rest'; - -describe('ci:github-actions-dispatch', () => { - const config = new ConfigReader({ - integrations: { - github: [ - { host: 'github.com', token: 'tokenlols' }, - { host: 'ghe.github.com' }, - ], - }, - }); - - const mockContext = { - input: { - owner: 'a-owner', - repoUrl: 'github.com?repo=repo&owner=owner', - repoName: 'repository-name', - workflowId: 'a-workflow-id', - branchOrTagName: 'main', - }, - workspacePath: 'lol', - logger: getVoidLogger(), - logStream: new PassThrough(), - output: jest.fn(), - createTemporaryDirectory: jest.fn(), - }; - - beforeEach(() => { - jest.resetAllMocks(); - }); - - it('should throw if there is no integration config provided', async () => { - const integrations = { - github: { - list: () => [], - byHost: jest.fn(), - byUrl: jest.fn(), - }, - } as ScmIntegrations; - const actionWithNoIntegrations = createGithubActionsDispatchAction({ - integrations, - config, - }); - await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( - /No matching integration configuration/, - ); - }); - - it('should throw if no token is provided', async () => { - const integrations = { - github: { - list: () => [ - { - config: { - host: 'github.com', - }, - }, - ], - byHost: () => ({ - config: { - apiBaseUrl: 'https://api.github.com', - }, - }), - byUrl: jest.fn(), - }, - } as ScmIntegrations; - const mockedCredentialsProvider = { - getCredentials: jest.fn(), - }; - mockedCredentialsProvider.getCredentials.mockResolvedValue({}); - GithubCredentialsProvider.create.mockReturnValueOnce( - mockedCredentialsProvider, - ); - const actionWithNoIntegrations = createGithubActionsDispatchAction({ - integrations, - config, - }); - await expect(actionWithNoIntegrations.handler(mockContext)).rejects.toThrow( - /No token available for host/, - ); - }); - - it('should call the githubApis for creating WorkflowDispatch', async () => { - const integrations = { - github: { - list: () => [ - { - config: { - host: 'github.com', - }, - }, - ], - byHost: () => ({ - config: { - apiBaseUrl: 'https://api.github.com', - }, - }), - byUrl: jest.fn(), - }, - } as ScmIntegrations; - const mockedCredentialsProvider = { - getCredentials: jest.fn(), - }; - mockedCredentialsProvider.getCredentials.mockResolvedValue({ - token: 'test-token', - }); - GithubCredentialsProvider.create.mockReturnValueOnce( - mockedCredentialsProvider, - ); - const mockedCreateWorkflowDispatch = jest.fn(); - mockedCreateWorkflowDispatch.mockResolvedValue({ - message: 'Success', - }); - Octokit.mockImplementation(() => { - return { - rest: { - actions: { - createWorkflowDispatch: mockedCreateWorkflowDispatch, - }, - }, - }; - }); - const owner = 'dx'; - const repoName = 'test1'; - const workflowId = 'dispatch_workflow'; - const branchOrTagName = 'main'; - const ctx = Object.assign({}, mockContext, { - input: { owner, repoName, workflowId, branchOrTagName }, - }); - const action = createGithubActionsDispatchAction({ integrations, config }); - await action.handler(ctx); - - expect(mockedCreateWorkflowDispatch).toHaveBeenCalledWith({ - owner, - repo: repoName, - workflow_id: workflowId, - ref: branchOrTagName, - }); - }); -}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts index 48c13436fa..b3d1460270 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/createBuiltinActions.ts @@ -37,7 +37,7 @@ import { createPublishGithubPullRequestAction, createPublishGitlabAction, } from './publish'; -import { createGithubActionsDispatchAction } from './ci'; +import { createGithubActionsDispatchAction } from './github'; export const createBuiltinActions = (options: { reader: UrlReader; @@ -94,7 +94,6 @@ export const createBuiltinActions = (options: { createFilesystemRenameAction(), createGithubActionsDispatchAction({ integrations, - config, }), ]; }; diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.test.ts new file mode 100644 index 0000000000..29e4338a39 --- /dev/null +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.test.ts @@ -0,0 +1,117 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +jest.mock('@octokit/rest'); + +import { createGithubActionsDispatchAction } from './githubActionsDispatch'; +import { ScmIntegrations } from '@backstage/integration'; +import { ConfigReader } from '@backstage/config'; +import { getVoidLogger } from '@backstage/backend-common'; +import { PassThrough } from 'stream'; + +describe('github:actions:dispatch', () => { + const config = new ConfigReader({ + integrations: { + github: [ + { host: 'github.com', token: 'tokenlols' }, + { host: 'ghe.github.com' }, + ], + }, + }); + + const integrations = ScmIntegrations.fromConfig(config); + const action = createGithubActionsDispatchAction({ integrations }); + + const mockContext = { + input: { + repoUrl: 'github.com?repo=repo&owner=owner', + workflowId: 'a-workflow-id', + branchOrTagName: 'main', + }, + workspacePath: 'lol', + logger: getVoidLogger(), + logStream: new PassThrough(), + output: jest.fn(), + createTemporaryDirectory: jest.fn(), + }; + + const { mockGithubClient } = require('@octokit/rest'); + + beforeEach(() => { + jest.resetAllMocks(); + }); + + it('should throw an error when the repoUrl is not well formed', async () => { + await expect( + action.handler({ + ...mockContext, + input: { repoUrl: 'github.com?repo=bob' }, + }), + ).rejects.toThrow(/missing owner/); + + await expect( + action.handler({ + ...mockContext, + input: { repoUrl: 'github.com?owner=owner' }, + }), + ).rejects.toThrow(/missing repo/); + }); + + it('should throw if there is no integration config provided', async () => { + await expect( + action.handler({ + ...mockContext, + input: { repoUrl: 'missing.com?repo=bob&owner=owner' }, + }), + ).rejects.toThrow(/No matching integration configuration/); + }); + + it('should throw if there is no token in the integration config that is returned', async () => { + await expect( + action.handler({ + ...mockContext, + input: { + repoUrl: 'ghe.github.com?repo=bob&owner=owner', + }, + }), + ).rejects.toThrow(/No token available for host/); + }); + + it('should call the githubApis for creating WorkflowDispatch', async () => { + mockGithubClient.rest.actions.createWorkflowDispatch.mockResolvedValue({ + data: { + foo: 'bar', + }, + }); + + const repoUrl = 'github.com?repo=repo&owner=owner'; + const workflowId = 'dispatch_workflow'; + const branchOrTagName = 'main'; + const ctx = Object.assign({}, mockContext, { + input: { repoUrl, workflowId, branchOrTagName }, + }); + await action.handler(ctx); + + expect( + mockGithubClient.rest.actions.createWorkflowDispatch, + ).toHaveBeenCalledWith({ + owner: 'owner', + repo: 'repo', + workflow_id: workflowId, + ref: branchOrTagName, + }); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts similarity index 80% rename from plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts rename to plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts index d7a5a26ccb..7f48a155ed 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/githubActionsDispatch.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/githubActionsDispatch.ts @@ -19,14 +19,11 @@ import { ScmIntegrationRegistry, } from '@backstage/integration'; import { Octokit } from '@octokit/rest'; +import { parseRepoUrl } from '../publish/util'; import { createTemplateAction } from '../../createTemplateAction'; -import { Config } from '@backstage/config'; - -const host = 'github.com'; export function createGithubActionsDispatchAction(options: { integrations: ScmIntegrationRegistry; - config: Config; }) { const { integrations } = options; @@ -38,27 +35,21 @@ export function createGithubActionsDispatchAction(options: { ); return createTemplateAction<{ - owner: string; - repoName: string; + repoUrl: string; workflowId: string; branchOrTagName: string; }>({ - id: 'ci:github-actions-dispatch', + id: 'github:actions:dispatch', description: 'Dispatches a GitHub Action workflow for a given branch or tag', schema: { input: { type: 'object', - required: ['owner', 'repoName', 'workflowId', 'branchOrTagName'], + required: ['repoUrl', 'workflowId', 'branchOrTagName'], properties: { - owner: { - title: 'Repository Owner', - description: 'GitHub Org or User name owner of the repository', - type: 'string', - }, - repoName: { - title: 'Repository Name', - description: `Repo`, + repoUrl: { + title: 'Repository Location', + description: `Accepts the format 'github.com?repo=reponame&owner=owner' where 'reponame' is the new repository name and 'owner' is an organization or username`, type: 'string', }, workflowId: { @@ -76,10 +67,12 @@ export function createGithubActionsDispatchAction(options: { }, }, async handler(ctx) { - const { owner, repoName, workflowId, branchOrTagName } = ctx.input; + const { repoUrl, workflowId, branchOrTagName } = ctx.input; + + const { owner, repo, host } = parseRepoUrl(repoUrl); ctx.logger.info( - `Dispatching workflow ${workflowId} for repo ${owner}/${repoName} on ${branchOrTagName}`, + `Dispatching workflow ${workflowId} for repo ${repoUrl} on ${branchOrTagName}`, ); const credentialsProvider = credentialsProviders.get(host); @@ -93,13 +86,13 @@ export function createGithubActionsDispatchAction(options: { const { token } = await credentialsProvider.getCredentials({ url: `https://${host}/${encodeURIComponent(owner)}/${encodeURIComponent( - repoName, + repo, )}`, }); if (!token) { throw new InputError( - `No token available for host: ${host}, with owner ${owner}, and repo ${repoName}`, + `No token available for host: ${host}, with owner ${owner}, and repo ${repo}`, ); } @@ -111,7 +104,7 @@ export function createGithubActionsDispatchAction(options: { await client.rest.actions.createWorkflowDispatch({ owner, - repo: repoName, + repo, workflow_id: workflowId, ref: branchOrTagName, }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/index.ts similarity index 100% rename from plugins/scaffolder-backend/src/scaffolder/actions/builtin/ci/index.ts rename to plugins/scaffolder-backend/src/scaffolder/actions/builtin/github/index.ts diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts index 2a583f369a..02c15abab1 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts @@ -20,6 +20,6 @@ export * from './debug'; export * from './fetch'; export * from './filesystem'; export * from './publish'; - export { createFetchCookiecutterAction } from '@backstage/plugin-scaffolder-backend-module-cookiecutter'; +export * from './github'; export { runCommand } from './helpers'; From 8db7ea4c2fde31dacb31240fbae2335c43760da1 Mon Sep 17 00:00:00 2001 From: Andrea Falzetti Date: Wed, 21 Jul 2021 17:14:09 +0100 Subject: [PATCH 29/29] docs: add api-reports Signed-off-by: Andrea Falzetti --- plugins/scaffolder-backend/api-report.md | 7 +++++++ .../src/scaffolder/actions/builtin/index.ts | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/plugins/scaffolder-backend/api-report.md b/plugins/scaffolder-backend/api-report.md index 0e7a25534c..baf85bce63 100644 --- a/plugins/scaffolder-backend/api-report.md +++ b/plugins/scaffolder-backend/api-report.md @@ -107,6 +107,13 @@ export const createFilesystemDeleteAction: () => TemplateAction; // @public (undocumented) export const createFilesystemRenameAction: () => TemplateAction; +// Warning: (ae-missing-release-tag) "createGithubActionsDispatchAction" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// +// @public (undocumented) +export function createGithubActionsDispatchAction(options: { + integrations: ScmIntegrationRegistry; +}): TemplateAction; + // Warning: (ae-missing-release-tag) "createPublishAzureAction" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) // // @public (undocumented) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts index 02c15abab1..2265f9b4f7 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/index.ts @@ -20,6 +20,7 @@ export * from './debug'; export * from './fetch'; export * from './filesystem'; export * from './publish'; -export { createFetchCookiecutterAction } from '@backstage/plugin-scaffolder-backend-module-cookiecutter'; export * from './github'; + +export { createFetchCookiecutterAction } from '@backstage/plugin-scaffolder-backend-module-cookiecutter'; export { runCommand } from './helpers';