From 1136ed83e3e3ba30a031e80bc0bdea11be4d0c36 Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Sun, 2 Jan 2022 11:39:33 -0500 Subject: [PATCH 1/6] wip: warn users of deprecated keys in app configurations Signed-off-by: Colton Padden --- packages/config-loader/api-report.md | 1 + packages/config-loader/package.json | 1 + .../config-loader/src/lib/schema/collect.ts | 4 +- .../src/lib/schema/compile.test.ts | 33 +++++++++++ .../config-loader/src/lib/schema/compile.ts | 9 +++ .../src/lib/schema/filtering.test.ts | 27 ++++++++- .../config-loader/src/lib/schema/filtering.ts | 31 ++++++++++ .../config-loader/src/lib/schema/load.test.ts | 56 ++++++++++++++++--- packages/config-loader/src/lib/schema/load.ts | 18 +++++- .../config-loader/src/lib/schema/types.ts | 14 +++++ packages/config/api-report.md | 4 ++ packages/config/src/reader.ts | 20 ++++++- packages/config/src/types.ts | 6 ++ 13 files changed, 210 insertions(+), 14 deletions(-) diff --git a/packages/config-loader/api-report.md b/packages/config-loader/api-report.md index 269159aab4..6a02a5632f 100644 --- a/packages/config-loader/api-report.md +++ b/packages/config-loader/api-report.md @@ -21,6 +21,7 @@ export type ConfigSchemaProcessingOptions = { visibility?: ConfigVisibility[]; valueTransform?: TransformFunc; withFilteredKeys?: boolean; + withDeprecatedKeys?: boolean; }; // Warning: (ae-missing-release-tag) "ConfigTarget" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/packages/config-loader/package.json b/packages/config-loader/package.json index d909cde58c..2ecea8e843 100644 --- a/packages/config-loader/package.json +++ b/packages/config-loader/package.json @@ -41,6 +41,7 @@ "json-schema": "^0.4.0", "json-schema-merge-allof": "^0.8.1", "json-schema-traverse": "^1.0.0", + "lodash": "^4.17.21", "node-fetch": "^2.6.1", "typescript-json-schema": "^0.52.0", "yaml": "^1.9.2", diff --git a/packages/config-loader/src/lib/schema/collect.ts b/packages/config-loader/src/lib/schema/collect.ts index 8e7f582e44..aa3202a1b5 100644 --- a/packages/config-loader/src/lib/schema/collect.ts +++ b/packages/config-loader/src/lib/schema/collect.ts @@ -182,10 +182,10 @@ function compileTsSchemas(paths: string[]) { program, // All schemas should export a `Config` symbol 'Config', - // This enables usage of @visibility is doc comments + // This enables usage of @visibility and @deprecated is doc comments { required: true, - validationKeywords: ['visibility'], + validationKeywords: ['visibility', 'deprecated'], }, [path.split(sep).join('/')], // Unix paths are expected for all OSes here ) as JsonObject | null; diff --git a/packages/config-loader/src/lib/schema/compile.test.ts b/packages/config-loader/src/lib/schema/compile.test.ts index 1e63f3cbf8..7615fa80f5 100644 --- a/packages/config-loader/src/lib/schema/compile.test.ts +++ b/packages/config-loader/src/lib/schema/compile.test.ts @@ -40,6 +40,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), + deprecationBySchemaPath: new Map(), }); expect(validate([{ data: { b: 'b' }, context: 'test' }])).toEqual({ errors: [ @@ -53,6 +54,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), + deprecationBySchemaPath: new Map(), }); }); @@ -112,6 +114,7 @@ describe('compileConfigSchemas', () => { '/properties/d/items': 'frontend', }), ), + deprecationBySchemaPath: new Map(), }); }); @@ -137,4 +140,34 @@ describe('compileConfigSchemas', () => { "Config schema visibility is both 'frontend' and 'secret' for properties/a/visibility", ); }); + + it('should discover deprecations', () => { + const validate = compileConfigSchemas([ + { + path: 'a1', + value: { + type: 'object', + properties: { + a: { type: 'string', deprecated: 'deprecation reason for a' }, + b: { type: 'string', deprecated: 'deprecation reason for b' }, + c: { type: 'string' }, + }, + }, + }, + ]); + expect( + validate([ + { data: { a: 'a', b: 'b', c: 'c', d: ['d'] }, context: 'test' }, + ]), + ).toEqual({ + deprecationBySchemaPath: new Map( + Object.entries({ + '/properties/a': 'deprecation reason for a', + '/properties/b': 'deprecation reason for b', + }), + ), + visibilityByDataPath: new Map(), + visibilityBySchemaPath: new Map(), + }); + }); }); diff --git a/packages/config-loader/src/lib/schema/compile.ts b/packages/config-loader/src/lib/schema/compile.ts index 1cbf5a404d..6dcd4713f5 100644 --- a/packages/config-loader/src/lib/schema/compile.ts +++ b/packages/config-loader/src/lib/schema/compile.ts @@ -81,6 +81,13 @@ export function compileConfigSchemas( const merged = mergeConfigSchemas(schemas.map(_ => _.value)); const validate = ajv.compile(merged); + const deprecationBySchemaPath = new Map(); + traverse(merged, (schema, path) => { + if ('deprecated' in schema) { + deprecationBySchemaPath.set(path, schema.deprecated); + } + }); + const visibilityBySchemaPath = new Map(); traverse(merged, (schema, path) => { if (schema.visibility && schema.visibility !== 'backend') { @@ -99,12 +106,14 @@ export function compileConfigSchemas( errors: validate.errors ?? [], visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, + deprecationBySchemaPath, }; } return { visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, + deprecationBySchemaPath, }; }; } diff --git a/packages/config-loader/src/lib/schema/filtering.test.ts b/packages/config-loader/src/lib/schema/filtering.test.ts index be07b915ed..2324bebc9d 100644 --- a/packages/config-loader/src/lib/schema/filtering.test.ts +++ b/packages/config-loader/src/lib/schema/filtering.test.ts @@ -16,7 +16,11 @@ import { JsonObject } from '@backstage/types'; import { ConfigVisibility } from './types'; -import { filterByVisibility, filterErrorsByVisibility } from './filtering'; +import { + filterByVisibility, + filterErrorsByVisibility, + filterByDeprecated, +} from './filtering'; const data = { arr: ['f', 'b', 's'], @@ -64,6 +68,13 @@ const visibility = new Map( }), ); +const deprecations = new Map( + Object.entries({ + '/properties/arr': 'deprecated array', + '/properties/objB/properties/never': 'deprecated nested property', + }), +); + describe('filterByVisibility', () => { test.each<[ConfigVisibility[], JsonObject]>([ [ @@ -381,3 +392,17 @@ describe('filterErrorsByVisibility', () => { ]); }); }); + +describe('filterByDeprecated', () => { + it('should allow empty list of deprecations', () => { + expect(filterByDeprecated(data, new Map())).toEqual({ deprecatedKeys: [] }); + }); + it('should return a list of deprecated properties', () => { + expect(filterByDeprecated(data, deprecations)).toEqual({ + deprecatedKeys: [ + { key: 'arr', description: 'deprecated array' }, + { key: 'objB.never', description: 'deprecated nested property' }, + ], + }); + }); +}); diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index b2f39d3cc4..f955343066 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -15,6 +15,7 @@ */ import { JsonObject, JsonValue } from '@backstage/types'; +import { has } from 'lodash'; import { ConfigVisibility, DEFAULT_CONFIG_VISIBILITY, @@ -22,6 +23,36 @@ import { ValidationError, } from './types'; +/** + * This filters data by deprecation status to only include those which have been deprecated + * + * @param data - configuration data + * @param deprecationBySchemaPath - mapping of schema path to deprecation description + * @returns deprecated configuration keys with their deprecation description + */ +export function filterByDeprecated( + data: JsonObject, + deprecationBySchemaPath: Map, + withDeprecatedKeys: boolean = true, +): { deprecatedKeys: { key: string; description: string }[] } { + const deprecatedKeys = new Array<{ key: string; description: string }>(); + if (!withDeprecatedKeys) { + return { deprecatedKeys }; + } + + deprecationBySchemaPath.forEach((desc, schemaPath) => { + // convert schema path to object path (/properties/techdocs/properties/storageUrl -> techdocs.storageUrl) + const propertyPath = schemaPath + .replace(/^\/properties\//, '') // remove leading `/properties/` entirely + .replace(/\/properties\//g, '.'); // replace all other `/properties/` with a `.` + if (has(data, propertyPath)) { + deprecatedKeys.push({ key: propertyPath, description: desc }); + } + }); + + return { deprecatedKeys }; +} + /** * This filters data by visibility by discovering the visibility of each * value, and then only keeping the ones that are specified in `includeVisibilities`. diff --git a/packages/config-loader/src/lib/schema/load.test.ts b/packages/config-loader/src/lib/schema/load.test.ts index e2ae3cc2c7..a2f1df8d5f 100644 --- a/packages/config-loader/src/lib/schema/load.test.ts +++ b/packages/config-loader/src/lib/schema/load.test.ts @@ -62,7 +62,11 @@ describe('loadConfigSchema', () => { expect(schema.process(configs)).toEqual(configs); expect(schema.process(configs, { visibility: ['frontend'] })).toEqual([ - { data: { key1: 'a' }, context: 'test' }, + { + data: { key1: 'a' }, + context: 'test', + deprecatedKeys: [], + }, ]); expect( schema.process(configs, { @@ -71,7 +75,12 @@ describe('loadConfigSchema', () => { withFilteredKeys: true, }), ).toEqual([ - { data: { key1: 'X' }, context: 'test', filteredKeys: ['key2'] }, + { + data: { key1: 'X' }, + context: 'test', + filteredKeys: ['key2'], + deprecatedKeys: [], + }, ]); expect( schema.process(configs, { @@ -79,14 +88,23 @@ describe('loadConfigSchema', () => { withFilteredKeys: true, }), ).toEqual([ - { data: { key1: 'X', key2: 'X' }, context: 'test', filteredKeys: [] }, + { + data: { key1: 'X', key2: 'X' }, + context: 'test', + filteredKeys: [], + deprecatedKeys: [], + }, ]); const serialized = schema.serialize(); const schema2 = await loadConfigSchema({ serialized }); expect(schema2.process(configs, { visibility: ['frontend'] })).toEqual([ - { data: { key1: 'a' }, context: 'test' }, + { + data: { key1: 'a' }, + context: 'test', + deprecatedKeys: [], + }, ]); expect(() => schema2.process([...configs, { data: { key1: 3 }, context: 'test2' }]), @@ -131,7 +149,11 @@ describe('loadConfigSchema', () => { 'Config validation failed, Config should be number { type=number } at /key2', ); expect(schema.process(configs, { visibility: ['frontend'] })).toEqual([ - { data: { key1: 'a' }, context: 'test' }, + { + data: { key1: 'a' }, + context: 'test', + deprecatedKeys: [], + }, ]); expect(() => schema.process(configs, { visibility: ['secret'] })).toThrow( 'Config validation failed, Config should be number { type=number } at /key2', @@ -179,7 +201,13 @@ describe('loadConfigSchema', () => { ]; expect( schema.process(mkConfig({ x: 1 }), { visibility: ['frontend'] }), - ).toEqual([{ data: { nested: [{}] }, context: 'test' }]); + ).toEqual([ + { + data: { nested: [{}] }, + context: 'test', + deprecatedKeys: [], + }, + ]); expect(() => schema.process(mkConfig({ y: 1 }))).toThrow( 'Config validation failed, Config should be string { type=string } at /nested/0/y', ); @@ -190,10 +218,22 @@ describe('loadConfigSchema', () => { ); expect( schema.process(mkConfig({ x: 'a' }), { visibility: ['frontend'] }), - ).toEqual([{ data: { nested: [{}] }, context: 'test' }]); + ).toEqual([ + { + data: { nested: [{}] }, + context: 'test', + deprecatedKeys: [], + }, + ]); expect( schema.process(mkConfig({ y: 'aaa' }), { visibility: ['frontend'] }), - ).toEqual([{ data: { nested: [{ y: 'aaa' }] }, context: 'test' }]); + ).toEqual([ + { + data: { nested: [{ y: 'aaa' }] }, + context: 'test', + deprecatedKeys: [], + }, + ]); expect(() => schema.process(mkConfig({ y: 'aaaa' }), { visibility: ['frontend'] }), ).toThrow( diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index 2d78785304..303fdfade9 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -18,7 +18,11 @@ import { AppConfig } from '@backstage/config'; import { JsonObject } from '@backstage/types'; import { compileConfigSchemas } from './compile'; import { collectConfigSchemas } from './collect'; -import { filterByVisibility, filterErrorsByVisibility } from './filtering'; +import { + filterByVisibility, + filterErrorsByVisibility, + filterByDeprecated, +} from './filtering'; import { ValidationError, ConfigSchema, @@ -82,7 +86,7 @@ export async function loadConfigSchema( return { process( configs: AppConfig[], - { visibility, valueTransform, withFilteredKeys } = {}, + { visibility, valueTransform, withFilteredKeys, withDeprecatedKeys } = {}, ): AppConfig[] { const result = validate(configs); @@ -108,6 +112,11 @@ export async function loadConfigSchema( valueTransform, withFilteredKeys, ), + ...filterByDeprecated( + data, + result.deprecationBySchemaPath, + withDeprecatedKeys, + ), })); } else if (valueTransform) { processedConfigs = processedConfigs.map(({ data, context }) => ({ @@ -119,6 +128,11 @@ export async function loadConfigSchema( valueTransform, withFilteredKeys, ), + ...filterByDeprecated( + data, + result.deprecationBySchemaPath, + withDeprecatedKeys, + ), })); } diff --git a/packages/config-loader/src/lib/schema/types.ts b/packages/config-loader/src/lib/schema/types.ts index 53f4dec88e..76cc337f83 100644 --- a/packages/config-loader/src/lib/schema/types.ts +++ b/packages/config-loader/src/lib/schema/types.ts @@ -81,6 +81,13 @@ type ValidationResult = { * The path in the key uses the form `/properties//items/additionalProperties/` */ visibilityBySchemaPath: Map; + + /** + * The deprecated options that were discovered during validation. + * + * The path in the key uses the form `/properties//items/additionalProperties/` + */ + deprecationBySchemaPath: Map; }; /** @@ -124,6 +131,13 @@ export type ConfigSchemaProcessingOptions = { * Default: `false`. */ withFilteredKeys?: boolean; + + /** + * Whether or not to include the `deprecatedKeys` property in the output `AppConfig`s. + * + * Default: `true`. + */ + withDeprecatedKeys?: boolean; }; /** diff --git a/packages/config/api-report.md b/packages/config/api-report.md index efa6b85559..df9baeb4cd 100644 --- a/packages/config/api-report.md +++ b/packages/config/api-report.md @@ -13,6 +13,10 @@ export type AppConfig = { context: string; data: JsonObject_2; filteredKeys?: string[]; + deprecatedKeys?: { + key: string; + description: string; + }[]; }; // @public diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index 8c0c4140a5..5c34b215ab 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -83,9 +83,27 @@ export class ConfigReader implements Config { // Merge together all configs into a single config with recursive fallback // readers, giving the first config object in the array the lowest priority. return configs.reduce( - (previousReader, { data, context, filteredKeys }) => { + (previousReader, { data, context, filteredKeys, deprecatedKeys }) => { const reader = new ConfigReader(data, context, previousReader); reader.filteredKeys = filteredKeys; + + // TODO(cmpadden) introduce `deprecatedKeys` and `notifiedDeprecatedKeys` & use logger + if (deprecatedKeys) { + for (const { key, description } of deprecatedKeys) { + if (description) { + // eslint-disable-next-line no-console + console.warn( + `The configuration key '${key}' is deprecated and may be removed soon. (reason: ${description})`, + ); + } else { + // eslint-disable-next-line no-console + console.warn( + `The configuration key '${key}' is deprecated and may be removed soon.`, + ); + } + } + } + return reader; }, undefined!, diff --git a/packages/config/src/types.ts b/packages/config/src/types.ts index a543233277..37c9595565 100644 --- a/packages/config/src/types.ts +++ b/packages/config/src/types.ts @@ -36,6 +36,12 @@ export type AppConfig = { * This can be used to warn the user if they try to read any of these keys. */ filteredKeys?: string[]; + /** + * A list of deprecated keys that were found in the configuration when it was loaded. + * + * This can be used to warn the user if they are using deprecated properties. + */ + deprecatedKeys?: { key: string; description: string }[]; }; /** From 63fa8f83715b9a998d6ee6deb8af170cc23f8089 Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Tue, 4 Jan 2022 16:24:06 -0500 Subject: [PATCH 2/6] move deprecation description key mapping into filterByVisibility Signed-off-by: Colton Padden --- .../config-loader/src/lib/schema/collect.ts | 2 +- .../src/lib/schema/compile.test.ts | 12 ++-- .../config-loader/src/lib/schema/compile.ts | 70 ++++++++++++------- .../src/lib/schema/filtering.test.ts | 51 ++++++++------ .../config-loader/src/lib/schema/filtering.ts | 46 ++++-------- .../config-loader/src/lib/schema/load.test.ts | 8 --- packages/config-loader/src/lib/schema/load.ts | 10 +-- .../config-loader/src/lib/schema/types.ts | 4 +- packages/config/src/reader.ts | 19 ++--- 9 files changed, 105 insertions(+), 117 deletions(-) diff --git a/packages/config-loader/src/lib/schema/collect.ts b/packages/config-loader/src/lib/schema/collect.ts index aa3202a1b5..a05796139b 100644 --- a/packages/config-loader/src/lib/schema/collect.ts +++ b/packages/config-loader/src/lib/schema/collect.ts @@ -182,7 +182,7 @@ function compileTsSchemas(paths: string[]) { program, // All schemas should export a `Config` symbol 'Config', - // This enables usage of @visibility and @deprecated is doc comments + // This enables usage of @visibility and @deprecated in doc comments { required: true, validationKeywords: ['visibility', 'deprecated'], diff --git a/packages/config-loader/src/lib/schema/compile.test.ts b/packages/config-loader/src/lib/schema/compile.test.ts index 7615fa80f5..7b7db87c8f 100644 --- a/packages/config-loader/src/lib/schema/compile.test.ts +++ b/packages/config-loader/src/lib/schema/compile.test.ts @@ -40,7 +40,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); expect(validate([{ data: { b: 'b' }, context: 'test' }])).toEqual({ errors: [ @@ -54,7 +54,7 @@ describe('compileConfigSchemas', () => { ], visibilityByDataPath: new Map(), visibilityBySchemaPath: new Map(), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); }); @@ -114,7 +114,7 @@ describe('compileConfigSchemas', () => { '/properties/d/items': 'frontend', }), ), - deprecationBySchemaPath: new Map(), + deprecationByDataPath: new Map(), }); }); @@ -160,10 +160,10 @@ describe('compileConfigSchemas', () => { { data: { a: 'a', b: 'b', c: 'c', d: ['d'] }, context: 'test' }, ]), ).toEqual({ - deprecationBySchemaPath: new Map( + deprecationByDataPath: new Map( Object.entries({ - '/properties/a': 'deprecation reason for a', - '/properties/b': 'deprecation reason for b', + '/a': 'deprecation reason for a', + '/b': 'deprecation reason for b', }), ), visibilityByDataPath: new Map(), diff --git a/packages/config-loader/src/lib/schema/compile.ts b/packages/config-loader/src/lib/schema/compile.ts index 6dcd4713f5..b7f705751e 100644 --- a/packages/config-loader/src/lib/schema/compile.ts +++ b/packages/config-loader/src/lib/schema/compile.ts @@ -40,6 +40,7 @@ export function compileConfigSchemas( // output during validation. We work around this by having this extra piece // of state that we reset before each validation. const visibilityByDataPath = new Map(); + const deprecationByDataPath = new Map(); const ajv = new Ajv({ allErrors: true, @@ -47,28 +48,48 @@ export function compileConfigSchemas( schemas: { 'https://backstage.io/schema/config-v1': true, }, - }).addKeyword({ - keyword: 'visibility', - metaSchema: { - type: 'string', - enum: CONFIG_VISIBILITIES, - }, - compile(visibility: ConfigVisibility) { - return (_data, context) => { - if (context?.dataPath === undefined) { - return false; - } - if (visibility && visibility !== 'backend') { + }) + .addKeyword({ + keyword: 'visibility', + metaSchema: { + type: 'string', + enum: CONFIG_VISIBILITIES, + }, + compile(visibility: ConfigVisibility) { + return (_data, context) => { + if (context?.dataPath === undefined) { + return false; + } + if (visibility && visibility !== 'backend') { + const normalizedPath = context.dataPath.replace( + /\['?(.*?)'?\]/g, + (_, segment) => `/${segment}`, + ); + visibilityByDataPath.set(normalizedPath, visibility); + } + return true; + }; + }, + }) + .removeKeyword('deprecated') // remove `deprecated` keyword so that we can implement our own compiler + .addKeyword({ + keyword: 'deprecated', + metaSchema: { type: 'string' }, + compile(deprecationDescription: string) { + return (_data, context) => { + if (context?.dataPath === undefined) { + return false; + } const normalizedPath = context.dataPath.replace( /\['?(.*?)'?\]/g, (_, segment) => `/${segment}`, ); - visibilityByDataPath.set(normalizedPath, visibility); - } - return true; - }; - }, - }); + // create mapping of deprecation description and data path of property + deprecationByDataPath.set(normalizedPath, deprecationDescription); + return true; + }; + }, + }); for (const schema of schemas) { try { @@ -79,14 +100,8 @@ export function compileConfigSchemas( } const merged = mergeConfigSchemas(schemas.map(_ => _.value)); - const validate = ajv.compile(merged); - const deprecationBySchemaPath = new Map(); - traverse(merged, (schema, path) => { - if ('deprecated' in schema) { - deprecationBySchemaPath.set(path, schema.deprecated); - } - }); + const validate = ajv.compile(merged); const visibilityBySchemaPath = new Map(); traverse(merged, (schema, path) => { @@ -101,19 +116,20 @@ export function compileConfigSchemas( visibilityByDataPath.clear(); const valid = validate(config); + if (!valid) { return { errors: validate.errors ?? [], visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, - deprecationBySchemaPath, + deprecationByDataPath, }; } return { visibilityByDataPath: new Map(visibilityByDataPath), visibilityBySchemaPath, - deprecationBySchemaPath, + deprecationByDataPath, }; }; } diff --git a/packages/config-loader/src/lib/schema/filtering.test.ts b/packages/config-loader/src/lib/schema/filtering.test.ts index 2324bebc9d..df06975c9f 100644 --- a/packages/config-loader/src/lib/schema/filtering.test.ts +++ b/packages/config-loader/src/lib/schema/filtering.test.ts @@ -16,11 +16,7 @@ import { JsonObject } from '@backstage/types'; import { ConfigVisibility } from './types'; -import { - filterByVisibility, - filterErrorsByVisibility, - filterByDeprecated, -} from './filtering'; +import { filterByVisibility, filterErrorsByVisibility } from './filtering'; const data = { arr: ['f', 'b', 's'], @@ -70,8 +66,8 @@ const visibility = new Map( const deprecations = new Map( Object.entries({ - '/properties/arr': 'deprecated array', - '/properties/objB/properties/never': 'deprecated nested property', + '/arr': 'deprecated array', + '/objB/never': 'deprecated nested property', }), ); @@ -196,9 +192,34 @@ describe('filterByVisibility', () => { [['frontend', 'backend', 'secret'], { data, filteredKeys: [] }], ])('should filter correctly with %p', (filter, expected) => { expect( - filterByVisibility(data, filter, visibility, undefined, true), + filterByVisibility( + data, + filter, + visibility, + deprecations, + undefined, + true, + false, + ), ).toEqual(expected); }); + + it('should include deprecated keys regardless of visibility', () => { + expect( + filterByVisibility( + data, + [], + visibility, + deprecations, + undefined, + true, + true, + ).deprecatedKeys, + ).toEqual([ + { key: 'arr', description: 'deprecated array' }, + { key: 'objB.never', description: 'deprecated nested property' }, + ]); + }); }); describe('filterErrorsByVisibility', () => { @@ -392,17 +413,3 @@ describe('filterErrorsByVisibility', () => { ]); }); }); - -describe('filterByDeprecated', () => { - it('should allow empty list of deprecations', () => { - expect(filterByDeprecated(data, new Map())).toEqual({ deprecatedKeys: [] }); - }); - it('should return a list of deprecated properties', () => { - expect(filterByDeprecated(data, deprecations)).toEqual({ - deprecatedKeys: [ - { key: 'arr', description: 'deprecated array' }, - { key: 'objB.never', description: 'deprecated nested property' }, - ], - }); - }); -}); diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index f955343066..11ee4cb47f 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -23,36 +23,6 @@ import { ValidationError, } from './types'; -/** - * This filters data by deprecation status to only include those which have been deprecated - * - * @param data - configuration data - * @param deprecationBySchemaPath - mapping of schema path to deprecation description - * @returns deprecated configuration keys with their deprecation description - */ -export function filterByDeprecated( - data: JsonObject, - deprecationBySchemaPath: Map, - withDeprecatedKeys: boolean = true, -): { deprecatedKeys: { key: string; description: string }[] } { - const deprecatedKeys = new Array<{ key: string; description: string }>(); - if (!withDeprecatedKeys) { - return { deprecatedKeys }; - } - - deprecationBySchemaPath.forEach((desc, schemaPath) => { - // convert schema path to object path (/properties/techdocs/properties/storageUrl -> techdocs.storageUrl) - const propertyPath = schemaPath - .replace(/^\/properties\//, '') // remove leading `/properties/` entirely - .replace(/\/properties\//g, '.'); // replace all other `/properties/` with a `.` - if (has(data, propertyPath)) { - deprecatedKeys.push({ key: propertyPath, description: desc }); - } - }); - - return { deprecatedKeys }; -} - /** * This filters data by visibility by discovering the visibility of each * value, and then only keeping the ones that are specified in `includeVisibilities`. @@ -61,10 +31,17 @@ export function filterByVisibility( data: JsonObject, includeVisibilities: ConfigVisibility[], visibilityByDataPath: Map, + deprecationByDataPath: Map, transformFunc?: TransformFunc, withFilteredKeys?: boolean, -): { data: JsonObject; filteredKeys?: string[] } { + withDeprecatedKeys?: boolean, +): { + data: JsonObject; + filteredKeys?: string[]; + deprecatedKeys?: { key: string; description: string }[]; +} { const filteredKeys = new Array(); + const deprecatedKeys = new Array<{ key: string; description: string }>(); function transform( jsonVal: JsonValue, @@ -75,6 +52,12 @@ export function filterByVisibility( visibilityByDataPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY; const isVisible = includeVisibilities.includes(visibility); + // deprecated keys are added regardless of visibility indicator + const deprecation = deprecationByDataPath.get(visibilityPath); + if (deprecation) { + deprecatedKeys.push({ key: filterPath, description: deprecation }); + } + if (typeof jsonVal !== 'object') { if (isVisible) { if (transformFunc) { @@ -140,6 +123,7 @@ export function filterByVisibility( return { filteredKeys: withFilteredKeys ? filteredKeys : undefined, + deprecatedKeys: withDeprecatedKeys ? deprecatedKeys : undefined, data: (transform(data, '', '') as JsonObject) ?? {}, }; } diff --git a/packages/config-loader/src/lib/schema/load.test.ts b/packages/config-loader/src/lib/schema/load.test.ts index a2f1df8d5f..8510ccb1d8 100644 --- a/packages/config-loader/src/lib/schema/load.test.ts +++ b/packages/config-loader/src/lib/schema/load.test.ts @@ -65,7 +65,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect( @@ -79,7 +78,6 @@ describe('loadConfigSchema', () => { data: { key1: 'X' }, context: 'test', filteredKeys: ['key2'], - deprecatedKeys: [], }, ]); expect( @@ -92,7 +90,6 @@ describe('loadConfigSchema', () => { data: { key1: 'X', key2: 'X' }, context: 'test', filteredKeys: [], - deprecatedKeys: [], }, ]); @@ -103,7 +100,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => @@ -152,7 +148,6 @@ describe('loadConfigSchema', () => { { data: { key1: 'a' }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => schema.process(configs, { visibility: ['secret'] })).toThrow( @@ -205,7 +200,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{}] }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => schema.process(mkConfig({ y: 1 }))).toThrow( @@ -222,7 +216,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{}] }, context: 'test', - deprecatedKeys: [], }, ]); expect( @@ -231,7 +224,6 @@ describe('loadConfigSchema', () => { { data: { nested: [{ y: 'aaa' }] }, context: 'test', - deprecatedKeys: [], }, ]); expect(() => diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index 303fdfade9..9eac629ee3 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -109,12 +109,9 @@ export async function loadConfigSchema( data, visibility, result.visibilityByDataPath, + result.deprecationByDataPath, valueTransform, withFilteredKeys, - ), - ...filterByDeprecated( - data, - result.deprecationBySchemaPath, withDeprecatedKeys, ), })); @@ -125,12 +122,9 @@ export async function loadConfigSchema( data, Array.from(CONFIG_VISIBILITIES), result.visibilityByDataPath, + result.deprecationByDataPath, valueTransform, withFilteredKeys, - ), - ...filterByDeprecated( - data, - result.deprecationBySchemaPath, withDeprecatedKeys, ), })); diff --git a/packages/config-loader/src/lib/schema/types.ts b/packages/config-loader/src/lib/schema/types.ts index 76cc337f83..75993b3c78 100644 --- a/packages/config-loader/src/lib/schema/types.ts +++ b/packages/config-loader/src/lib/schema/types.ts @@ -85,9 +85,9 @@ type ValidationResult = { /** * The deprecated options that were discovered during validation. * - * The path in the key uses the form `/properties//items/additionalProperties/` + * The path in the key uses the form `////` */ - deprecationBySchemaPath: Map; + deprecationByDataPath: Map; }; /** diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index 5c34b215ab..d85ea6f482 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -87,20 +87,15 @@ export class ConfigReader implements Config { const reader = new ConfigReader(data, context, previousReader); reader.filteredKeys = filteredKeys; - // TODO(cmpadden) introduce `deprecatedKeys` and `notifiedDeprecatedKeys` & use logger + // TODO(cmpadden) `withDeprecatedKeys` is defaulting to false on app start if (deprecatedKeys) { for (const { key, description } of deprecatedKeys) { - if (description) { - // eslint-disable-next-line no-console - console.warn( - `The configuration key '${key}' is deprecated and may be removed soon. (reason: ${description})`, - ); - } else { - // eslint-disable-next-line no-console - console.warn( - `The configuration key '${key}' is deprecated and may be removed soon.`, - ); - } + // eslint-disable-next-line no-console + console.warn( + `The configuration key '${key}' of ${context} is deprecated and may be removed soon. ${ + description || '' + }`, + ); } } From 6e34e2cfbfbd83aa61ec184a78374e1c4220ca96 Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Tue, 4 Jan 2022 16:29:28 -0500 Subject: [PATCH 3/6] Add `backstage-cli config:check --deprecated` to list deprecated settings Signed-off-by: Colton Padden --- .changeset/long-clouds-rest.md | 12 ++++++++++++ docs/local-dev/cli-commands.md | 1 + packages/cli/src/commands/config/validate.ts | 1 + packages/cli/src/commands/index.ts | 1 + packages/cli/src/lib/config.ts | 2 ++ 5 files changed, 17 insertions(+) create mode 100644 .changeset/long-clouds-rest.md diff --git a/.changeset/long-clouds-rest.md b/.changeset/long-clouds-rest.md new file mode 100644 index 0000000000..701be91536 --- /dev/null +++ b/.changeset/long-clouds-rest.md @@ -0,0 +1,12 @@ +--- +'@backstage/cli': patch +--- + +Introduce `--deprecated` option to `config:check` to log all deprecated app configuration properties + +```sh +$ yarn backstage-cli config:check --lax --deprecated +config:check --lax --deprecated +Loaded config from app-config.yaml +The configuration key 'catalog.processors.githubOrg' of app-config.yaml is deprecated and may be removed soon. Configure a GitHub integration instead. +``` diff --git a/docs/local-dev/cli-commands.md b/docs/local-dev/cli-commands.md index 412e3c4598..c65fc30da6 100644 --- a/docs/local-dev/cli-commands.md +++ b/docs/local-dev/cli-commands.md @@ -556,6 +556,7 @@ Options: --package <name> Only load config schema that applies to the given package --lax Do not require environment variables to be set --frontend Only validate the frontend configuration + --deprecated List all deprecated configuration settings --config <path> Config files to load instead of app-config.yaml (default: []) -h, --help display help for command ``` diff --git a/packages/cli/src/commands/config/validate.ts b/packages/cli/src/commands/config/validate.ts index 2d3ce60366..8d528b8300 100644 --- a/packages/cli/src/commands/config/validate.ts +++ b/packages/cli/src/commands/config/validate.ts @@ -23,5 +23,6 @@ export default async (cmd: Command) => { fromPackage: cmd.package, mockEnv: cmd.lax, fullVisibility: !cmd.frontend, + withDeprecatedKeys: cmd.deprecated, }); }; diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index bef5dfc285..49e700ac1e 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -193,6 +193,7 @@ export function registerCommands(program: CommanderStatic) { ) .option('--lax', 'Do not require environment variables to be set') .option('--frontend', 'Only validate the frontend configuration') + .option('--deprecated', 'Output deprecated configuration settings') .option(...configOption) .description( 'Validate that the given configuration loads and matches schema', diff --git a/packages/cli/src/lib/config.ts b/packages/cli/src/lib/config.ts index 986e409e58..c24021fd0b 100644 --- a/packages/cli/src/lib/config.ts +++ b/packages/cli/src/lib/config.ts @@ -28,6 +28,7 @@ type Options = { fromPackage?: string; mockEnv?: boolean; withFilteredKeys?: boolean; + withDeprecatedKeys?: boolean; fullVisibility?: boolean; }; @@ -82,6 +83,7 @@ export async function loadCliConfig(options: Options) { ? ['frontend', 'backend', 'secret'] : ['frontend'], withFilteredKeys: options.withFilteredKeys, + withDeprecatedKeys: options.withDeprecatedKeys, }); const frontendConfig = ConfigReader.fromConfigs(frontendAppConfigs); From f9c5841c801a1a543c61d8acc81befbb7aa3ae1c Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Tue, 4 Jan 2022 19:33:42 -0500 Subject: [PATCH 4/6] remove deleted artifacts of filterByDeprecated Signed-off-by: Colton Padden --- packages/config-loader/package.json | 1 - packages/config-loader/src/lib/schema/filtering.ts | 1 - packages/config-loader/src/lib/schema/load.ts | 6 +----- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/config-loader/package.json b/packages/config-loader/package.json index 2ecea8e843..d909cde58c 100644 --- a/packages/config-loader/package.json +++ b/packages/config-loader/package.json @@ -41,7 +41,6 @@ "json-schema": "^0.4.0", "json-schema-merge-allof": "^0.8.1", "json-schema-traverse": "^1.0.0", - "lodash": "^4.17.21", "node-fetch": "^2.6.1", "typescript-json-schema": "^0.52.0", "yaml": "^1.9.2", diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index 11ee4cb47f..2855979b1f 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -15,7 +15,6 @@ */ import { JsonObject, JsonValue } from '@backstage/types'; -import { has } from 'lodash'; import { ConfigVisibility, DEFAULT_CONFIG_VISIBILITY, diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index 9eac629ee3..aa7ed06305 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -18,11 +18,7 @@ import { AppConfig } from '@backstage/config'; import { JsonObject } from '@backstage/types'; import { compileConfigSchemas } from './compile'; import { collectConfigSchemas } from './collect'; -import { - filterByVisibility, - filterErrorsByVisibility, - filterByDeprecated, -} from './filtering'; +import { filterByVisibility, filterErrorsByVisibility } from './filtering'; import { ValidationError, ConfigSchema, From a505347140e3091915c2a7dfbf4dcb202c835734 Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Wed, 5 Jan 2022 14:17:11 -0500 Subject: [PATCH 5/6] enable withDeprecatedKeys in schema process for backend-common and app-backend Signed-off-by: Colton Padden --- packages/backend-common/src/config.ts | 5 ++++- packages/config/src/reader.ts | 1 - plugins/app-backend/src/lib/config.ts | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/backend-common/src/config.ts b/packages/backend-common/src/config.ts index 399898cfc4..8e0c05b86e 100644 --- a/packages/backend-common/src/config.ts +++ b/packages/backend-common/src/config.ts @@ -38,7 +38,10 @@ const updateRedactionList = ( configs: AppConfig[], logger: Logger, ) => { - const secretAppConfigs = schema.process(configs, { visibility: ['secret'] }); + const secretAppConfigs = schema.process(configs, { + visibility: ['secret'], + withDeprecatedKeys: true, + }); const secretConfig = ConfigReader.fromConfigs(secretAppConfigs); const values = new Set(); const data = secretConfig.get(); diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index d85ea6f482..b4bbf0722e 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -87,7 +87,6 @@ export class ConfigReader implements Config { const reader = new ConfigReader(data, context, previousReader); reader.filteredKeys = filteredKeys; - // TODO(cmpadden) `withDeprecatedKeys` is defaulting to false on app start if (deprecatedKeys) { for (const { key, description } of deprecatedKeys) { // eslint-disable-next-line no-console diff --git a/plugins/app-backend/src/lib/config.ts b/plugins/app-backend/src/lib/config.ts index c37dc34dcc..6959253336 100644 --- a/plugins/app-backend/src/lib/config.ts +++ b/plugins/app-backend/src/lib/config.ts @@ -91,7 +91,7 @@ export async function readConfigs(options: ReadOptions): Promise { const frontendConfigs = await schema.process( [{ data: config.get() as JsonObject, context: 'app' }], - { visibility: ['frontend'] }, + { visibility: ['frontend'], withDeprecatedKeys: true }, ); appConfigs.push(...frontendConfigs); } catch (error) { From f685e1398f857b129f5ff3402883ba31e12f3be1 Mon Sep 17 00:00:00 2001 From: Colton Padden Date: Wed, 5 Jan 2022 14:46:39 -0500 Subject: [PATCH 6/6] add changeset for deprecation warning feature Signed-off-by: Colton Padden --- .changeset/light-trainers-allow.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .changeset/light-trainers-allow.md diff --git a/.changeset/light-trainers-allow.md b/.changeset/light-trainers-allow.md new file mode 100644 index 0000000000..0ea13cd19e --- /dev/null +++ b/.changeset/light-trainers-allow.md @@ -0,0 +1,22 @@ +--- +'@backstage/backend-common': patch +'@backstage/config': patch +'@backstage/config-loader': patch +'@backstage/plugin-app-backend': patch +--- + +Loading of app configurations now reference the `@deprecated` construct from +JSDoc to determine if a property in-use has been deprecated. Users are notified +of deprecated keys in the format: + +```txt +The configuration key 'catalog.processors.githubOrg' of app-config.yaml is deprecated and may be removed soon. Configure a GitHub integration instead. +``` + +When the `withDeprecatedKeys` option is set to `true` in the `process` method +of `loadConfigSchema`, the user will be notified that deprecated keys have been +identified in their app configuration. + +The `backend-common` and `plugin-app-backend` packages have been updated to set +`withDeprecatedKeys` to true so that users are notified of deprecated settings +by default.