From ae57dd4ac0fb92112cec118381291ed759d865cc Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 6 Apr 2023 17:37:35 +0200 Subject: [PATCH] config-loader: apply review feedback Signed-off-by: Patrik Oldsberg --- packages/backend-app-api/api-report.md | 2 +- .../config/configServiceFactory.ts | 2 +- packages/config-loader/api-report.md | 5 ++- packages/config-loader/package.json | 4 +- packages/config-loader/src/loader.ts | 4 +- .../src/sources/ConfigSources.test.ts | 4 +- .../src/sources/ConfigSources.ts | 12 ++++-- .../src/sources/FileConfigSource.ts | 6 ++- .../src/sources/RemoteConfigSource.test.ts | 2 +- .../src/sources/RemoteConfigSource.ts | 40 ++++++++++++++----- yarn.lock | 8 ++-- 11 files changed, 60 insertions(+), 29 deletions(-) diff --git a/packages/backend-app-api/api-report.md b/packages/backend-app-api/api-report.md index f3958fde29..27b218ecb3 100644 --- a/packages/backend-app-api/api-report.md +++ b/packages/backend-app-api/api-report.md @@ -55,7 +55,7 @@ export const cacheServiceFactory: () => ServiceFactory; // @public (undocumented) export interface ConfigFactoryOptions { argv?: string[]; - remote?: Pick; + remote?: Pick; } // @public (undocumented) diff --git a/packages/backend-app-api/src/services/implementations/config/configServiceFactory.ts b/packages/backend-app-api/src/services/implementations/config/configServiceFactory.ts index 3643dbc3a5..d3b29ce545 100644 --- a/packages/backend-app-api/src/services/implementations/config/configServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/config/configServiceFactory.ts @@ -33,7 +33,7 @@ export interface ConfigFactoryOptions { /** * Enables and sets options for remote configuration loading. */ - remote?: Pick; + remote?: Pick; } /** @public */ diff --git a/packages/config-loader/api-report.md b/packages/config-loader/api-report.md index 9c9f16da9c..662446ad03 100644 --- a/packages/config-loader/api-report.md +++ b/packages/config-loader/api-report.md @@ -5,6 +5,7 @@ ```ts import { AppConfig } from '@backstage/config'; import { Config } from '@backstage/config'; +import { HumanDuration } from '@backstage/types'; import { JsonObject } from '@backstage/types'; import { JSONSchema7 } from 'json-schema'; import { Observable } from '@backstage/types'; @@ -31,7 +32,7 @@ export interface AsyncConfigSourceIterator // @public export interface BaseConfigSourcesOptions { // (undocumented) - remote?: Pick; + remote?: Pick; // (undocumented) rootDir?: string; // (undocumented) @@ -246,7 +247,7 @@ export class RemoteConfigSource implements ConfigSource { // @public export interface RemoteConfigSourceOptions { - reloadIntervalSeconds?: number; + reloadInterval?: HumanDuration; substitutionFunc?: EnvFunc; url: string; } diff --git a/packages/config-loader/package.json b/packages/config-loader/package.json index 42886dc597..bf2b82345b 100644 --- a/packages/config-loader/package.json +++ b/packages/config-loader/package.json @@ -44,8 +44,8 @@ "json-schema": "^0.4.0", "json-schema-merge-allof": "^0.8.1", "json-schema-traverse": "^1.0.0", - "lodash": "^4.14.151", - "minimist": "^1.2.8", + "lodash": "^4.17.21", + "minimist": "^1.2.5", "node-fetch": "^2.6.7", "typescript-json-schema": "^0.55.0", "yaml": "^2.0.0", diff --git a/packages/config-loader/src/loader.ts b/packages/config-loader/src/loader.ts index 743adf592c..8b6fd2a953 100644 --- a/packages/config-loader/src/loader.ts +++ b/packages/config-loader/src/loader.ts @@ -104,7 +104,9 @@ export async function loadConfig( ): Promise { const source = ConfigSources.default({ substitutionFunc: options.experimentalEnvFunc, - remote: options.remote, + remote: options.remote && { + reloadInterval: { seconds: options.remote.reloadIntervalSeconds }, + }, rootDir: options.configRoot, argv: options.configTargets.flatMap(t => [ '--config', diff --git a/packages/config-loader/src/sources/ConfigSources.test.ts b/packages/config-loader/src/sources/ConfigSources.test.ts index b2fe125e55..190515aa12 100644 --- a/packages/config-loader/src/sources/ConfigSources.test.ts +++ b/packages/config-loader/src/sources/ConfigSources.test.ts @@ -135,14 +135,14 @@ describe('ConfigSources', () => { ConfigSources.defaultForTargets({ rootDir: '/', targets: [{ type: 'url', target: 'http://example.com/config.yaml' }], - remote: { reloadIntervalSeconds: 5 }, + remote: { reloadInterval: { minutes: 2 } }, }), ), ).toEqual([ { name: 'RemoteConfigSource', url: 'http://example.com/config.yaml', - reloadIntervalSeconds: 5, + reloadInterval: { minutes: 2 }, }, ]); }); diff --git a/packages/config-loader/src/sources/ConfigSources.ts b/packages/config-loader/src/sources/ConfigSources.ts index 069c8e642d..d60e9d53e9 100644 --- a/packages/config-loader/src/sources/ConfigSources.ts +++ b/packages/config-loader/src/sources/ConfigSources.ts @@ -72,7 +72,7 @@ export interface ClosableConfig extends Config { */ export interface BaseConfigSourcesOptions { rootDir?: string; - remote?: Pick; + remote?: Pick; substitutionFunc?: SubstitutionFunc; } @@ -112,8 +112,12 @@ export class ConfigSources { const args: string[] = [parseArgs(argv).config].flat().filter(Boolean); return args.map(target => { try { - // eslint-disable-next-line no-new - new URL(target); + const url = new URL(target); + + // Some file paths are valid relative URLs, so check if the host is empty too + if (!url.host) { + return { type: 'path', target }; + } return { type: 'url', target }; } catch { return { type: 'path', target }; @@ -151,7 +155,7 @@ export class ConfigSources { return RemoteConfigSource.create({ url: arg.target, substitutionFunc: options.substitutionFunc, - reloadIntervalSeconds: options.remote.reloadIntervalSeconds, + reloadInterval: options.remote.reloadInterval, }); } return FileConfigSource.create({ diff --git a/packages/config-loader/src/sources/FileConfigSource.ts b/packages/config-loader/src/sources/FileConfigSource.ts index b6d7181e02..6e0cf6fde7 100644 --- a/packages/config-loader/src/sources/FileConfigSource.ts +++ b/packages/config-loader/src/sources/FileConfigSource.ts @@ -147,9 +147,11 @@ export class FileConfigSource implements ConfigSource { } }; - signal?.addEventListener('abort', () => { + const onAbort = () => { + signal?.removeEventListener('abort', onAbort); watcher.close(); - }); + }; + signal?.addEventListener('abort', onAbort); yield { configs: await readConfigFile() }; diff --git a/packages/config-loader/src/sources/RemoteConfigSource.test.ts b/packages/config-loader/src/sources/RemoteConfigSource.test.ts index c62e33abc9..6082c05e65 100644 --- a/packages/config-loader/src/sources/RemoteConfigSource.test.ts +++ b/packages/config-loader/src/sources/RemoteConfigSource.test.ts @@ -74,7 +74,7 @@ app: const source = RemoteConfigSource.create({ url: 'http://localhost/config.yaml', - reloadIntervalSeconds: 0, + reloadInterval: { seconds: 0 }, }); await expect(readN(source, 2)).resolves.toEqual([ diff --git a/packages/config-loader/src/sources/RemoteConfigSource.ts b/packages/config-loader/src/sources/RemoteConfigSource.ts index b8917a6f13..71aa800e59 100644 --- a/packages/config-loader/src/sources/RemoteConfigSource.ts +++ b/packages/config-loader/src/sources/RemoteConfigSource.ts @@ -15,7 +15,7 @@ */ import { ResponseError } from '@backstage/errors'; -import { JsonObject } from '@backstage/types'; +import { HumanDuration, JsonObject } from '@backstage/types'; import isEqual from 'lodash/isEqual'; import fetch from 'node-fetch'; import yaml from 'yaml'; @@ -27,7 +27,28 @@ import { ReadConfigDataOptions, } from './types'; -const DEFAULT_RELOAD_INTERVAL_SECONDS = 60; +const DEFAULT_RELOAD_INTERVAL = { seconds: 60 }; + +function durationToMs(duration: HumanDuration): number { + const { + years = 0, + months = 0, + weeks = 0, + days = 0, + hours = 0, + minutes = 0, + seconds = 0, + milliseconds = 0, + } = duration; + + const totalDays = years * 365 + months * 30 + weeks * 7 + days; + const totalHours = totalDays * 24 + hours; + const totalMinutes = totalHours * 60 + minutes; + const totalSeconds = totalMinutes * 60 + seconds; + const totalMilliseconds = totalSeconds * 1000 + milliseconds; + + return totalMilliseconds; +} /** * Options for {@link RemoteConfigSource.create}. @@ -45,7 +66,7 @@ export interface RemoteConfigSourceOptions { * * Set to Infinity to disable reloading. */ - reloadIntervalSeconds?: number; + reloadInterval?: HumanDuration; /** * A substitution function to use instead of the default environment substitution. @@ -78,13 +99,14 @@ export class RemoteConfigSource implements ConfigSource { } readonly #url: string; - readonly #reloadIntervalSeconds: number; + readonly #reloadIntervalMs: number; readonly #transformer: ConfigTransformer; private constructor(options: RemoteConfigSourceOptions) { this.#url = options.url; - this.#reloadIntervalSeconds = - options.reloadIntervalSeconds ?? DEFAULT_RELOAD_INTERVAL_SECONDS; + this.#reloadIntervalMs = durationToMs( + options.reloadInterval ?? DEFAULT_RELOAD_INTERVAL, + ); this.#transformer = createConfigTransformer({ substitutionFunc: options.substitutionFunc, }); @@ -98,12 +120,12 @@ export class RemoteConfigSource implements ConfigSource { yield { configs: [{ data, context: this.#url }] }; for (;;) { + await this.#wait(options?.signal); + if (options?.signal?.aborted) { return; } - await this.#wait(options?.signal); - try { const newData = await this.#load(options?.signal); if (newData && !isEqual(data, newData)) { @@ -146,7 +168,7 @@ export class RemoteConfigSource implements ConfigSource { async #wait(signal?: AbortSignal) { return new Promise(resolve => { - const timeoutId = setTimeout(onDone, this.#reloadIntervalSeconds * 1000); + const timeoutId = setTimeout(onDone, this.#reloadIntervalMs); signal?.addEventListener('abort', onDone); function onDone() { diff --git a/yarn.lock b/yarn.lock index c35f795817..ee5ebf0cf7 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3940,8 +3940,8 @@ __metadata: json-schema: ^0.4.0 json-schema-merge-allof: ^0.8.1 json-schema-traverse: ^1.0.0 - lodash: ^4.14.151 - minimist: ^1.2.8 + lodash: ^4.17.21 + minimist: ^1.2.5 mock-fs: ^5.1.0 msw: ^1.0.0 node-fetch: ^2.6.7 @@ -29512,7 +29512,7 @@ __metadata: languageName: node linkType: hard -"lodash@npm:4.17.21, lodash@npm:^4.14.151, lodash@npm:^4.15.0, lodash@npm:^4.17.10, lodash@npm:^4.17.11, lodash@npm:^4.17.14, lodash@npm:^4.17.15, lodash@npm:^4.17.19, lodash@npm:^4.17.20, lodash@npm:^4.17.21, lodash@npm:^4.17.4, lodash@npm:^4.7.0, lodash@npm:~4.17.0, lodash@npm:~4.17.15": +"lodash@npm:4.17.21, lodash@npm:^4.15.0, lodash@npm:^4.17.10, lodash@npm:^4.17.11, lodash@npm:^4.17.14, lodash@npm:^4.17.15, lodash@npm:^4.17.19, lodash@npm:^4.17.20, lodash@npm:^4.17.21, lodash@npm:^4.17.4, lodash@npm:^4.7.0, lodash@npm:~4.17.0, lodash@npm:~4.17.15": version: 4.17.21 resolution: "lodash@npm:4.17.21" checksum: eb835a2e51d381e561e508ce932ea50a8e5a68f4ebdd771ea240d3048244a8d13658acbd502cd4829768c56f2e16bdd4340b9ea141297d472517b83868e677f7 @@ -30756,7 +30756,7 @@ __metadata: languageName: node linkType: hard -"minimist@npm:>=1.2.2, minimist@npm:^1.2.0, minimist@npm:^1.2.3, minimist@npm:^1.2.5, minimist@npm:^1.2.6, minimist@npm:^1.2.7, minimist@npm:^1.2.8": +"minimist@npm:>=1.2.2, minimist@npm:^1.2.0, minimist@npm:^1.2.3, minimist@npm:^1.2.5, minimist@npm:^1.2.6, minimist@npm:^1.2.7": version: 1.2.8 resolution: "minimist@npm:1.2.8" checksum: 75a6d645fb122dad29c06a7597bddea977258957ed88d7a6df59b5cd3fe4a527e253e9bbf2e783e4b73657f9098b96a5fe96ab8a113655d4109108577ecf85b0