From c18e8eb91006165328d040b58251c8648d7bf006 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 5 Jul 2021 11:56:28 +0200 Subject: [PATCH 01/12] Provide an optional `logStream: Writable` argument to the `GeneratorBase#run(...)` command Signed-off-by: Dominik Henneke --- .changeset/techdocs-new-candles-sell.md | 6 +++++ packages/techdocs-common/api-report.md | 11 +++++++- .../src/stages/generate/index.ts | 6 ++++- .../src/stages/generate/techdocs.ts | 27 +++++++++++++++---- 4 files changed, 43 insertions(+), 7 deletions(-) create mode 100644 .changeset/techdocs-new-candles-sell.md diff --git a/.changeset/techdocs-new-candles-sell.md b/.changeset/techdocs-new-candles-sell.md new file mode 100644 index 0000000000..b9ff9c3c4b --- /dev/null +++ b/.changeset/techdocs-new-candles-sell.md @@ -0,0 +1,6 @@ +--- +'@backstage/techdocs-common': patch +--- + +Provide an optional `logStream: Writable` argument to the `GeneratorBase#run(...)` command. +The stream receives all log messages that are emitted during the generator run. diff --git a/packages/techdocs-common/api-report.md b/packages/techdocs-common/api-report.md index 3fff030eee..12bf066b78 100644 --- a/packages/techdocs-common/api-report.md +++ b/packages/techdocs-common/api-report.md @@ -47,6 +47,15 @@ export type GeneratorBuilder = { get(entity: Entity): GeneratorBase; }; +// @public +export type GeneratorRunOptions = { + inputDir: string; + outputDir: string; + parsedLocationAnnotation?: ParsedLocationAnnotation; + etag?: string; + logStream?: Writable; +}; + // @public (undocumented) export class Generators implements GeneratorBuilder { // (undocumented) @@ -158,7 +167,7 @@ export class TechdocsGenerator implements GeneratorBase { config: Config; }); // (undocumented) - run({ inputDir, outputDir, parsedLocationAnnotation, etag, }: GeneratorRunOptions): Promise; + run({ inputDir, outputDir, parsedLocationAnnotation, etag, logStream: callerLogStream, }: GeneratorRunOptions): Promise; } // @public diff --git a/packages/techdocs-common/src/stages/generate/index.ts b/packages/techdocs-common/src/stages/generate/index.ts index 382712ccbc..eb015a5c2d 100644 --- a/packages/techdocs-common/src/stages/generate/index.ts +++ b/packages/techdocs-common/src/stages/generate/index.ts @@ -15,4 +15,8 @@ */ export { TechdocsGenerator } from './techdocs'; export { Generators } from './generators'; -export type { GeneratorBuilder, GeneratorBase } from './types'; +export type { + GeneratorBuilder, + GeneratorBase, + GeneratorRunOptions, +} from './types'; diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index 041dff1d2f..f31bc86905 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -18,6 +18,7 @@ import { ContainerRunner } from '@backstage/backend-common'; import { Config } from '@backstage/config'; import path from 'path'; import { PassThrough } from 'stream'; +import * as winston from 'winston'; import { Logger } from 'winston'; import { addBuildTimestampMetadata, @@ -74,9 +75,25 @@ export class TechdocsGenerator implements GeneratorBase { outputDir, parsedLocationAnnotation, etag, + logStream: callerLogStream, }: GeneratorRunOptions): Promise { + // create a copy of the logger. we want to keep all settings but want to add a new logger target. + // without the copy, we would add the target to the original (parent) logger. + const childLogger = winston.createLogger(this.logger); + const [log, logStream] = createStream(); + // Forward the logs to the caller + if (callerLogStream) { + childLogger.add( + new winston.transports.Stream({ stream: callerLogStream }), + ); + // don't use logStream.pipe(callerLogStream) because it would block others to write to callerLogStream + logStream.on('data', data => { + callerLogStream.write(data); + }); + } + // TODO: In future mkdocs.yml can be mkdocs.yaml. So, use a config variable here to find out // the correct file name. // Do some updates to mkdocs.yml before generating docs e.g. adding repo_url @@ -84,7 +101,7 @@ export class TechdocsGenerator implements GeneratorBase { if (parsedLocationAnnotation) { await patchMkdocsYmlPreBuild( mkdocsYmlPath, - this.logger, + childLogger, parsedLocationAnnotation, ); } @@ -108,7 +125,7 @@ export class TechdocsGenerator implements GeneratorBase { }, logStream, }); - this.logger.info( + childLogger.info( `Successfully generated docs from ${inputDir} into ${outputDir} using local mkdocs`, ); break; @@ -123,7 +140,7 @@ export class TechdocsGenerator implements GeneratorBase { // write to, otherwise they will just fail trying to write to / envVars: { HOME: '/tmp' }, }); - this.logger.info( + childLogger.info( `Successfully generated docs from ${inputDir} into ${outputDir} using techdocs-container`, ); break; @@ -136,7 +153,7 @@ export class TechdocsGenerator implements GeneratorBase { this.logger.debug( `Failed to generate docs from ${inputDir} into ${outputDir}`, ); - this.logger.error(`Build failed with error: ${log}`); + childLogger.error(`Build failed with error: ${log}`); throw new Error( `Failed to generate docs from ${inputDir} into ${outputDir} with error ${error.message}`, ); @@ -150,7 +167,7 @@ export class TechdocsGenerator implements GeneratorBase { // Creates techdocs_metadata.json if file does not exist. await addBuildTimestampMetadata( path.join(outputDir, 'techdocs_metadata.json'), - this.logger, + childLogger, ); // Add etag of the prepared tree to techdocs_metadata.json From f1200f44c8a939ce577dfb0615719c684ae01db0 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 5 Jul 2021 12:43:23 +0200 Subject: [PATCH 02/12] Rewrite the `/sync/:namespace/:kind/:name` to return an event-stream Signed-off-by: Dominik Henneke --- .changeset/techdocs-loud-spies-attack.md | 8 + plugins/techdocs-backend/package.json | 2 + .../src/DocsBuilder/builder.ts | 9 +- .../src/service/router.test.ts | 267 ++++++++++++++++++ .../techdocs-backend/src/service/router.ts | 181 +++++++----- plugins/techdocs/api-report.md | 6 +- plugins/techdocs/dev/index.tsx | 21 +- plugins/techdocs/package.json | 2 + plugins/techdocs/src/api.ts | 7 +- plugins/techdocs/src/client.test.ts | 201 ++++++++++++- plugins/techdocs/src/client.ts | 66 +++-- .../techdocs/src/reader/components/Reader.tsx | 8 +- .../reader/components/useReaderState.test.tsx | 40 +-- .../src/reader/components/useReaderState.ts | 46 ++- yarn.lock | 12 + 15 files changed, 691 insertions(+), 185 deletions(-) create mode 100644 .changeset/techdocs-loud-spies-attack.md create mode 100644 plugins/techdocs-backend/src/service/router.test.ts diff --git a/.changeset/techdocs-loud-spies-attack.md b/.changeset/techdocs-loud-spies-attack.md new file mode 100644 index 0000000000..ea80c73116 --- /dev/null +++ b/.changeset/techdocs-loud-spies-attack.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-techdocs': minor +'@backstage/plugin-techdocs-backend': minor +--- + +Rewrite the `/sync/:namespace/:kind/:name` to return an event-stream. +This change allows the sync process to take longer than a normal HTTP timeout. +The stream also emits log events, so the caller can follow the build process in the frontend. diff --git a/plugins/techdocs-backend/package.json b/plugins/techdocs-backend/package.json index 0f0752a0c2..9fb6671031 100644 --- a/plugins/techdocs-backend/package.json +++ b/plugins/techdocs-backend/package.json @@ -31,6 +31,7 @@ }, "dependencies": { "@backstage/backend-common": "^0.8.4", + "@backstage/catalog-client": "^0.3.15", "@backstage/catalog-model": "^0.8.4", "@backstage/config": "^0.1.5", "@backstage/errors": "^0.1.1", @@ -47,6 +48,7 @@ "devDependencies": { "@backstage/cli": "^0.7.3", "@types/dockerode": "^3.2.1", + "msw": "^0.29.0", "supertest": "^6.1.3" }, "files": [ diff --git a/plugins/techdocs-backend/src/DocsBuilder/builder.ts b/plugins/techdocs-backend/src/DocsBuilder/builder.ts index e9df0b8400..a66125521d 100644 --- a/plugins/techdocs-backend/src/DocsBuilder/builder.ts +++ b/plugins/techdocs-backend/src/DocsBuilder/builder.ts @@ -18,6 +18,7 @@ import { ENTITY_DEFAULT_NAMESPACE, stringifyEntityRef, } from '@backstage/catalog-model'; +import { Config } from '@backstage/config'; import { NotModifiedError } from '@backstage/errors'; import { GeneratorBase, @@ -31,8 +32,8 @@ import { import fs from 'fs-extra'; import os from 'os'; import path from 'path'; +import { Writable } from 'stream'; import { Logger } from 'winston'; -import { Config } from '@backstage/config'; import { BuildMetadataStorage } from './BuildMetadataStorage'; type DocsBuilderArguments = { @@ -42,6 +43,7 @@ type DocsBuilderArguments = { entity: Entity; logger: Logger; config: Config; + logStream?: Writable; }; export class DocsBuilder { @@ -51,6 +53,7 @@ export class DocsBuilder { private entity: Entity; private logger: Logger; private config: Config; + private logStream: Writable | undefined; constructor({ preparers, @@ -59,6 +62,7 @@ export class DocsBuilder { entity, logger, config, + logStream, }: DocsBuilderArguments) { this.preparer = preparers.get(entity); this.generator = generators.get(entity); @@ -66,6 +70,7 @@ export class DocsBuilder { this.entity = entity; this.logger = logger; this.config = config; + this.logStream = logStream; } /** @@ -159,12 +164,14 @@ export class DocsBuilder { const outputDir = await fs.mkdtemp( path.join(tmpdirResolvedPath, 'techdocs-tmp-'), ); + const parsedLocationAnnotation = getLocationForEntity(this.entity); await this.generator.run({ inputDir: preparedDir, outputDir, parsedLocationAnnotation, etag: newEtag, + logStream: this.logStream, }); // Remove Prepared directory since it is no longer needed. diff --git a/plugins/techdocs-backend/src/service/router.test.ts b/plugins/techdocs-backend/src/service/router.test.ts new file mode 100644 index 0000000000..a3e17589ae --- /dev/null +++ b/plugins/techdocs-backend/src/service/router.test.ts @@ -0,0 +1,267 @@ +/* + * 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 { + errorHandler, + getVoidLogger, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; +import { ConfigReader } from '@backstage/config'; +import { + GeneratorBuilder, + PreparerBuilder, + PublisherBase, +} from '@backstage/techdocs-common'; +import express from 'express'; +import { rest } from 'msw'; +import { setupServer } from 'msw/node'; +import request from 'supertest'; +import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; +import { createRouter } from './router'; + +jest.mock('@backstage/config'); +jest.mock('../DocsBuilder'); + +const MockedConfigReader = ConfigReader as jest.MockedClass< + typeof ConfigReader +>; +const MockedDocsBuilder = DocsBuilder as jest.MockedClass; + +const server = setupServer(); + +describe('createRouter', () => { + // the calls from supertest should not be handled by msw so we only warn onUnhandledRequest + beforeAll(() => server.listen({ onUnhandledRequest: 'warn' })); + afterAll(() => server.close()); + afterEach(() => server.resetHandlers()); + + const preparers: jest.Mocked = { + register: jest.fn(), + get: jest.fn(), + }; + const generators: jest.Mocked = { + register: jest.fn(), + get: jest.fn(), + }; + const publisher: jest.Mocked = { + docsRouter: jest.fn(), + fetchTechDocsMetadata: jest.fn(), + getReadiness: jest.fn(), + hasDocsBeenGenerated: jest.fn(), + publish: jest.fn(), + }; + const discovery: jest.Mocked = { + getBaseUrl: jest.fn(), + getExternalBaseUrl: jest.fn(), + }; + + let app: express.Express; + + beforeEach(() => { + jest.resetAllMocks(); + }); + + beforeEach(async () => { + publisher.docsRouter.mockReturnValue(() => {}); + discovery.getBaseUrl.mockImplementation(async type => { + return `http://backstage.local/api/${type}`; + }); + + const router = await createRouter({ + preparers, + generators, + publisher, + config: new ConfigReader({}), + logger: getVoidLogger(), + discovery, + }); + + router.use(errorHandler()); + app = express(); + app.use(router); + }); + + describe('GET /sync/:namespace/:kind/:name', () => { + it('should execute an update', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); + MockedConfigReader.prototype.getString.mockReturnValue('local'); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + server.use( + rest.get( + 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', + (_req, res, ctx) => { + return res(ctx.json(entity)); + }, + ), + ); + + MockedDocsBuilder.prototype.build.mockImplementation(async () => { + // extract the logStream from the constructor call + const logStream = MockedDocsBuilder.mock.calls[0][0].logStream; + + logStream?.write('Some log'); + logStream?.write('Another log'); + + return true; + }); + + publisher.hasDocsBeenGenerated.mockResolvedValue(true); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(200); + expect(response.get('content-type')).toBe('text/event-stream'); + expect(response.text).toEqual( + `event: log +data: "Some log" + +event: log +data: "Another log" + +event: finish +data: {"updated":true} + +`, + ); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); + expect(DocsBuilder.prototype.build).toBeCalledTimes(1); + }); + + it('should not check for an update too often', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(false); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + server.use( + rest.get( + 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', + (_req, res, ctx) => { + return res(ctx.json(entity)); + }, + ), + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(200); + expect(response.get('content-type')).toBe('text/event-stream'); + expect(response.text).toEqual( + `event: finish +data: {"updated":false} + +`, + ); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(0); + expect(DocsBuilder.prototype.build).toBeCalledTimes(0); + }); + + it('should not check for an update without local builder', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); + MockedConfigReader.prototype.getString.mockReturnValue('external'); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + server.use( + rest.get( + 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', + (_req, res, ctx) => { + return res(ctx.json(entity)); + }, + ), + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(200); + expect(response.get('content-type')).toBe('text/event-stream'); + expect(response.text).toEqual( + `event: finish +data: {"updated":false} + +`, + ); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); + expect(DocsBuilder.prototype.build).toBeCalledTimes(0); + }); + + it('rejects when entity is not found', async () => { + server.use( + rest.get( + 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', + (_req, res, ctx) => { + return res(ctx.status(404)); + }, + ), + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(404); + }); + }); +}); diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index eceae2e3d3..08c5301f20 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -14,9 +14,10 @@ * limitations under the License. */ import { PluginEndpointDiscovery } from '@backstage/backend-common'; +import { CatalogClient } from '@backstage/catalog-client'; import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import { NotFoundError, NotModifiedError } from '@backstage/errors'; +import { NotFoundError } from '@backstage/errors'; import { GeneratorBuilder, getLocationForEntity, @@ -24,12 +25,12 @@ import { PublisherBase, } from '@backstage/techdocs-common'; import fetch from 'cross-fetch'; -import express from 'express'; +import express, { Response } from 'express'; import Router from 'express-promise-router'; import { Knex } from 'knex'; +import { PassThrough } from 'stream'; import { Logger } from 'winston'; -import { DocsBuilder } from '../DocsBuilder'; -import { shouldCheckForUpdate } from '../DocsBuilder/BuildMetadataStorage'; +import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; type RouterOptions = { preparers: PreparerBuilder; @@ -50,6 +51,7 @@ export async function createRouter({ discovery, }: RouterOptions): Promise { const router = Router(); + const catalogClient = new CatalogClient({ discoveryApi: discovery }); router.get('/metadata/techdocs/:namespace/:kind/:name', async (req, res) => { const { kind, namespace, name } = req.params; @@ -108,56 +110,45 @@ export async function createRouter({ }); // Check if docs are the latest version and trigger rebuilds if not - // Responds with immediate success if rebuild not needed + // Responds with an event-stream that closes after the build finished + // Responds with an immediate success if rebuild not needed // If a build is required, responds with a success when finished router.get('/sync/:namespace/:kind/:name', async (req, res) => { const { kind, namespace, name } = req.params; - const catalogUrl = await discovery.getBaseUrl('catalog'); - const triple = [kind, namespace, name].map(encodeURIComponent).join('/'); - const token = getBearerToken(req.headers.authorization); - const catalogRes = await fetch(`${catalogUrl}/entities/by-name/${triple}`, { - headers: token ? { Authorization: `Bearer ${token}` } : {}, - }); - if (!catalogRes.ok) { - const catalogResText = await catalogRes.text(); - res.status(catalogRes.status); - res.send(catalogResText); - return; - } - const entity: Entity = await catalogRes.json(); + const entity = await catalogClient.getEntityByName( + { kind, namespace, name }, + { token }, + ); - if (!entity.metadata.uid) { + if (!entity?.metadata?.uid) { throw new NotFoundError('Entity metadata UID missing'); } + + // open the event-stream + const { log, error, finish } = createEventStream(res); + + // create an in-memory stream to forward logs to the event-stream + const logStream = new PassThrough(); + logStream.on('data', async data => { + log(data.toString().trim()); + }); + + // check if the last update check was too recent if (!shouldCheckForUpdate(entity.metadata.uid)) { - res.status(200).json({ - message: `Last check for documentation update is recent, did not retry.`, - }); + finish({ updated: false }); return; } - let publisherType = ''; - try { - publisherType = config.getString('techdocs.publisher.type'); - } catch (err) { - throw new Error( - 'Unable to get techdocs.publisher.type in your app config. Set it to either ' + - "'local', 'googleGcs' or other support storage providers. Read more here " + - 'https://backstage.io/docs/features/techdocs/architecture', - ); - } // techdocs-backend will only try to build documentation for an entity if techdocs.builder is set to 'local' // If set to 'external', it will assume that an external process (e.g. CI/CD pipeline // of the repository) is responsible for building and publishing documentation to the storage provider if (config.getString('techdocs.builder') !== 'local') { - res.status(200).json({ - message: - '`techdocs.builder` app config is not set to `local`, so docs will not be generated locally and sync is not required.', - }); + finish({ updated: false }); return; } + const docsBuilder = new DocsBuilder({ preparers, generators, @@ -165,52 +156,41 @@ export async function createRouter({ logger, entity, config, + logStream, }); + let foundDocs = false; - switch (publisherType) { - case 'local': - case 'awsS3': - case 'azureBlobStorage': - case 'openStackSwift': - case 'googleGcs': { - // This block should be valid for all storage implementations. So no need to duplicate in future, - // add the publisher type in the list here. - const updated = await docsBuilder.build(); - if (!updated) { - throw new NotModifiedError(); - } + const updated = await docsBuilder.build(); - // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched - // on the user's page. If not, respond with a message asking them to check back later. - // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second - for (let attempt = 0; attempt < 5; attempt++) { - if (await publisher.hasDocsBeenGenerated(entity)) { - foundDocs = true; - break; - } - await new Promise(r => setTimeout(r, 1000)); - } - if (!foundDocs) { - logger.error( - 'Published files are taking longer to show up in storage. Something went wrong.', - ); - throw new NotFoundError( - 'Sorry! It took too long for the generated docs to show up in storage. Check back later.', - ); - } + if (!updated) { + finish({ updated: false }); + return; + } - res - .status(201) - .json({ message: 'Docs updated or did not need updating' }); + // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched + // on the user's page. If not, respond with a message asking them to check back later. + // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second + for (let attempt = 0; attempt < 5; attempt++) { + if (await publisher.hasDocsBeenGenerated(entity)) { + foundDocs = true; break; } - - default: - throw new NotFoundError( - `Publisher type ${publisherType} is not supported by techdocs-backend docs builder.`, - ); + await new Promise(r => setTimeout(r, 1000)); } + if (!foundDocs) { + logger.error( + 'Published files are taking longer to show up in storage. Something went wrong.', + ); + error( + new NotFoundError( + 'Sorry! It took too long for the generated docs to show up in storage. Check back later.', + ), + ); + return; + } + + finish({ updated: true }); }); // Route middleware which serves files from the storage set in the publisher. @@ -222,3 +202,56 @@ export async function createRouter({ function getBearerToken(header?: string): string | undefined { return header?.match(/(?:Bearer)\s+(\S+)/i)?.[1]; } + +/** + * Create an event-stream response that emits the events 'log', 'error', and 'finish'. + * + * @param res the response to write the event-stream to + * @returns A tuple of callbacks to emit messages. A call to 'error' or 'finish' + * will close the event-stream. + */ +function createEventStream( + res: Response, +): { + log: (message: string) => void; + error: (e: Error) => void; + finish: (result: { updated: boolean }) => void; +} { + // Mandatory headers and http status to keep connection open + res.writeHead(200, { + Connection: 'keep-alive', + 'Cache-Control': 'no-cache', + 'Content-Type': 'text/event-stream', + }); + + // client closes connection + res.socket?.on('close', () => { + res.end(); + }); + + // write the event to the stream + const send = (type: 'error' | 'finish' | 'log', data: any) => { + res.write(`event: ${type}\ndata: ${JSON.stringify(data)}\n\n`); + + // res.flush() is only available with the compression middleware + if (res.flush) { + res.flush(); + } + }; + + return { + log: data => { + send('log', data); + }, + + error: e => { + send('error', e.message); + res.end(); + }, + + finish: result => { + send('finish', result); + res.end(); + }, + }; +} diff --git a/plugins/techdocs/api-report.md b/plugins/techdocs/api-report.md index dbd5e3ab26..825db437d9 100644 --- a/plugins/techdocs/api-report.md +++ b/plugins/techdocs/api-report.md @@ -44,7 +44,7 @@ export const Reader: ({ entityId, onReady }: Props_2) => JSX.Element; export const Router: () => JSX.Element; // @public (undocumented) -export type SyncResult = 'cached' | 'updated' | 'timeout'; +export type SyncResult = 'cached' | 'updated'; // @public (undocumented) export interface TechDocsApi { @@ -112,7 +112,7 @@ export interface TechDocsStorageApi { // (undocumented) getStorageUrl(): Promise; // (undocumented) - syncEntityDocs(entityId: EntityName): Promise; + syncEntityDocs(entityId: EntityName, logHandler?: (line: string) => void): Promise; } // @public (undocumented) @@ -140,7 +140,7 @@ export class TechDocsStorageClient implements TechDocsStorageApi { getStorageUrl(): Promise; // (undocumented) identityApi: IdentityApi; - syncEntityDocs(entityId: EntityName): Promise; + syncEntityDocs(entityId: EntityName, logHandler?: (line: string) => void): Promise; } diff --git a/plugins/techdocs/dev/index.tsx b/plugins/techdocs/dev/index.tsx index cdc81caa5a..80e81fe178 100644 --- a/plugins/techdocs/dev/index.tsx +++ b/plugins/techdocs/dev/index.tsx @@ -17,6 +17,7 @@ import { createDevApp } from '@backstage/dev-utils'; import { NotFoundError } from '@backstage/errors'; import React from 'react'; +import { EntityName } from '@backstage/catalog-model'; import { Reader, SyncResult, @@ -81,9 +82,18 @@ function createPage({ }); } - async syncEntityDocs() { + async syncEntityDocs(_: EntityName, logHandler?: (line: string) => void) { if (syncDocsDelay) { - await new Promise(resolve => setTimeout(resolve, syncDocsDelay)); + for (let i = 0; i < 10; i++) { + setTimeout( + () => logHandler?.call(this, `Log line ${i}`), + ((i + 1) * syncDocsDelay) / 10, + ); + } + + await new Promise(resolve => { + setTimeout(resolve, syncDocsDelay); + }); } return syncDocs(); @@ -195,13 +205,6 @@ createDevApp() syncDocsDelay: 2000, })} - - - {createPage({ - syncDocs: () => 'timeout', - syncDocsDelay: 2000, - })} - ), diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 1a5abc6fbf..1e67549e23 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -44,6 +44,7 @@ "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.45", "@material-ui/styles": "^4.10.0", + "eventsource": "^1.1.0", "react": "^16.13.1", "react-dom": "^16.13.1", "react-router": "6.0.0-beta.0", @@ -60,6 +61,7 @@ "@testing-library/react": "^11.2.5", "@testing-library/react-hooks": "^3.4.2", "@testing-library/user-event": "^13.1.8", + "@types/eventsource": "^1.1.5", "@types/jest": "^26.0.7", "@types/node": "^14.14.32", "@types/react": "^16.9", diff --git a/plugins/techdocs/src/api.ts b/plugins/techdocs/src/api.ts index 739c5fc133..a23d9fc7f0 100644 --- a/plugins/techdocs/src/api.ts +++ b/plugins/techdocs/src/api.ts @@ -28,14 +28,17 @@ export const techdocsApiRef = createApiRef({ description: 'Used to make requests towards techdocs API', }); -export type SyncResult = 'cached' | 'updated' | 'timeout'; +export type SyncResult = 'cached' | 'updated'; export interface TechDocsStorageApi { getApiOrigin(): Promise; getStorageUrl(): Promise; getBuilder(): Promise; getEntityDocs(entityId: EntityName, path: string): Promise; - syncEntityDocs(entityId: EntityName): Promise; + syncEntityDocs( + entityId: EntityName, + logHandler?: (line: string) => void, + ): Promise; getBaseUrl( oldBaseUrl: string, entityId: EntityName, diff --git a/plugins/techdocs/src/client.test.ts b/plugins/techdocs/src/client.test.ts index 329d3cbcb6..4cc7aca6a6 100644 --- a/plugins/techdocs/src/client.test.ts +++ b/plugins/techdocs/src/client.test.ts @@ -13,9 +13,19 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { Config } from '@backstage/config'; -import { TechDocsStorageClient } from './client'; import { UrlPatternDiscovery } from '@backstage/core-app-api'; +import { IdentityApi } from '@backstage/core-plugin-api'; +import { NotFoundError } from '@backstage/errors'; +import EventSource from 'eventsource'; +import { TechDocsStorageClient } from './client'; + +const MockedEventSource: jest.MockedClass< + typeof EventSource +> = EventSource as any; + +jest.mock('eventsource'); const mockEntity = { kind: 'Component', @@ -29,6 +39,16 @@ describe('TechDocsStorageClient', () => { getOptionalString: () => 'http://backstage:9191/api/techdocs', } as Partial; const discoveryApi = UrlPatternDiscovery.compile(mockBaseUrl); + const identityApi: jest.Mocked = { + getIdToken: jest.fn(), + getProfile: jest.fn(), + getUserId: jest.fn(), + signOut: jest.fn(), + }; + + beforeEach(() => { + jest.resetAllMocks(); + }); it('should return correct base url based on defined storage', async () => { // @ts-ignore Partial not assignable to Config. @@ -51,4 +71,183 @@ describe('TechDocsStorageClient', () => { `${mockBaseUrl}/static/docs/${mockEntity.namespace}/${mockEntity.kind}/${mockEntity.name}/test/`, ); }); + + describe('syncEntityDocs', () => { + it('should create eventsource without headers', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + MockedEventSource.prototype.addEventListener.mockImplementation( + (type, fn) => { + if (type === 'finish') { + fn({ data: '{"updated": false}' } as any); + } + }, + ); + + await storageApi.syncEntityDocs(mockEntity); + + expect( + MockedEventSource, + ).toBeCalledWith( + 'http://backstage:9191/api/techdocs/sync/default/Component/test-component', + { withCredentials: true, headers: {} }, + ); + }); + + it('should create eventsource with headers', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + MockedEventSource.prototype.addEventListener.mockImplementation( + (type, fn) => { + if (type === 'finish') { + fn({ data: '{"updated": false}' } as any); + } + }, + ); + + identityApi.getIdToken.mockResolvedValue('token'); + + await storageApi.syncEntityDocs(mockEntity); + + expect( + MockedEventSource, + ).toBeCalledWith( + 'http://backstage:9191/api/techdocs/sync/default/Component/test-component', + { withCredentials: true, headers: { Authorization: 'Bearer token' } }, + ); + }); + + it('should resolve to cached', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + MockedEventSource.prototype.addEventListener.mockImplementation( + (type, fn) => { + if (type === 'finish') { + fn({ data: '{"updated": false}' } as any); + } + }, + ); + + await expect(storageApi.syncEntityDocs(mockEntity)).resolves.toEqual( + 'cached', + ); + }); + + it('should resolve to updated', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + MockedEventSource.prototype.addEventListener.mockImplementation( + (type, fn) => { + if (type === 'finish') { + fn({ data: '{"updated": true}' } as any); + } + }, + ); + + await expect(storageApi.syncEntityDocs(mockEntity)).resolves.toEqual( + 'updated', + ); + }); + + it('should log values', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + MockedEventSource.prototype.addEventListener.mockImplementation( + (type, fn) => { + if (type === 'log') { + fn({ data: '"A log message"' } as any); + } + + if (type === 'finish') { + fn({ data: '{"updated": false}' } as any); + } + }, + ); + + const logHandler = jest.fn(); + await expect( + storageApi.syncEntityDocs(mockEntity, logHandler), + ).resolves.toEqual('cached'); + + expect(logHandler).toBeCalledTimes(1); + expect(logHandler).toBeCalledWith('A log message'); + }); + + it('should throw NotFoundError', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + // we await later after we emitted the error + const promise = storageApi.syncEntityDocs(mockEntity).then(); + + // flush the event loop + await new Promise(setImmediate); + + const instance = MockedEventSource.mock + .instances[0] as jest.Mocked; + + instance.onerror({ + status: 404, + message: 'Some not found warning', + } as any); + + await expect(promise).rejects.toThrow(NotFoundError); + await expect(promise).rejects.toThrowError('Some not found warning'); + }); + + it('should throw generic errors', async () => { + const storageApi = new TechDocsStorageClient({ + // @ts-ignore Partial not assignable to Config. + configApi, + discoveryApi, + identityApi, + }); + + // we await later after we emitted the error + const promise = storageApi.syncEntityDocs(mockEntity).then(); + + // flush the event loop + await new Promise(setImmediate); + + const instance = MockedEventSource.mock + .instances[0] as jest.Mocked; + + instance.onerror({ + status: 500, + message: 'Some other error', + } as any); + + await expect(promise).rejects.toThrow(Error); + await expect(promise).rejects.toThrowError('Some other error'); + }); + }); }); diff --git a/plugins/techdocs/src/client.ts b/plugins/techdocs/src/client.ts index 19fc565a2f..29e6e018d0 100644 --- a/plugins/techdocs/src/client.ts +++ b/plugins/techdocs/src/client.ts @@ -16,10 +16,11 @@ import { EntityName } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; +import { DiscoveryApi, IdentityApi } from '@backstage/core-plugin-api'; import { NotFoundError } from '@backstage/errors'; +import EventSource from 'eventsource'; import { SyncResult, TechDocsApi, TechDocsStorageApi } from './api'; import { TechDocsEntityMetadata, TechDocsMetadata } from './types'; -import { DiscoveryApi, IdentityApi } from '@backstage/core-plugin-api'; /** * API to talk to techdocs-backend. @@ -192,44 +193,59 @@ export class TechDocsStorageClient implements TechDocsStorageApi { * Check if docs are on the latest version and trigger rebuild if not * * @param {EntityName} entityId Object containing entity data like name, namespace, etc. + * @param {Function} logHandler Callback to receive log messages from the build process * @returns {SyncResult} Whether documents are currently synchronized to newest version * @throws {Error} Throws error on error from sync endpoint in Techdocs Backend */ - async syncEntityDocs(entityId: EntityName): Promise { + async syncEntityDocs( + entityId: EntityName, + logHandler: (line: string) => void = () => {}, + ): Promise { const { kind, namespace, name } = entityId; const apiOrigin = await this.getApiOrigin(); const url = `${apiOrigin}/sync/${namespace}/${kind}/${name}`; const token = await this.identityApi.getIdToken(); - let request; - let attempts: number = 0; - // retry if request times out, up to 5 times - // can happen due to docs taking too long to generate - while (!request || (request.status === 408 && attempts < 5)) { - attempts++; - request = await fetch(url, { + + return new Promise((resolve, reject) => { + const source = new EventSource(url, { + withCredentials: true, headers: token ? { Authorization: `Bearer ${token}` } : {}, }); - } - switch (request.status) { - case 404: - throw new NotFoundError((await request.json()).error); + source.addEventListener('log', (e: any) => { + if (e.data) { + logHandler(JSON.parse(e.data)); + } + }); - case 200: - case 304: - return 'cached'; + source.addEventListener('finish', (e: any) => { + let updated: boolean = false; - case 201: - return 'updated'; + if (e.data) { + ({ updated } = JSON.parse(e.data)); + } - // for timeout and misc errors, handle without error to allow viewing older docs - // if older docs not available, - // Reader will show 404 error coming from getEntityDocs - case 408: - default: - return 'timeout'; - } + resolve(updated ? 'updated' : 'cached'); + }); + + source.onerror = (e: any) => { + source.close(); + + switch (e.status) { + // the endpoint returned a 404 status + case 404: + reject(new NotFoundError(e.message)); + return; + + // also handles the event-stream close. the reject is ignored if the Promise was already + // resolved by a finish event. + default: + reject(new Error(e.message)); + return; + } + }; + }); } async getBaseUrl( diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index f80cf6cc4c..cc76d789b6 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -15,6 +15,7 @@ */ import { EntityName } from '@backstage/catalog-model'; +import { useApi } from '@backstage/core-plugin-api'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; import { BackstageTheme } from '@backstage/theme'; import { useTheme } from '@material-ui/core'; @@ -37,7 +38,6 @@ import { import { TechDocsNotFound } from './TechDocsNotFound'; import TechDocsProgressBar from './TechDocsProgressBar'; import { useReaderState } from './useReaderState'; -import { useApi } from '@backstage/core-plugin-api'; type Props = { entityId: EntityName; @@ -328,12 +328,6 @@ export const Reader = ({ entityId, onReady }: Props) => { to view. )} - {state === 'CONTENT_STALE_TIMEOUT' && ( - - Building a newer version of this documentation took longer than - expected. Please refresh to try again. - - )} {state === 'CONTENT_STALE_ERROR' && ( Building a newer version of this documentation failed. {errorMessage} diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 1edf41af30..d3f5362a07 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -14,6 +14,7 @@ * limitations under the License. */ +import { ApiProvider, ApiRegistry } from '@backstage/core-app-api'; import { NotFoundError } from '@backstage/errors'; import { act, renderHook } from '@testing-library/react-hooks'; import React from 'react'; @@ -23,7 +24,6 @@ import { reducer, useReaderState, } from './useReaderState'; -import { ApiProvider, ApiRegistry } from '@backstage/core-app-api'; describe('useReaderState', () => { let Wrapper: React.ComponentType; @@ -55,14 +55,12 @@ describe('useReaderState', () => { ${false} | ${undefined} | ${'BUILDING'} | ${'INITIAL_BUILD'} ${false} | ${undefined} | ${'BUILD_READY'} | ${'CONTENT_NOT_FOUND'} ${false} | ${undefined} | ${'BUILD_READY_RELOAD'} | ${'CHECKING'} - ${false} | ${undefined} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_NOT_FOUND'} ${false} | ${undefined} | ${'UP_TO_DATE'} | ${'CONTENT_NOT_FOUND'} ${false} | ${undefined} | ${'ERROR'} | ${'CONTENT_NOT_FOUND'} ${false} | ${'asdf'} | ${'CHECKING'} | ${'CONTENT_FRESH'} ${false} | ${'asdf'} | ${'BUILDING'} | ${'CONTENT_STALE_REFRESHING'} ${false} | ${'asdf'} | ${'BUILD_READY'} | ${'CONTENT_STALE_READY'} ${false} | ${'asdf'} | ${'BUILD_READY_RELOAD'} | ${'CHECKING'} - ${false} | ${'asdf'} | ${'BUILD_TIMED_OUT'} | ${'CONTENT_STALE_TIMEOUT'} ${false} | ${'asdf'} | ${'UP_TO_DATE'} | ${'CONTENT_FRESH'} ${false} | ${'asdf'} | ${'ERROR'} | ${'CONTENT_STALE_ERROR'} `( @@ -369,42 +367,6 @@ describe('useReaderState', () => { }); }); - it('should handle timed-out refresh', async () => { - techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); - techdocsStorageApi.syncEntityDocs.mockResolvedValue('timeout'); - - await act(async () => { - const { result, waitForValueToChange } = await renderHook( - () => useReaderState('Component', 'default', 'backstage', '/example'), - { wrapper: Wrapper }, - ); - - expect(result.current).toEqual({ - state: 'CHECKING', - content: undefined, - errorMessage: '', - }); - - // the content is returned but the sync is in progress - await waitForValueToChange(() => result.current.state); - expect(result.current).toEqual({ - state: 'CONTENT_STALE_TIMEOUT', - content: 'my content', - errorMessage: '', - }); - - expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( - { kind: 'Component', namespace: 'default', name: 'backstage' }, - '/example', - ); - expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ - kind: 'Component', - namespace: 'default', - name: 'backstage', - }); - }); - }); - it('should handle content error', async () => { techdocsStorageApi.getEntityDocs.mockRejectedValue( new NotFoundError('Some error description'), diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index ac5145ee49..be47ee301d 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -14,10 +14,10 @@ * limitations under the License. */ +import { useApi } from '@backstage/core-plugin-api'; import { useEffect, useMemo, useReducer, useRef } from 'react'; import { useAsync, useAsyncRetry } from 'react-use'; import { techdocsStorageApiRef } from '../../api'; -import { useApi } from '@backstage/core-plugin-api'; /** * A state representation that is used to configure the UI of @@ -35,9 +35,6 @@ type ContentStateTypes = /** There is content, but after a reload, the content will be different */ | 'CONTENT_STALE_READY' - /** There is content, the backend tried to update it, but it took too long */ - | 'CONTENT_STALE_TIMEOUT' - /** There is content, the backend tried to update it, but failed */ | 'CONTENT_STALE_ERROR' @@ -93,11 +90,6 @@ export function calculateDisplayState({ return 'CONTENT_STALE_READY'; } - // the build timed out, but the content is still stale - if (activeSyncState === 'BUILD_TIMED_OUT') { - return 'CONTENT_STALE_TIMEOUT'; - } - // the build failed, but the content is still stale if (activeSyncState === 'ERROR') { return 'CONTENT_STALE_ERROR'; @@ -127,9 +119,6 @@ type SyncStates = */ | 'BUILD_READY_RELOAD' - /** Building the documentation timed out */ - | 'BUILD_TIMED_OUT' - /** No need for a sync. The content was already up-to-date. */ | 'UP_TO_DATE' @@ -274,18 +263,27 @@ export function useReaderState( name, }); - if (result === 'updated') { - // if there was no content prior to building, retry the loading - if (!contentRef.current.content) { - contentRef.current.reload(); - dispatch({ type: 'sync', state: 'BUILD_READY_RELOAD' }); - } else { - dispatch({ type: 'sync', state: 'BUILD_READY' }); - } - } else if (result === 'cached') { - dispatch({ type: 'sync', state: 'UP_TO_DATE' }); - } else { - dispatch({ type: 'sync', state: 'BUILD_TIMED_OUT' }); + switch (result) { + case 'updated': + // if there was no content prior to building, retry the loading + if (!contentRef.current.content) { + contentRef.current.reload(); + dispatch({ type: 'sync', state: 'BUILD_READY_RELOAD' }); + } else { + dispatch({ type: 'sync', state: 'BUILD_READY' }); + } + break; + case 'cached': + dispatch({ type: 'sync', state: 'UP_TO_DATE' }); + break; + + default: + dispatch({ + type: 'sync', + state: 'ERROR', + syncError: new Error('Unexpected return state'), + }); + break; } } catch (e) { dispatch({ type: 'sync', state: 'ERROR', syncError: e }); diff --git a/yarn.lock b/yarn.lock index b04756527c..4b0383a653 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5745,6 +5745,11 @@ resolved "https://registry.npmjs.org/@types/estree/-/estree-0.0.39.tgz#e177e699ee1b8c22d23174caaa7422644389509f" integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw== +"@types/eventsource@^1.1.5": + version "1.1.5" + resolved "https://registry.npmjs.org/@types/eventsource/-/eventsource-1.1.5.tgz#408e9b45efb176c8bea672ab58c81e7ab00d24bc" + integrity sha512-BA9q9uC2PAMkUS7DunHTxWZZaVpeNzDG8lkBxcKwzKJClfDQ4Z59/Csx7HSH/SIqFN2JWh0tAKAM6k/wRR0OZg== + "@types/express-serve-static-core@*", "@types/express-serve-static-core@^4.17.18", "@types/express-serve-static-core@^4.17.21", "@types/express-serve-static-core@^4.17.5": version "4.17.21" resolved "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-4.17.21.tgz#a427278e106bca77b83ad85221eae709a3414d42" @@ -12604,6 +12609,13 @@ eventsource@^1.0.5, eventsource@^1.0.7: dependencies: original "^1.0.0" +eventsource@^1.1.0: + version "1.1.0" + resolved "https://registry.npmjs.org/eventsource/-/eventsource-1.1.0.tgz#00e8ca7c92109e94b0ddf32dac677d841028cfaf" + integrity sha512-VSJjT5oCNrFvCS6igjzPAt5hBzQ2qPBFIbJ03zLI9SE0mxwZpMw6BfJrbFHm1a141AavMEB8JHmBhWAd66PfCg== + dependencies: + original "^1.0.0" + evp_bytestokey@^1.0.0, evp_bytestokey@^1.0.3: version "1.0.3" resolved "https://registry.npmjs.org/evp_bytestokey/-/evp_bytestokey-1.0.3.tgz#7fcbdb198dc71959432efe13842684e0525acb02" From 3af126cddfd3de598c10e7feb2f8219164fc8390 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 5 Jul 2021 12:48:12 +0200 Subject: [PATCH 03/12] Provide a Drawer component to follow a running build Signed-off-by: Dominik Henneke --- .changeset/techdocs-lovely-bottles-beam.md | 6 + plugins/techdocs/package.json | 1 + .../techdocs/src/reader/components/Reader.tsx | 33 ++++- .../components/TechDocsBuildLogs.test.tsx | 87 +++++++++++++ .../reader/components/TechDocsBuildLogs.tsx | 121 ++++++++++++++++++ .../components/TechDocsProgressBar.test.tsx | 38 ------ .../reader/components/TechDocsProgressBar.tsx | 50 -------- .../reader/components/useReaderState.test.tsx | 118 +++++++++++++---- .../src/reader/components/useReaderState.ts | 42 +++++- 9 files changed, 366 insertions(+), 130 deletions(-) create mode 100644 .changeset/techdocs-lovely-bottles-beam.md create mode 100644 plugins/techdocs/src/reader/components/TechDocsBuildLogs.test.tsx create mode 100644 plugins/techdocs/src/reader/components/TechDocsBuildLogs.tsx delete mode 100644 plugins/techdocs/src/reader/components/TechDocsProgressBar.test.tsx delete mode 100644 plugins/techdocs/src/reader/components/TechDocsProgressBar.tsx diff --git a/.changeset/techdocs-lovely-bottles-beam.md b/.changeset/techdocs-lovely-bottles-beam.md new file mode 100644 index 0000000000..3f89396c79 --- /dev/null +++ b/.changeset/techdocs-lovely-bottles-beam.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Provide a Drawer component to follow a running build. +This can be used to debug the rendering and get build logs in case an error occurs. diff --git a/plugins/techdocs/package.json b/plugins/techdocs/package.json index 1e67549e23..1b1c488429 100644 --- a/plugins/techdocs/package.json +++ b/plugins/techdocs/package.json @@ -47,6 +47,7 @@ "eventsource": "^1.1.0", "react": "^16.13.1", "react-dom": "^16.13.1", + "react-lazylog": "^4.5.2", "react-router": "6.0.0-beta.0", "react-router-dom": "6.0.0-beta.0", "react-use": "^17.2.4", diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index cc76d789b6..f8938cf8d8 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -15,10 +15,11 @@ */ import { EntityName } from '@backstage/catalog-model'; +import { Progress } from '@backstage/core-components'; import { useApi } from '@backstage/core-plugin-api'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; import { BackstageTheme } from '@backstage/theme'; -import { useTheme } from '@material-ui/core'; +import { 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'; @@ -35,8 +36,8 @@ import { simplifyMkdocsFooter, transform as transformer, } from '../transformers'; +import { TechDocsBuildLogs } from './TechDocsBuildLogs'; import { TechDocsNotFound } from './TechDocsNotFound'; -import TechDocsProgressBar from './TechDocsProgressBar'; import { useReaderState } from './useReaderState'; type Props = { @@ -49,7 +50,7 @@ export const Reader = ({ entityId, onReady }: Props) => { const { '*': path } = useParams(); const theme = useTheme(); - const { state, content: rawPage, errorMessage } = useReaderState( + const { state, content: rawPage, errorMessage, buildLog } = useReaderState( kind, namespace, name, @@ -313,11 +314,25 @@ export const Reader = ({ entityId, onReady }: Props) => { return ( <> - {(state === 'CHECKING' || state === 'INITIAL_BUILD') && ( - + {state === 'CHECKING' && } + {state === 'INITIAL_BUILD' && ( + } + action={} + > + Documentation is accessed for the first time and is being prepared. + The subsequent loads are much faster. + )} {state === 'CONTENT_STALE_REFRESHING' && ( - + } + action={} + > A newer version of this documentation is being prepared and will be available shortly. @@ -329,7 +344,11 @@ export const Reader = ({ entityId, onReady }: Props) => { )} {state === 'CONTENT_STALE_ERROR' && ( - + } + > Building a newer version of this documentation failed. {errorMessage} )} diff --git a/plugins/techdocs/src/reader/components/TechDocsBuildLogs.test.tsx b/plugins/techdocs/src/reader/components/TechDocsBuildLogs.test.tsx new file mode 100644 index 0000000000..d1bb99b76c --- /dev/null +++ b/plugins/techdocs/src/reader/components/TechDocsBuildLogs.test.tsx @@ -0,0 +1,87 @@ +/* + * 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 { render } from '@testing-library/react'; +import React from 'react'; +import { + TechDocsBuildLogs, + TechDocsBuildLogsDrawerContent, +} from './TechDocsBuildLogs'; + +// react-lazylog is based on a react-virtualized component which doesn't +// write the content to the dom, so we mock it. +jest.mock('react-lazylog', () => { + return { + LazyLog: ({ text }: { text: string }) => { + return

{text}

; + }, + }; +}); + +describe('', () => { + it('should render with button', () => { + const rendered = render(); + expect(rendered.getByText(/Show Build Logs/i)).toBeInTheDocument(); + expect(rendered.queryByText(/Build Details/i)).not.toBeInTheDocument(); + }); + + it('should open drawer', () => { + const rendered = render(); + rendered.getByText(/Show Build Logs/i).click(); + expect(rendered.getByText(/Build Details/i)).toBeInTheDocument(); + }); +}); + +describe('', () => { + it('should render with empty log', () => { + const onClose = jest.fn(); + const rendered = render( + , + ); + expect(rendered.getByText(/Build Details/i)).toBeInTheDocument(); + expect(rendered.getByText(/Waiting for logs.../i)).toBeInTheDocument(); + + expect(onClose).toBeCalledTimes(0); + }); + + it('should render with empty logs', () => { + const onClose = jest.fn(); + const rendered = render( + , + ); + expect(rendered.getByText(/Build Details/i)).toBeInTheDocument(); + expect( + rendered.queryByText(/Waiting for logs.../i), + ).not.toBeInTheDocument(); + expect(rendered.getByText(/Line 1/i)).toBeInTheDocument(); + expect(rendered.getByText(/Line 2/i)).toBeInTheDocument(); + + expect(onClose).toBeCalledTimes(0); + }); + + it('should call onClose', () => { + const onClose = jest.fn(); + const rendered = render( + , + ); + rendered.getByRole('button').click(); + + expect(onClose).toBeCalledTimes(1); + }); +}); diff --git a/plugins/techdocs/src/reader/components/TechDocsBuildLogs.tsx b/plugins/techdocs/src/reader/components/TechDocsBuildLogs.tsx new file mode 100644 index 0000000000..d9da954152 --- /dev/null +++ b/plugins/techdocs/src/reader/components/TechDocsBuildLogs.tsx @@ -0,0 +1,121 @@ +/* + * 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 { + Button, + createStyles, + Drawer, + Grid, + IconButton, + makeStyles, + Theme, + Typography, +} from '@material-ui/core'; +import Close from '@material-ui/icons/Close'; +import * as React from 'react'; +import { useState } from 'react'; +import { LazyLog } from 'react-lazylog'; + +const useDrawerStyles = makeStyles((theme: Theme) => + createStyles({ + paper: { + width: '100%', + [theme.breakpoints.up('sm')]: { + width: '75%', + }, + [theme.breakpoints.up('md')]: { + width: '50%', + }, + padding: theme.spacing(2.5), + }, + root: { + height: '100%', + overflow: 'hidden', + }, + }), +); + +export const TechDocsBuildLogsDrawerContent = ({ + buildLog, + onClose, +}: { + buildLog: string[]; + onClose: () => void; +}) => { + const classes = useDrawerStyles(); + return ( + + + Build Details + + + + + + + + ); +}; + +export const TechDocsBuildLogs = ({ buildLog }: { buildLog: string[] }) => { + const classes = useDrawerStyles(); + const [open, setOpen] = useState(false); + + return ( + <> + + setOpen(false)} + > + setOpen(false)} + /> + + + ); +}; diff --git a/plugins/techdocs/src/reader/components/TechDocsProgressBar.test.tsx b/plugins/techdocs/src/reader/components/TechDocsProgressBar.test.tsx deleted file mode 100644 index f33ed62b97..0000000000 --- a/plugins/techdocs/src/reader/components/TechDocsProgressBar.test.tsx +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright 2020 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 TechDocsProgressBar from './TechDocsProgressBar'; -import React from 'react'; -import { render } from '@testing-library/react'; -import { wrapInTestApp } from '@backstage/test-utils'; -import { act } from 'react-dom/test-utils'; - -jest.useFakeTimers(); - -describe('', () => { - it('should render a message if techdocs page takes more time to load', () => { - const rendered = render(wrapInTestApp()); - act(() => { - jest.advanceTimersByTime(250); - }); - expect(rendered.getByTestId('progress')).toBeInTheDocument(); - expect(rendered.queryByTestId('delay-reason')).toBeNull(); - act(() => { - jest.advanceTimersByTime(5000); - }); - expect(rendered.getByTestId('delay-reason')).toBeInTheDocument(); - }); -}); diff --git a/plugins/techdocs/src/reader/components/TechDocsProgressBar.tsx b/plugins/techdocs/src/reader/components/TechDocsProgressBar.tsx deleted file mode 100644 index 7089f5ae08..0000000000 --- a/plugins/techdocs/src/reader/components/TechDocsProgressBar.tsx +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright 2020 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, { useState, useEffect } from 'react'; -import { useMountedState } from 'react-use'; -import { Typography } from '@material-ui/core'; -import { Progress } from '@backstage/core-components'; - -const TechDocsProgressBar = () => { - const isMounted = useMountedState(); - const [hasBeenDelayed, setHasBeenDelayed] = useState(false); - - const delayReason = `Docs are still loading...Backstage takes some extra time to load docs - for the first time. The subsequent loads are much faster.`; - - // Allowed time that docs can take to load (in milliseconds) - const allowedDelayTime = 5000; - - useEffect(() => { - setTimeout(() => { - if (isMounted()) { - setHasBeenDelayed(true); - } - }, allowedDelayTime); - }); - - return ( - <> - {hasBeenDelayed ? ( - {delayReason} - ) : null} - - - ); -}; - -export default TechDocsProgressBar; diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index d3f5362a07..3a26f91279 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -82,6 +82,7 @@ describe('useReaderState', () => { activeSyncState: 'CHECKING', contentLoading: false, path: '', + buildLog: ['1', '2'], }; it('should return a copy of the state', () => { @@ -89,12 +90,14 @@ describe('useReaderState', () => { activeSyncState: 'CHECKING', contentLoading: false, path: '/', + buildLog: ['1', '2'], }); expect(oldState).toEqual({ activeSyncState: 'CHECKING', contentLoading: false, path: '', + buildLog: ['1', '2'], }); }); @@ -208,6 +211,33 @@ describe('useReaderState', () => { activeSyncState: 'BUILDING', }); }); + + it('should clear buildLog on "CHECKING"', () => { + expect( + reducer(oldState, { + type: 'sync', + state: 'CHECKING', + }), + ).toEqual({ + ...oldState, + activeSyncState: 'CHECKING', + buildLog: [], + }); + }); + }); + + describe('"buildLog" action', () => { + it('should work', () => { + expect( + reducer(oldState, { + type: 'buildLog', + log: 'Another Line', + }), + ).toEqual({ + ...oldState, + buildLog: ['1', '2', 'Another Line'], + }); + }); }); }); @@ -228,6 +258,7 @@ describe('useReaderState', () => { state: 'CHECKING', content: undefined, errorMessage: '', + buildLog: [], }); await waitForValueToChange(() => result.current.state); @@ -236,17 +267,21 @@ describe('useReaderState', () => { state: 'CONTENT_FRESH', content: 'my content', errorMessage: '', + buildLog: [], }); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( { kind: 'Component', namespace: 'default', name: 'backstage' }, '/example', ); - expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ - kind: 'Component', - namespace: 'default', - name: 'backstage', - }); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); }); }); @@ -257,10 +292,14 @@ describe('useReaderState', () => { await new Promise(resolve => setTimeout(resolve, 500)); return 'my content'; }); - techdocsStorageApi.syncEntityDocs.mockImplementation(async () => { - await new Promise(resolve => setTimeout(resolve, 1100)); - return 'updated'; - }); + techdocsStorageApi.syncEntityDocs.mockImplementation( + async (_, logHandler) => { + logHandler?.call(this, 'Line 1'); + logHandler?.call(this, 'Line 2'); + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'updated'; + }, + ); await act(async () => { const { result, waitForValueToChange } = await renderHook( @@ -272,6 +311,7 @@ describe('useReaderState', () => { state: 'CHECKING', content: undefined, errorMessage: '', + buildLog: [], }); await waitForValueToChange(() => result.current.state); @@ -280,6 +320,7 @@ describe('useReaderState', () => { state: 'INITIAL_BUILD', content: undefined, errorMessage: ' Load error: NotFoundError: Page Not Found', + buildLog: ['Line 1', 'Line 2'], }); await waitForValueToChange(() => result.current.state); @@ -288,6 +329,7 @@ describe('useReaderState', () => { state: 'CHECKING', content: undefined, errorMessage: '', + buildLog: [], }); await waitForValueToChange(() => result.current.state); @@ -296,6 +338,7 @@ describe('useReaderState', () => { state: 'CONTENT_FRESH', content: 'my content', errorMessage: '', + buildLog: [], }); expect(techdocsStorageApi.getEntityDocs).toBeCalledTimes(2); @@ -304,20 +347,27 @@ describe('useReaderState', () => { '/example', ); expect(techdocsStorageApi.syncEntityDocs).toBeCalledTimes(1); - expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ - kind: 'Component', - namespace: 'default', - name: 'backstage', - }); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); }); }); it('should handle stale content', async () => { techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); - techdocsStorageApi.syncEntityDocs.mockImplementation(async () => { - await new Promise(resolve => setTimeout(resolve, 1100)); - return 'updated'; - }); + techdocsStorageApi.syncEntityDocs.mockImplementation( + async (_, logHandler) => { + logHandler?.call(this, 'Line 1'); + logHandler?.call(this, 'Line 2'); + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'updated'; + }, + ); await act(async () => { const { result, waitForValueToChange } = await renderHook( @@ -329,6 +379,7 @@ describe('useReaderState', () => { state: 'CHECKING', content: undefined, errorMessage: '', + buildLog: [], }); // the content is returned but the sync is in progress @@ -337,6 +388,7 @@ describe('useReaderState', () => { state: 'CONTENT_FRESH', content: 'my content', errorMessage: '', + buildLog: ['Line 1', 'Line 2'], }); // the sync takes longer than 1 seconds so the refreshing state starts @@ -345,6 +397,7 @@ describe('useReaderState', () => { state: 'CONTENT_STALE_REFRESHING', content: 'my content', errorMessage: '', + buildLog: ['Line 1', 'Line 2'], }); // the content is up-to-date @@ -353,17 +406,21 @@ describe('useReaderState', () => { state: 'CONTENT_STALE_READY', content: 'my content', errorMessage: '', + buildLog: ['Line 1', 'Line 2'], }); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( { kind: 'Component', namespace: 'default', name: 'backstage' }, '/example', ); - expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ - kind: 'Component', - namespace: 'default', - name: 'backstage', - }); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); }); }); @@ -383,6 +440,7 @@ describe('useReaderState', () => { state: 'CHECKING', content: undefined, errorMessage: '', + buildLog: [], }); // the content loading threw an error @@ -391,17 +449,21 @@ describe('useReaderState', () => { state: 'CONTENT_NOT_FOUND', content: undefined, errorMessage: ' Load error: NotFoundError: Some error description', + buildLog: [], }); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( { kind: 'Component', namespace: 'default', name: 'backstage' }, '/example', ); - expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith({ - kind: 'Component', - namespace: 'default', - name: 'backstage', - }); + expect(techdocsStorageApi.syncEntityDocs).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'backstage', + }, + expect.any(Function), + ); }); }); }); diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index be47ee301d..a2e70b5a38 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -137,7 +137,8 @@ type ReducerActions = contentLoading?: true; contentError?: Error; } - | { type: 'navigate'; path: string }; + | { type: 'navigate'; path: string } + | { type: 'buildLog'; log: string }; type ReducerState = { /** @@ -161,6 +162,11 @@ type ReducerState = { contentError?: Error; syncError?: Error; + + /** + * A list of log messages that were emitted by the build process. + */ + buildLog: string[]; }; export function reducer( @@ -171,6 +177,11 @@ export function reducer( switch (action.type) { case 'sync': + // reset the build log when a new check starts + if (action.state === 'CHECKING') { + newState.buildLog = []; + } + newState.activeSyncState = action.state; newState.syncError = action.syncError; break; @@ -185,6 +196,10 @@ export function reducer( newState.path = action.path; break; + case 'buildLog': + newState.buildLog = newState.buildLog.concat(action.log); + break; + default: throw new Error(); } @@ -195,6 +210,7 @@ export function reducer( ['content', 'navigate'].includes(action.type) ) { newState.activeSyncState = 'UP_TO_DATE'; + newState.buildLog = []; } return newState; @@ -205,11 +221,17 @@ export function useReaderState( namespace: string, name: string, path: string, -): { state: ContentStateTypes; content?: string; errorMessage?: string } { +): { + state: ContentStateTypes; + content?: string; + errorMessage?: string; + buildLog: string[]; +} { const [state, dispatch] = useReducer(reducer, { activeSyncState: 'CHECKING', path, contentLoading: true, + buildLog: [], }); const techdocsStorageApi = useApi(techdocsStorageApiRef); @@ -257,11 +279,16 @@ export function useReaderState( }, 1000); try { - const result = await techdocsStorageApi.syncEntityDocs({ - kind, - namespace, - name, - }); + const result = await techdocsStorageApi.syncEntityDocs( + { + kind, + namespace, + name, + }, + log => { + dispatch({ type: 'buildLog', log }); + }, + ); switch (result) { case 'updated': @@ -317,5 +344,6 @@ export function useReaderState( state: displayState, content: state.content, errorMessage, + buildLog: state.buildLog, }; } From 136a9197481e502b83d764b28055b8831c5d3b02 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Mon, 5 Jul 2021 14:07:44 +0200 Subject: [PATCH 04/12] Show a "Refresh" button to if the content is stale Signed-off-by: Dominik Henneke --- .changeset/techdocs-few-balloons-lie.md | 6 +++ plugins/techdocs/dev/index.tsx | 5 +++ .../techdocs/src/reader/components/Reader.tsx | 25 +++++++---- .../reader/components/useReaderState.test.tsx | 45 ++++++++++++++++++- .../src/reader/components/useReaderState.ts | 2 + 5 files changed, 73 insertions(+), 10 deletions(-) create mode 100644 .changeset/techdocs-few-balloons-lie.md diff --git a/.changeset/techdocs-few-balloons-lie.md b/.changeset/techdocs-few-balloons-lie.md new file mode 100644 index 0000000000..4e94c117ac --- /dev/null +++ b/.changeset/techdocs-few-balloons-lie.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-techdocs': patch +--- + +Show a "Refresh" button to if the content is stale. +This removes the need to do a full page-reload to display more recent TechDocs content. diff --git a/plugins/techdocs/dev/index.tsx b/plugins/techdocs/dev/index.tsx index 80e81fe178..659a4e2caf 100644 --- a/plugins/techdocs/dev/index.tsx +++ b/plugins/techdocs/dev/index.tsx @@ -148,6 +148,11 @@ createDevApp() {createPage({ + entityDocs: ({ called, content }) => { + return called === 0 + ? content + : content.replace(/World/, 'New World'); + }, syncDocs: () => 'updated', syncDocsDelay: 2000, })} diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index f8938cf8d8..dcb53dfedc 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -19,7 +19,7 @@ import { Progress } from '@backstage/core-components'; import { useApi } from '@backstage/core-plugin-api'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; import { BackstageTheme } from '@backstage/theme'; -import { CircularProgress, useTheme } from '@material-ui/core'; +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'; @@ -50,12 +50,13 @@ export const Reader = ({ entityId, onReady }: Props) => { const { '*': path } = useParams(); const theme = useTheme(); - const { state, content: rawPage, errorMessage, buildLog } = useReaderState( - kind, - namespace, - name, - path, - ); + const { + state, + contentReload, + content: rawPage, + errorMessage, + buildLog, + } = useReaderState(kind, namespace, name, path); const techdocsStorageApi = useApi(techdocsStorageApiRef); const [sidebars, setSidebars] = useState(); @@ -338,7 +339,15 @@ export const Reader = ({ entityId, onReady }: Props) => {
)} {state === 'CONTENT_STALE_READY' && ( - + contentReload()}> + Refresh + + } + > A newer version of this documentation is now available, please refresh to view. diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 3a26f91279..99ecf05bee 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -259,6 +259,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); await waitForValueToChange(() => result.current.state); @@ -268,6 +269,7 @@ describe('useReaderState', () => { content: 'my content', errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( @@ -312,6 +314,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); await waitForValueToChange(() => result.current.state); @@ -321,6 +324,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: ' Load error: NotFoundError: Page Not Found', buildLog: ['Line 1', 'Line 2'], + contentReload: expect.any(Function), }); await waitForValueToChange(() => result.current.state); @@ -330,6 +334,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); await waitForValueToChange(() => result.current.state); @@ -339,6 +344,7 @@ describe('useReaderState', () => { content: 'my content', errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); expect(techdocsStorageApi.getEntityDocs).toBeCalledTimes(2); @@ -359,7 +365,12 @@ describe('useReaderState', () => { }); it('should handle stale content', async () => { - techdocsStorageApi.getEntityDocs.mockResolvedValue('my content'); + techdocsStorageApi.getEntityDocs + .mockResolvedValueOnce('my content') + .mockImplementationOnce(async () => { + await new Promise(resolve => setTimeout(resolve, 1100)); + return 'my new content'; + }); techdocsStorageApi.syncEntityDocs.mockImplementation( async (_, logHandler) => { logHandler?.call(this, 'Line 1'); @@ -380,6 +391,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); // the content is returned but the sync is in progress @@ -389,6 +401,7 @@ describe('useReaderState', () => { content: 'my content', errorMessage: '', buildLog: ['Line 1', 'Line 2'], + contentReload: expect.any(Function), }); // the sync takes longer than 1 seconds so the refreshing state starts @@ -398,17 +411,43 @@ describe('useReaderState', () => { content: 'my content', errorMessage: '', buildLog: ['Line 1', 'Line 2'], + contentReload: expect.any(Function), }); - // the content is up-to-date + // the content is updated but not yet displayed await waitForValueToChange(() => result.current.state); expect(result.current).toEqual({ state: 'CONTENT_STALE_READY', content: 'my content', errorMessage: '', buildLog: ['Line 1', 'Line 2'], + contentReload: expect.any(Function), }); + // reload the content + result.current.contentReload(); + + // the new content refresh is triggered + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CHECKING', + content: undefined, + errorMessage: '', + buildLog: [], + contentReload: expect.any(Function), + }); + + // the new content is loaded + await waitForValueToChange(() => result.current.state); + expect(result.current).toEqual({ + state: 'CONTENT_FRESH', + content: 'my new content', + errorMessage: '', + buildLog: [], + contentReload: expect.any(Function), + }); + + expect(techdocsStorageApi.getEntityDocs).toBeCalledTimes(2); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( { kind: 'Component', namespace: 'default', name: 'backstage' }, '/example', @@ -441,6 +480,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: '', buildLog: [], + contentReload: expect.any(Function), }); // the content loading threw an error @@ -450,6 +490,7 @@ describe('useReaderState', () => { content: undefined, errorMessage: ' Load error: NotFoundError: Some error description', buildLog: [], + contentReload: expect.any(Function), }); expect(techdocsStorageApi.getEntityDocs).toBeCalledWith( diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index a2e70b5a38..f7201c84f7 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -223,6 +223,7 @@ export function useReaderState( path: string, ): { state: ContentStateTypes; + contentReload: () => void; content?: string; errorMessage?: string; buildLog: string[]; @@ -342,6 +343,7 @@ export function useReaderState( return { state: displayState, + contentReload, content: state.content, errorMessage, buildLog: state.buildLog, From 9b98328e99bfd17abf0d34a77af54c839cba2143 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Tue, 6 Jul 2021 18:38:40 +0200 Subject: [PATCH 05/12] Move the synchronization code into a dedicated class and still support non-eventstream responses Signed-off-by: Dominik Henneke --- .changeset/techdocs-loud-spies-attack.md | 6 +- plugins/techdocs-backend/src/index.ts | 2 +- .../src/service/DocsSynchronizer.test.ts | 250 ++++++++++ .../src/service/DocsSynchronizer.ts | 154 +++++++ .../src/service/router.test.ts | 428 +++++++++++------- .../techdocs-backend/src/service/router.ts | 135 +++--- 6 files changed, 732 insertions(+), 243 deletions(-) create mode 100644 plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts create mode 100644 plugins/techdocs-backend/src/service/DocsSynchronizer.ts diff --git a/.changeset/techdocs-loud-spies-attack.md b/.changeset/techdocs-loud-spies-attack.md index ea80c73116..50fbe82fbc 100644 --- a/.changeset/techdocs-loud-spies-attack.md +++ b/.changeset/techdocs-loud-spies-attack.md @@ -1,8 +1,8 @@ --- -'@backstage/plugin-techdocs': minor -'@backstage/plugin-techdocs-backend': minor +'@backstage/plugin-techdocs': patch +'@backstage/plugin-techdocs-backend': patch --- -Rewrite the `/sync/:namespace/:kind/:name` to return an event-stream. +Rewrite the `/sync/:namespace/:kind/:name` endpoint to support an event-stream as response. This change allows the sync process to take longer than a normal HTTP timeout. The stream also emits log events, so the caller can follow the build process in the frontend. diff --git a/plugins/techdocs-backend/src/index.ts b/plugins/techdocs-backend/src/index.ts index afbf04aba9..6d1e551629 100644 --- a/plugins/techdocs-backend/src/index.ts +++ b/plugins/techdocs-backend/src/index.ts @@ -14,5 +14,5 @@ * limitations under the License. */ -export * from './service/router'; +export { createRouter } from './service/router'; export * from '@backstage/techdocs-common'; diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts new file mode 100644 index 0000000000..ba411e538f --- /dev/null +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts @@ -0,0 +1,250 @@ +/* + * 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 { + getVoidLogger, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; +import { CatalogApi } from '@backstage/catalog-client'; +import { ConfigReader } from '@backstage/config'; +import { NotFoundError } from '@backstage/errors'; +import { + GeneratorBuilder, + PreparerBuilder, + PublisherBase, +} from '@backstage/techdocs-common'; +import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; +import { DocsSynchronizer, DocsSynchronizerSyncOpts } from './DocsSynchronizer'; + +jest.mock('@backstage/config'); +jest.mock('../DocsBuilder'); + +const MockedConfigReader = ConfigReader as jest.MockedClass< + typeof ConfigReader +>; +const MockedDocsBuilder = DocsBuilder as jest.MockedClass; + +describe('DocsSynchronizer', () => { + const preparers: jest.Mocked = { + register: jest.fn(), + get: jest.fn(), + }; + const generators: jest.Mocked = { + register: jest.fn(), + get: jest.fn(), + }; + const publisher: jest.Mocked = { + docsRouter: jest.fn(), + fetchTechDocsMetadata: jest.fn(), + getReadiness: jest.fn(), + hasDocsBeenGenerated: jest.fn(), + publish: jest.fn(), + }; + const discovery: jest.Mocked = { + getBaseUrl: jest.fn(), + getExternalBaseUrl: jest.fn(), + }; + const catalogClient: jest.Mocked = { + getLocationById: jest.fn(), + getEntityByName: jest.fn(), + getEntities: jest.fn(), + addLocation: jest.fn(), + removeLocationById: jest.fn(), + getOriginLocationByEntity: jest.fn(), + getLocationByEntity: jest.fn(), + removeEntityByUid: jest.fn(), + }; + + let docsSynchronizer: DocsSynchronizer; + const mockResponseHandler: jest.Mocked = { + log: jest.fn(), + finish: jest.fn(), + error: jest.fn(), + }; + + beforeEach(async () => { + publisher.docsRouter.mockReturnValue(() => {}); + discovery.getBaseUrl.mockImplementation(async type => { + return `http://backstage.local/api/${type}`; + }); + + docsSynchronizer = new DocsSynchronizer({ + preparers, + generators, + publisher, + catalogClient, + config: new ConfigReader({}), + logger: getVoidLogger(), + }); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('doSync', () => { + it('should execute an update', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); + MockedConfigReader.prototype.getString.mockReturnValue('local'); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + catalogClient.getEntityByName.mockResolvedValue(entity); + + MockedDocsBuilder.prototype.build.mockImplementation(async () => { + // extract the logStream from the constructor call + const logStream = MockedDocsBuilder.mock.calls[0][0].logStream; + + logStream?.write('Some log'); + logStream?.write('Another log'); + + return true; + }); + + publisher.hasDocsBeenGenerated.mockResolvedValue(true); + + await docsSynchronizer.doSync(() => mockResponseHandler, { + kind: 'Component', + namespace: 'default', + name: 'test', + token: undefined, + }); + + expect(catalogClient.getEntityByName).toBeCalledWith( + { + kind: 'Component', + namespace: 'default', + name: 'test', + }, + { token: undefined }, + ); + + expect(mockResponseHandler.log).toBeCalledTimes(2); + expect(mockResponseHandler.log).toBeCalledWith('Some log'); + expect(mockResponseHandler.log).toBeCalledWith('Another log'); + + expect(mockResponseHandler.finish).toBeCalledWith({ updated: true }); + + expect(mockResponseHandler.error).toBeCalledTimes(0); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); + expect(DocsBuilder.prototype.build).toBeCalledTimes(1); + }); + + it('should not check for an update too often', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(false); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + catalogClient.getEntityByName.mockResolvedValue(entity); + + await docsSynchronizer.doSync(() => mockResponseHandler, { + kind: 'Component', + namespace: 'default', + name: 'test', + token: undefined, + }); + + expect(mockResponseHandler.finish).toBeCalledWith({ updated: false }); + + expect(mockResponseHandler.log).toBeCalledTimes(0); + expect(mockResponseHandler.error).toBeCalledTimes(0); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(0); + expect(DocsBuilder.prototype.build).toBeCalledTimes(0); + }); + + it('should not check for an update without local builder', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); + MockedConfigReader.prototype.getString.mockReturnValue('external'); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + catalogClient.getEntityByName.mockResolvedValue(entity); + + await docsSynchronizer.doSync(() => mockResponseHandler, { + kind: 'Component', + namespace: 'default', + name: 'test', + token: undefined, + }); + + expect(mockResponseHandler.finish).toBeCalledWith({ updated: false }); + + expect(mockResponseHandler.log).toBeCalledTimes(0); + expect(mockResponseHandler.error).toBeCalledTimes(0); + + expect(shouldCheckForUpdate).toBeCalledTimes(1); + expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); + expect(DocsBuilder.prototype.build).toBeCalledTimes(0); + }); + + it('rejects when entity is not found', async () => { + catalogClient.getEntityByName.mockResolvedValue(undefined); + + await expect( + docsSynchronizer.doSync(() => mockResponseHandler, { + kind: 'Component', + namespace: 'default', + name: 'test', + token: undefined, + }), + ).rejects.toThrowError(NotFoundError); + + expect(mockResponseHandler.finish).toBeCalledTimes(0); + expect(mockResponseHandler.log).toBeCalledTimes(0); + expect(mockResponseHandler.error).toBeCalledTimes(0); + }); + }); +}); diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts new file mode 100644 index 0000000000..f4ffa1149d --- /dev/null +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts @@ -0,0 +1,154 @@ +/* + * 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 { CatalogApi } from '@backstage/catalog-client'; +import { Config } from '@backstage/config'; +import { NotFoundError } from '@backstage/errors'; +import { + GeneratorBuilder, + PreparerBuilder, + PublisherBase, +} from '@backstage/techdocs-common'; +import { PassThrough } from 'stream'; +import { Logger } from 'winston'; +import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; + +export type DocsSynchronizerSyncOpts = { + log: (message: string) => void; + error: (e: Error) => void; + finish: (result: { updated: boolean }) => void; +}; + +export class DocsSynchronizer { + private readonly preparers: PreparerBuilder; + private readonly generators: GeneratorBuilder; + private readonly publisher: PublisherBase; + private readonly logger: Logger; + private readonly config: Config; + private readonly catalogClient: CatalogApi; + + constructor({ + preparers, + generators, + publisher, + logger, + config, + catalogClient, + }: { + preparers: PreparerBuilder; + generators: GeneratorBuilder; + publisher: PublisherBase; + logger: Logger; + config: Config; + catalogClient: CatalogApi; + }) { + this.catalogClient = catalogClient; + this.config = config; + this.logger = logger; + this.publisher = publisher; + this.generators = generators; + this.preparers = preparers; + } + + async doSync( + initResponseHandler: () => DocsSynchronizerSyncOpts, + { + kind, + namespace, + name, + token, + }: { + kind: string; + namespace: string; + name: string; + token: string | undefined; + }, + ) { + const entity = await this.catalogClient.getEntityByName( + { kind, namespace, name }, + { token }, + ); + + if (!entity?.metadata?.uid) { + throw new NotFoundError('Entity metadata UID missing'); + } + + // open the event-stream + const { log, error, finish } = initResponseHandler(); + // create an in-memory stream to forward logs to the event-stream + const logStream = new PassThrough(); + logStream.on('data', async data => { + log(data.toString().trim()); + }); + + // check if the last update check was too recent + if (!shouldCheckForUpdate(entity.metadata.uid)) { + finish({ updated: false }); + return; + } + + // techdocs-backend will only try to build documentation for an entity if techdocs.builder is set to 'local' + // If set to 'external', it will assume that an external process (e.g. CI/CD pipeline + // of the repository) is responsible for building and publishing documentation to the storage provider + if (this.config.getString('techdocs.builder') !== 'local') { + finish({ updated: false }); + return; + } + + const docsBuilder = new DocsBuilder({ + preparers: this.preparers, + generators: this.generators, + publisher: this.publisher, + logger: this.logger, + entity, + config: this.config, + logStream, + }); + + let foundDocs = false; + + const updated = await docsBuilder.build(); + + if (!updated) { + finish({ updated: false }); + return; + } + + // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched + // on the user's page. If not, respond with a message asking them to check back later. + // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second + for (let attempt = 0; attempt < 5; attempt++) { + if (await this.publisher.hasDocsBeenGenerated(entity)) { + foundDocs = true; + break; + } + await new Promise(r => setTimeout(r, 1000)); + } + if (!foundDocs) { + this.logger.error( + 'Published files are taking longer to show up in storage. Something went wrong.', + ); + error( + new NotFoundError( + 'Sorry! It took too long for the generated docs to show up in storage. Check back later.', + ), + ); + return; + } + + finish({ updated: true }); + } +} diff --git a/plugins/techdocs-backend/src/service/router.test.ts b/plugins/techdocs-backend/src/service/router.test.ts index a3e17589ae..6216d515f7 100644 --- a/plugins/techdocs-backend/src/service/router.test.ts +++ b/plugins/techdocs-backend/src/service/router.test.ts @@ -20,34 +20,24 @@ import { PluginEndpointDiscovery, } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; +import { NotFoundError, NotModifiedError } from '@backstage/errors'; import { GeneratorBuilder, PreparerBuilder, PublisherBase, } from '@backstage/techdocs-common'; -import express from 'express'; -import { rest } from 'msw'; -import { setupServer } from 'msw/node'; +import express, { Response } from 'express'; import request from 'supertest'; -import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; -import { createRouter } from './router'; +import { DocsSynchronizer, DocsSynchronizerSyncOpts } from './DocsSynchronizer'; +import { createEventStream, createHttpResponse, createRouter } from './router'; -jest.mock('@backstage/config'); -jest.mock('../DocsBuilder'); +jest.mock('./DocsSynchronizer'); -const MockedConfigReader = ConfigReader as jest.MockedClass< - typeof ConfigReader +const MockDocsSynchronizer = DocsSynchronizer as jest.MockedClass< + typeof DocsSynchronizer >; -const MockedDocsBuilder = DocsBuilder as jest.MockedClass; - -const server = setupServer(); describe('createRouter', () => { - // the calls from supertest should not be handled by msw so we only warn onUnhandledRequest - beforeAll(() => server.listen({ onUnhandledRequest: 'warn' })); - afterAll(() => server.close()); - afterEach(() => server.resetHandlers()); - const preparers: jest.Mocked = { register: jest.fn(), get: jest.fn(), @@ -95,53 +85,124 @@ describe('createRouter', () => { }); describe('GET /sync/:namespace/:kind/:name', () => { - it('should execute an update', async () => { - (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); - MockedConfigReader.prototype.getString.mockReturnValue('local'); + describe('accept application/json', () => { + it('should execute synchronization', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => handler().finish({ updated: true }), + ); - const entity = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { - uid: '0', - name: 'test', - namespace: 'default', - annotations: { - 'sda.se/release-notes-location': - 'github-releases:https://github.com/backstage/backstage', + await request(app).get('/sync/default/Component/test').send(); + + expect(MockDocsSynchronizer.prototype.doSync).toBeCalledTimes(1); + expect(MockDocsSynchronizer.prototype.doSync).toBeCalledWith( + expect.any(Function), + { + kind: 'Component', + name: 'test', + namespace: 'default', + token: undefined, }, - }, - }; - - server.use( - rest.get( - 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', - (_req, res, ctx) => { - return res(ctx.json(entity)); - }, - ), - ); - - MockedDocsBuilder.prototype.build.mockImplementation(async () => { - // extract the logStream from the constructor call - const logStream = MockedDocsBuilder.mock.calls[0][0].logStream; - - logStream?.write('Some log'); - logStream?.write('Another log'); - - return true; + ); }); - publisher.hasDocsBeenGenerated.mockResolvedValue(true); + it('should return on updated', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => { + const { log, finish } = handler(); - const response = await request(app) - .get('/sync/default/Component/test') - .send(); + log('Some log'); - expect(response.status).toBe(200); - expect(response.get('content-type')).toBe('text/event-stream'); - expect(response.text).toEqual( - `event: log + finish({ updated: true }); + }, + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(201); + expect(response.get('content-type')).toMatch(/application\/json/); + expect(response.text).toEqual( + '{"message":"Docs updated or did not need updating"}', + ); + }); + + it('should return error', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => { + const { log, error } = handler(); + + log('Some log'); + + error(new Error('Some Error')); + }, + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(500); + expect(response.text).toMatch(/Some Error/); + }); + + it('should return not found', async () => { + MockDocsSynchronizer.prototype.doSync.mockRejectedValue( + new NotFoundError(), + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .send(); + + expect(response.status).toBe(404); + }); + }); + + describe('accept text/event-stream', () => { + it('should execute synchronization', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => handler().finish({ updated: true }), + ); + + await request(app) + .get('/sync/default/Component/test') + .set('accept', 'text/event-stream') + .send(); + + expect(MockDocsSynchronizer.prototype.doSync).toBeCalledTimes(1); + expect(MockDocsSynchronizer.prototype.doSync).toBeCalledWith( + expect.any(Function), + { + kind: 'Component', + name: 'test', + namespace: 'default', + token: undefined, + }, + ); + }); + + it('should return an event-stream', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => { + const { log, finish } = handler(); + + log('Some log'); + log('Another log'); + + finish({ updated: true }); + }, + ); + + const response = await request(app) + .get('/sync/default/Component/test') + .set('accept', 'text/event-stream') + .send(); + + expect(response.status).toBe(200); + expect(response.get('content-type')).toBe('text/event-stream'); + expect(response.text).toEqual( + `event: log data: "Some log" event: log @@ -151,117 +212,170 @@ event: finish data: {"updated":true} `, - ); + ); + }); - expect(shouldCheckForUpdate).toBeCalledTimes(1); - expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); - expect(DocsBuilder.prototype.build).toBeCalledTimes(1); - }); + it('should return error', async () => { + MockDocsSynchronizer.prototype.doSync.mockImplementation( + async handler => { + const { log, error } = handler(); - it('should not check for an update too often', async () => { - (shouldCheckForUpdate as jest.Mock).mockReturnValue(false); + log('Some log'); - const entity = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { - uid: '0', - name: 'test', - namespace: 'default', - annotations: { - 'sda.se/release-notes-location': - 'github-releases:https://github.com/backstage/backstage', + error(new Error('Some Error')); }, - }, - }; + ); - server.use( - rest.get( - 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', - (_req, res, ctx) => { - return res(ctx.json(entity)); - }, - ), - ); + const response = await request(app) + .get('/sync/default/Component/test') + .set('accept', 'text/event-stream') + .send(); - const response = await request(app) - .get('/sync/default/Component/test') - .send(); + expect(response.status).toBe(200); + expect(response.get('content-type')).toBe('text/event-stream'); + expect(response.text).toEqual( + `event: log +data: "Some log" - expect(response.status).toBe(200); - expect(response.get('content-type')).toBe('text/event-stream'); - expect(response.text).toEqual( - `event: finish -data: {"updated":false} +event: error +data: "Some Error" `, - ); + ); + }); - expect(shouldCheckForUpdate).toBeCalledTimes(1); - expect(MockedConfigReader.prototype.getString).toBeCalledTimes(0); - expect(DocsBuilder.prototype.build).toBeCalledTimes(0); - }); + it('should return not found', async () => { + MockDocsSynchronizer.prototype.doSync.mockRejectedValue( + new NotFoundError(), + ); - it('should not check for an update without local builder', async () => { - (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); - MockedConfigReader.prototype.getString.mockReturnValue('external'); + const response = await request(app) + .get('/sync/default/Component/test') + .set('accept', 'text/event-stream') + .send(); - const entity = { - apiVersion: 'backstage.io/v1alpha1', - kind: 'Component', - metadata: { - uid: '0', - name: 'test', - namespace: 'default', - annotations: { - 'sda.se/release-notes-location': - 'github-releases:https://github.com/backstage/backstage', - }, - }, - }; - - server.use( - rest.get( - 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', - (_req, res, ctx) => { - return res(ctx.json(entity)); - }, - ), - ); - - const response = await request(app) - .get('/sync/default/Component/test') - .send(); - - expect(response.status).toBe(200); - expect(response.get('content-type')).toBe('text/event-stream'); - expect(response.text).toEqual( - `event: finish -data: {"updated":false} - -`, - ); - - expect(shouldCheckForUpdate).toBeCalledTimes(1); - expect(MockedConfigReader.prototype.getString).toBeCalledTimes(1); - expect(DocsBuilder.prototype.build).toBeCalledTimes(0); - }); - - it('rejects when entity is not found', async () => { - server.use( - rest.get( - 'http://backstage.local/api/catalog/entities/by-name/Component/default/test', - (_req, res, ctx) => { - return res(ctx.status(404)); - }, - ), - ); - - const response = await request(app) - .get('/sync/default/Component/test') - .send(); - - expect(response.status).toBe(404); + expect(response.status).toBe(404); + }); }); }); }); + +describe('createEventStream', () => { + const res: jest.Mocked = { + writeHead: jest.fn(), + write: jest.fn(), + end: jest.fn(), + } as any; + + let handlers: DocsSynchronizerSyncOpts; + + beforeEach(() => { + handlers = createEventStream(res); + }); + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should return correct event stream', async () => { + // called in beforeEach + + expect(res.writeHead).toBeCalledTimes(1); + expect(res.writeHead).toBeCalledWith(200, { + 'Cache-Control': 'no-cache', + Connection: 'keep-alive', + 'Content-Type': 'text/event-stream', + }); + }); + + it('should flush after write if defined', async () => { + res.flush = jest.fn(); + + handlers.log('A Message'); + + expect(res.write).toBeCalledTimes(1); + expect(res.write).toBeCalledWith(`event: log +data: "A Message" + +`); + expect(res.flush).toBeCalledTimes(1); + }); + + it('should write log', async () => { + handlers.log('A Message'); + + expect(res.write).toBeCalledTimes(1); + expect(res.write).toBeCalledWith(`event: log +data: "A Message" + +`); + expect(res.end).toBeCalledTimes(0); + }); + + it('should write error and end the connection', async () => { + handlers.error(new Error('Some Error')); + + expect(res.write).toBeCalledTimes(1); + expect(res.write).toBeCalledWith(`event: error +data: "Some Error" + +`); + expect(res.end).toBeCalledTimes(1); + }); + + it('should finish and end the connection', async () => { + handlers.finish({ updated: true }); + + expect(res.write).toBeCalledTimes(1); + expect(res.write).toBeCalledWith(`event: finish +data: {"updated":true} + +`); + + expect(res.end).toBeCalledTimes(1); + }); +}); + +describe('createHttpResponse', () => { + const res: jest.Mocked = { + status: jest.fn(), + json: jest.fn(), + } as any; + + let handlers: DocsSynchronizerSyncOpts; + + beforeEach(() => { + res.status.mockImplementation(() => res); + handlers = createHttpResponse(res); + }); + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should return CREATED if updated', async () => { + handlers.finish({ updated: true }); + + expect(res.status).toBeCalledTimes(1); + expect(res.status).toBeCalledWith(201); + + expect(res.json).toBeCalledTimes(1); + expect(res.json).toBeCalledWith({ + message: 'Docs updated or did not need updating', + }); + }); + + it('should return NOT_MODIFIED if not updated', async () => { + expect(() => handlers.finish({ updated: false })).toThrowError( + NotModifiedError, + ); + }); + + it('should throw custom error', async () => { + expect(() => handlers.error(new Error('Some Error'))).toThrowError( + /Some Error/, + ); + }); + + it('should ignore logs', async () => { + expect(() => handlers.log('Some Message')).not.toThrow(); + }); +}); diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 08c5301f20..8b189cff16 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -17,7 +17,7 @@ import { PluginEndpointDiscovery } from '@backstage/backend-common'; import { CatalogClient } from '@backstage/catalog-client'; import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { Config } from '@backstage/config'; -import { NotFoundError } from '@backstage/errors'; +import { NotFoundError, NotModifiedError } from '@backstage/errors'; import { GeneratorBuilder, getLocationForEntity, @@ -28,9 +28,8 @@ import fetch from 'cross-fetch'; import express, { Response } from 'express'; import Router from 'express-promise-router'; import { Knex } from 'knex'; -import { PassThrough } from 'stream'; import { Logger } from 'winston'; -import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; +import { DocsSynchronizer, DocsSynchronizerSyncOpts } from './DocsSynchronizer'; type RouterOptions = { preparers: PreparerBuilder; @@ -52,6 +51,14 @@ export async function createRouter({ }: RouterOptions): Promise { const router = Router(); const catalogClient = new CatalogClient({ discoveryApi: discovery }); + const docsSynchronizer = new DocsSynchronizer({ + preparers: preparers, + generators: generators, + publisher: publisher, + logger: logger, + config: config, + catalogClient: catalogClient, + }); router.get('/metadata/techdocs/:namespace/:kind/:name', async (req, res) => { const { kind, namespace, name } = req.params; @@ -117,80 +124,21 @@ export async function createRouter({ const { kind, namespace, name } = req.params; const token = getBearerToken(req.headers.authorization); - const entity = await catalogClient.getEntityByName( - { kind, namespace, name }, - { token }, + await docsSynchronizer.doSync( + () => { + if (req.header('accept') !== 'text/event-stream') { + return createHttpResponse(res); + } + + return createEventStream(res); + }, + { + kind, + namespace, + name, + token, + }, ); - - if (!entity?.metadata?.uid) { - throw new NotFoundError('Entity metadata UID missing'); - } - - // open the event-stream - const { log, error, finish } = createEventStream(res); - - // create an in-memory stream to forward logs to the event-stream - const logStream = new PassThrough(); - logStream.on('data', async data => { - log(data.toString().trim()); - }); - - // check if the last update check was too recent - if (!shouldCheckForUpdate(entity.metadata.uid)) { - finish({ updated: false }); - return; - } - - // techdocs-backend will only try to build documentation for an entity if techdocs.builder is set to 'local' - // If set to 'external', it will assume that an external process (e.g. CI/CD pipeline - // of the repository) is responsible for building and publishing documentation to the storage provider - if (config.getString('techdocs.builder') !== 'local') { - finish({ updated: false }); - return; - } - - const docsBuilder = new DocsBuilder({ - preparers, - generators, - publisher, - logger, - entity, - config, - logStream, - }); - - let foundDocs = false; - - const updated = await docsBuilder.build(); - - if (!updated) { - finish({ updated: false }); - return; - } - - // With a maximum of ~5 seconds wait, check if the files got published and if docs will be fetched - // on the user's page. If not, respond with a message asking them to check back later. - // The delay here is to make sure GCS/AWS/etc. registers newly uploaded files which is usually <1 second - for (let attempt = 0; attempt < 5; attempt++) { - if (await publisher.hasDocsBeenGenerated(entity)) { - foundDocs = true; - break; - } - await new Promise(r => setTimeout(r, 1000)); - } - if (!foundDocs) { - logger.error( - 'Published files are taking longer to show up in storage. Something went wrong.', - ); - error( - new NotFoundError( - 'Sorry! It took too long for the generated docs to show up in storage. Check back later.', - ), - ); - return; - } - - finish({ updated: true }); }); // Route middleware which serves files from the storage set in the publisher. @@ -210,13 +158,9 @@ function getBearerToken(header?: string): string | undefined { * @returns A tuple of callbacks to emit messages. A call to 'error' or 'finish' * will close the event-stream. */ -function createEventStream( +export function createEventStream( res: Response, -): { - log: (message: string) => void; - error: (e: Error) => void; - finish: (result: { updated: boolean }) => void; -} { +): DocsSynchronizerSyncOpts { // Mandatory headers and http status to keep connection open res.writeHead(200, { Connection: 'keep-alive', @@ -255,3 +199,30 @@ function createEventStream( }, }; } + +/** + * Create a HTTP response. This is used for the legacy non-event-stream implementation of the sync endpoint. + * + * @param res the response to write the event-stream to + * @returns A tuple of callbacks to emit messages. A call to 'error' or 'finish' + * will close the event-stream. + */ +export function createHttpResponse( + res: Response, +): DocsSynchronizerSyncOpts { + return { + log: () => {}, + error: e => { + throw e; + }, + finish: ({ updated }) => { + if (!updated) { + throw new NotModifiedError(); + } + + res + .status(201) + .json({ message: 'Docs updated or did not need updating' }); + }, + }; +} From 380a519b2da4db6c4ca68fc9fab140b840106557 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Wed, 7 Jul 2021 09:47:13 +0200 Subject: [PATCH 06/12] Add a warning to the deprecated non-eventsource sync endpoint Signed-off-by: Dominik Henneke --- plugins/techdocs-backend/src/service/router.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 8b189cff16..53f262ac6c 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -127,6 +127,9 @@ export async function createRouter({ await docsSynchronizer.doSync( () => { if (req.header('accept') !== 'text/event-stream') { + console.warn( + "The /sync/:namespace/:kind/:name endpoint but the call wasn't done by an EventSource. This behavior is deprecated and will be removed soon. Make sure to update the @backstage/plugin-techdocs package in the frontend to the latest version.", + ); return createHttpResponse(res); } From 800445cca0486c6d0a28aa8cac14a12627bfe890 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Fri, 9 Jul 2021 14:24:16 +0200 Subject: [PATCH 07/12] Move the logging one level up to send all logs and exceptions to the user With this change, only failed builds are logged to the backend logger Signed-off-by: Dominik Henneke --- .changeset/techdocs-new-candles-sell.md | 4 +- packages/techdocs-common/api-report.md | 3 +- .../src/stages/generate/techdocs.ts | 35 +--------------- .../src/stages/generate/types.ts | 6 ++- .../src/DocsBuilder/builder.ts | 2 + .../src/service/DocsSynchronizer.test.ts | 41 +++++++++++++++++++ .../src/service/DocsSynchronizer.ts | 36 ++++++++++++---- 7 files changed, 83 insertions(+), 44 deletions(-) diff --git a/.changeset/techdocs-new-candles-sell.md b/.changeset/techdocs-new-candles-sell.md index b9ff9c3c4b..05443c9dbd 100644 --- a/.changeset/techdocs-new-candles-sell.md +++ b/.changeset/techdocs-new-candles-sell.md @@ -2,5 +2,5 @@ '@backstage/techdocs-common': patch --- -Provide an optional `logStream: Writable` argument to the `GeneratorBase#run(...)` command. -The stream receives all log messages that are emitted during the generator run. +Provide optional `logger: Logger` and `logStream: Writable` arguments to the `GeneratorBase#run(...)` command. +They receive all log messages that are emitted during the generator run. diff --git a/packages/techdocs-common/api-report.md b/packages/techdocs-common/api-report.md index 12bf066b78..64e619646b 100644 --- a/packages/techdocs-common/api-report.md +++ b/packages/techdocs-common/api-report.md @@ -53,6 +53,7 @@ export type GeneratorRunOptions = { outputDir: string; parsedLocationAnnotation?: ParsedLocationAnnotation; etag?: string; + logger: Logger; logStream?: Writable; }; @@ -167,7 +168,7 @@ export class TechdocsGenerator implements GeneratorBase { config: Config; }); // (undocumented) - run({ inputDir, outputDir, parsedLocationAnnotation, etag, logStream: callerLogStream, }: GeneratorRunOptions): Promise; + run({ inputDir, outputDir, parsedLocationAnnotation, etag, logger: childLogger, logStream, }: GeneratorRunOptions): Promise; } // @public diff --git a/packages/techdocs-common/src/stages/generate/techdocs.ts b/packages/techdocs-common/src/stages/generate/techdocs.ts index f31bc86905..46ffc022fc 100644 --- a/packages/techdocs-common/src/stages/generate/techdocs.ts +++ b/packages/techdocs-common/src/stages/generate/techdocs.ts @@ -17,8 +17,6 @@ import { ContainerRunner } from '@backstage/backend-common'; import { Config } from '@backstage/config'; import path from 'path'; -import { PassThrough } from 'stream'; -import * as winston from 'winston'; import { Logger } from 'winston'; import { addBuildTimestampMetadata, @@ -36,18 +34,6 @@ type TechdocsGeneratorOptions = { runGeneratorIn: string; }; -const createStream = (): [string[], PassThrough] => { - const log = [] as Array; - - const stream = new PassThrough(); - stream.on('data', chunk => { - const textValue = chunk.toString().trim(); - if (textValue?.length > 1) log.push(textValue); - }); - - return [log, stream]; -}; - export class TechdocsGenerator implements GeneratorBase { private readonly logger: Logger; private readonly containerRunner: ContainerRunner; @@ -75,25 +61,9 @@ export class TechdocsGenerator implements GeneratorBase { outputDir, parsedLocationAnnotation, etag, - logStream: callerLogStream, + logger: childLogger, + logStream, }: GeneratorRunOptions): Promise { - // create a copy of the logger. we want to keep all settings but want to add a new logger target. - // without the copy, we would add the target to the original (parent) logger. - const childLogger = winston.createLogger(this.logger); - - const [log, logStream] = createStream(); - - // Forward the logs to the caller - if (callerLogStream) { - childLogger.add( - new winston.transports.Stream({ stream: callerLogStream }), - ); - // don't use logStream.pipe(callerLogStream) because it would block others to write to callerLogStream - logStream.on('data', data => { - callerLogStream.write(data); - }); - } - // TODO: In future mkdocs.yml can be mkdocs.yaml. So, use a config variable here to find out // the correct file name. // Do some updates to mkdocs.yml before generating docs e.g. adding repo_url @@ -153,7 +123,6 @@ export class TechdocsGenerator implements GeneratorBase { this.logger.debug( `Failed to generate docs from ${inputDir} into ${outputDir}`, ); - childLogger.error(`Build failed with error: ${log}`); throw new Error( `Failed to generate docs from ${inputDir} into ${outputDir} with error ${error.message}`, ); diff --git a/packages/techdocs-common/src/stages/generate/types.ts b/packages/techdocs-common/src/stages/generate/types.ts index 5ea9721c54..185a7175dc 100644 --- a/packages/techdocs-common/src/stages/generate/types.ts +++ b/packages/techdocs-common/src/stages/generate/types.ts @@ -13,8 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { Entity } from '@backstage/catalog-model'; import { Writable } from 'stream'; +import { Logger } from 'winston'; import { ParsedLocationAnnotation } from '../../helpers'; /** @@ -24,13 +26,15 @@ import { ParsedLocationAnnotation } from '../../helpers'; * @param {string} outputDir Directory to store generated docs in. Usually - a newly created temporary directory. * @param {ParsedLocationAnnotation} parsedLocationAnnotation backstage.io/techdocs-ref annotation of an entity * @param {string} etag A unique identifier for the prepared tree e.g. commit SHA. If provided it will be stored in techdocs_metadata.json. - * @param {Writable} [logStream] A dedicated log stream + * @param {Logger} [logger] A logger that forwards the messages to the caller to be displayed outside of the backend. + * @param {Writable} [logStream] A log stream that can send raw log messages to the caller to be displayed outside of the backend.. */ export type GeneratorRunOptions = { inputDir: string; outputDir: string; parsedLocationAnnotation?: ParsedLocationAnnotation; etag?: string; + logger: Logger; logStream?: Writable; }; diff --git a/plugins/techdocs-backend/src/DocsBuilder/builder.ts b/plugins/techdocs-backend/src/DocsBuilder/builder.ts index a66125521d..2cd6f57154 100644 --- a/plugins/techdocs-backend/src/DocsBuilder/builder.ts +++ b/plugins/techdocs-backend/src/DocsBuilder/builder.ts @@ -120,6 +120,7 @@ export class DocsBuilder { try { const preparerResponse = await this.preparer.prepare(this.entity, { etag: storedEtag, + logger: this.logger, }); preparedDir = preparerResponse.preparedDir; @@ -171,6 +172,7 @@ export class DocsBuilder { outputDir, parsedLocationAnnotation, etag: newEtag, + logger: this.logger, logStream: this.logStream, }); diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts index ba411e538f..d246d499cd 100644 --- a/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.test.ts @@ -230,6 +230,47 @@ describe('DocsSynchronizer', () => { expect(DocsBuilder.prototype.build).toBeCalledTimes(0); }); + it('should forward build errors', async () => { + (shouldCheckForUpdate as jest.Mock).mockReturnValue(true); + MockedConfigReader.prototype.getString.mockReturnValue('local'); + + const entity = { + apiVersion: 'backstage.io/v1alpha1', + kind: 'Component', + metadata: { + uid: '0', + name: 'test', + namespace: 'default', + annotations: { + 'sda.se/release-notes-location': + 'github-releases:https://github.com/backstage/backstage', + }, + }, + }; + + catalogClient.getEntityByName.mockResolvedValue(entity); + + const error = new Error('Some random error'); + MockedDocsBuilder.prototype.build.mockRejectedValue(error); + + await docsSynchronizer.doSync(() => mockResponseHandler, { + kind: 'Component', + namespace: 'default', + name: 'test', + token: undefined, + }); + + expect(mockResponseHandler.log).toBeCalledTimes(1); + expect(mockResponseHandler.log).toBeCalledWith( + expect.stringMatching( + /error.*: Failed to build the docs page: Some random error/, + ), + ); + expect(mockResponseHandler.finish).toBeCalledTimes(0); + expect(mockResponseHandler.error).toBeCalledTimes(1); + expect(mockResponseHandler.error).toBeCalledWith(error); + }); + it('rejects when entity is not found', async () => { catalogClient.getEntityByName.mockResolvedValue(undefined); diff --git a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts index f4ffa1149d..2ce549c8c9 100644 --- a/plugins/techdocs-backend/src/service/DocsSynchronizer.ts +++ b/plugins/techdocs-backend/src/service/DocsSynchronizer.ts @@ -23,7 +23,7 @@ import { PublisherBase, } from '@backstage/techdocs-common'; import { PassThrough } from 'stream'; -import { Logger } from 'winston'; +import * as winston from 'winston'; import { DocsBuilder, shouldCheckForUpdate } from '../DocsBuilder'; export type DocsSynchronizerSyncOpts = { @@ -36,7 +36,7 @@ export class DocsSynchronizer { private readonly preparers: PreparerBuilder; private readonly generators: GeneratorBuilder; private readonly publisher: PublisherBase; - private readonly logger: Logger; + private readonly logger: winston.Logger; private readonly config: Config; private readonly catalogClient: CatalogApi; @@ -51,7 +51,7 @@ export class DocsSynchronizer { preparers: PreparerBuilder; generators: GeneratorBuilder; publisher: PublisherBase; - logger: Logger; + logger: winston.Logger; config: Config; catalogClient: CatalogApi; }) { @@ -88,12 +88,26 @@ export class DocsSynchronizer { // open the event-stream const { log, error, finish } = initResponseHandler(); + + // create a new logger to log data to the caller + const taskLogger = winston.createLogger({ + level: process.env.LOG_LEVEL || 'info', + format: winston.format.combine( + winston.format.colorize(), + winston.format.timestamp(), + winston.format.simple(), + ), + defaultMeta: {}, + }); + // create an in-memory stream to forward logs to the event-stream const logStream = new PassThrough(); logStream.on('data', async data => { log(data.toString().trim()); }); + taskLogger.add(new winston.transports.Stream({ stream: logStream })); + // check if the last update check was too recent if (!shouldCheckForUpdate(entity.metadata.uid)) { finish({ updated: false }); @@ -112,7 +126,7 @@ export class DocsSynchronizer { preparers: this.preparers, generators: this.generators, publisher: this.publisher, - logger: this.logger, + logger: taskLogger, entity, config: this.config, logStream, @@ -120,10 +134,18 @@ export class DocsSynchronizer { let foundDocs = false; - const updated = await docsBuilder.build(); + try { + const updated = await docsBuilder.build(); - if (!updated) { - finish({ updated: false }); + if (!updated) { + finish({ updated: false }); + return; + } + } catch (e) { + const msg = `Failed to build the docs page: ${e.message}`; + taskLogger.error(msg); + this.logger.error(msg, e); + error(e); return; } From 4255617c2ec840f417ebca330091436bf58b993c Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Fri, 9 Jul 2021 14:27:22 +0200 Subject: [PATCH 08/12] Show the error alert on the not found page in case there are sync errors This allows access to the build logs in the initial build view Signed-off-by: Dominik Henneke --- .../techdocs/src/reader/components/Reader.tsx | 20 +++++++-- .../reader/components/useReaderState.test.tsx | 42 ++++++++++++------- .../src/reader/components/useReaderState.ts | 16 ++----- 3 files changed, 49 insertions(+), 29 deletions(-) diff --git a/plugins/techdocs/src/reader/components/Reader.tsx b/plugins/techdocs/src/reader/components/Reader.tsx index dcb53dfedc..8661604eca 100644 --- a/plugins/techdocs/src/reader/components/Reader.tsx +++ b/plugins/techdocs/src/reader/components/Reader.tsx @@ -54,7 +54,8 @@ export const Reader = ({ entityId, onReady }: Props) => { state, contentReload, content: rawPage, - errorMessage, + contentErrorMessage, + syncErrorMessage, buildLog, } = useReaderState(kind, namespace, name, path); @@ -358,11 +359,24 @@ export const Reader = ({ entityId, onReady }: Props) => { severity="error" action={} > - Building a newer version of this documentation failed. {errorMessage} + Building a newer version of this documentation failed.{' '} + {syncErrorMessage} )} {state === 'CONTENT_NOT_FOUND' && ( - + <> + {syncErrorMessage && ( + } + > + Building a newer version of this documentation failed.{' '} + {syncErrorMessage} + + )} + + )}
diff --git a/plugins/techdocs/src/reader/components/useReaderState.test.tsx b/plugins/techdocs/src/reader/components/useReaderState.test.tsx index 99ecf05bee..9d9a8e2b1a 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.test.tsx +++ b/plugins/techdocs/src/reader/components/useReaderState.test.tsx @@ -257,7 +257,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -267,7 +268,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', content: 'my content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -312,7 +314,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -322,7 +325,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'INITIAL_BUILD', content: undefined, - errorMessage: ' Load error: NotFoundError: Page Not Found', + contentErrorMessage: 'NotFoundError: Page Not Found', + syncErrorMessage: undefined, buildLog: ['Line 1', 'Line 2'], contentReload: expect.any(Function), }); @@ -332,7 +336,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -342,7 +347,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', content: 'my content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -389,7 +395,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -399,7 +406,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', content: 'my content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: ['Line 1', 'Line 2'], contentReload: expect.any(Function), }); @@ -409,7 +417,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_STALE_REFRESHING', content: 'my content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: ['Line 1', 'Line 2'], contentReload: expect.any(Function), }); @@ -419,7 +428,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_STALE_READY', content: 'my content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: ['Line 1', 'Line 2'], contentReload: expect.any(Function), }); @@ -432,7 +442,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -442,7 +453,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_FRESH', content: 'my new content', - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -478,7 +490,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CHECKING', content: undefined, - errorMessage: '', + contentErrorMessage: undefined, + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); @@ -488,7 +501,8 @@ describe('useReaderState', () => { expect(result.current).toEqual({ state: 'CONTENT_NOT_FOUND', content: undefined, - errorMessage: ' Load error: NotFoundError: Some error description', + contentErrorMessage: 'NotFoundError: Some error description', + syncErrorMessage: undefined, buildLog: [], contentReload: expect.any(Function), }); diff --git a/plugins/techdocs/src/reader/components/useReaderState.ts b/plugins/techdocs/src/reader/components/useReaderState.ts index f7201c84f7..178c0746bf 100644 --- a/plugins/techdocs/src/reader/components/useReaderState.ts +++ b/plugins/techdocs/src/reader/components/useReaderState.ts @@ -225,7 +225,8 @@ export function useReaderState( state: ContentStateTypes; contentReload: () => void; content?: string; - errorMessage?: string; + contentErrorMessage?: string; + syncErrorMessage?: string; buildLog: string[]; } { const [state, dispatch] = useReducer(reducer, { @@ -331,21 +332,12 @@ export function useReaderState( [state.activeSyncState, state.content, state.contentLoading], ); - const errorMessage = useMemo(() => { - let errMessage = ''; - if (state.contentError) { - errMessage += ` Load error: ${state.contentError}`; - } - if (state.syncError) errMessage += ` Build error: ${state.syncError}`; - - return errMessage; - }, [state.syncError, state.contentError]); - return { state: displayState, contentReload, content: state.content, - errorMessage, + contentErrorMessage: state.contentError?.toString(), + syncErrorMessage: state.syncError?.toString(), buildLog: state.buildLog, }; } From 221b3080f9ca5854f437c0faa214592837c6b4ec Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Fri, 9 Jul 2021 14:38:50 +0200 Subject: [PATCH 09/12] Fix warning Signed-off-by: Dominik Henneke --- plugins/techdocs-backend/src/service/router.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 53f262ac6c..02f10eef09 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -128,7 +128,7 @@ export async function createRouter({ () => { if (req.header('accept') !== 'text/event-stream') { console.warn( - "The /sync/:namespace/:kind/:name endpoint but the call wasn't done by an EventSource. This behavior is deprecated and will be removed soon. Make sure to update the @backstage/plugin-techdocs package in the frontend to the latest version.", + "The call to /sync/:namespace/:kind/:name wasn't done by an EventSource. This behavior is deprecated and will be removed soon. Make sure to update the @backstage/plugin-techdocs package in the frontend to the latest version.", ); return createHttpResponse(res); } From fbba17140540ef0dde37b31a45dea315d60c20a1 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Fri, 9 Jul 2021 14:49:01 +0200 Subject: [PATCH 10/12] Update api report Signed-off-by: Dominik Henneke --- packages/techdocs-common/api-report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/techdocs-common/api-report.md b/packages/techdocs-common/api-report.md index 789cb9c890..f9687103ef 100644 --- a/packages/techdocs-common/api-report.md +++ b/packages/techdocs-common/api-report.md @@ -53,7 +53,7 @@ export type GeneratorRunOptions = { outputDir: string; parsedLocationAnnotation?: ParsedLocationAnnotation; etag?: string; - logger: Logger; + logger: Logger_2; logStream?: Writable; }; From 2d58075d46348cf94c255c05f19559ac4f010354 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Tue, 13 Jul 2021 16:48:20 +0200 Subject: [PATCH 11/12] Display the correct error message Signed-off-by: Dominik Henneke --- plugins/techdocs/src/client.test.ts | 4 ++-- plugins/techdocs/src/client.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/techdocs/src/client.test.ts b/plugins/techdocs/src/client.test.ts index 4cc7aca6a6..982c9496b2 100644 --- a/plugins/techdocs/src/client.test.ts +++ b/plugins/techdocs/src/client.test.ts @@ -242,8 +242,8 @@ describe('TechDocsStorageClient', () => { .instances[0] as jest.Mocked; instance.onerror({ - status: 500, - message: 'Some other error', + type: 'error', + data: 'Some other error', } as any); await expect(promise).rejects.toThrow(Error); diff --git a/plugins/techdocs/src/client.ts b/plugins/techdocs/src/client.ts index 29e6e018d0..afbf4b74a0 100644 --- a/plugins/techdocs/src/client.ts +++ b/plugins/techdocs/src/client.ts @@ -241,7 +241,7 @@ export class TechDocsStorageClient implements TechDocsStorageApi { // also handles the event-stream close. the reject is ignored if the Promise was already // resolved by a finish event. default: - reject(new Error(e.message)); + reject(new Error(e.data)); return; } }; From 386e95b94c2e987a8809d326a65af00ee8b8cff2 Mon Sep 17 00:00:00 2001 From: Dominik Henneke Date: Tue, 13 Jul 2021 16:56:30 +0200 Subject: [PATCH 12/12] Add missing comment Signed-off-by: Dominik Henneke --- plugins/techdocs-backend/src/service/router.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/techdocs-backend/src/service/router.ts b/plugins/techdocs-backend/src/service/router.ts index 38a4908b50..b889312abd 100644 --- a/plugins/techdocs-backend/src/service/router.ts +++ b/plugins/techdocs-backend/src/service/router.ts @@ -176,6 +176,7 @@ export async function createRouter( return; } + // Set the synchronization and build process if "out-of-the-box" configuration is provided. if (isOutOfTheBoxOption(options)) { const { preparers, generators } = options;