From 1a845e86a95d62edc5a8be68a902bcc66e8fb82a Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Tue, 29 Nov 2022 12:56:54 -0500 Subject: [PATCH] Remove cli options parsing from @backstage/config-loader. Update fixtures. Signed-off-by: Aramis Sennyey --- packages/cli/e2e-test.config.js | 22 ------ .../e2e-tests/test-project/app-config.yaml | 6 -- packages/cli/package.json | 2 +- .../__fixtures__/test-project/app-config.yaml | 6 ++ .../__fixtures__}/test-project/backstage.json | 0 .../__fixtures__}/test-project/package.json | 0 .../test-project/packages/app/package.json | 0 .../packages/app/public/index.html | 0 .../test-project/packages/app/src/index.tsx | 1 - .../packages/backend/package.json | 0 .../packages/backend/src/index.ts | 1 + .../__fixtures__}/test-project/yarn.lock | 0 .../cli/src/commands/build/buildFrontend.ts | 3 +- .../src/lib/config.test.ts} | 2 +- packages/cli/src/lib/config.ts | 47 +++++++++++-- packages/cli/{e2e-tests => src}/serve.test.ts | 68 ++++++++++--------- packages/config-loader/src/lib/cli.ts | 51 -------------- packages/config-loader/src/loader.ts | 12 +--- yarn.lock | 2 +- 19 files changed, 89 insertions(+), 134 deletions(-) delete mode 100644 packages/cli/e2e-test.config.js delete mode 100644 packages/cli/e2e-tests/test-project/app-config.yaml create mode 100644 packages/cli/src/__fixtures__/test-project/app-config.yaml rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/backstage.json (100%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/package.json (100%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/packages/app/package.json (100%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/packages/app/public/index.html (100%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/packages/app/src/index.tsx (96%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/packages/backend/package.json (100%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/packages/backend/src/index.ts (98%) rename packages/cli/{e2e-tests => src/__fixtures__}/test-project/yarn.lock (100%) rename packages/{config-loader/src/lib/cli.test.ts => cli/src/lib/config.test.ts} (98%) rename packages/cli/{e2e-tests => src}/serve.test.ts (77%) delete mode 100644 packages/config-loader/src/lib/cli.ts diff --git a/packages/cli/e2e-test.config.js b/packages/cli/e2e-test.config.js deleted file mode 100644 index 1c1cf7a75b..0000000000 --- a/packages/cli/e2e-test.config.js +++ /dev/null @@ -1,22 +0,0 @@ -/* - * Copyright 2022 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. - */ - -const path = require('path'); - -module.exports = require('./config/jest').then(baseConfig => ({ - ...baseConfig, - rootDir: path.resolve(__dirname, 'e2e-tests'), -})); diff --git a/packages/cli/e2e-tests/test-project/app-config.yaml b/packages/cli/e2e-tests/test-project/app-config.yaml deleted file mode 100644 index 477580da86..0000000000 --- a/packages/cli/e2e-tests/test-project/app-config.yaml +++ /dev/null @@ -1,6 +0,0 @@ -app: - baseUrl: http://localhost:3000/test - title: test - -backend: - baseUrl: http://localhost:7007 \ No newline at end of file diff --git a/packages/cli/package.json b/packages/cli/package.json index d605c6f31f..0fcd8cd89f 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -86,6 +86,7 @@ "express": "^4.17.1", "fork-ts-checker-webpack-plugin": "^7.0.0-alpha.8", "fs-extra": "10.1.0", + "get-port": "^6.1.2", "glob": "^7.1.7", "global-agent": "^3.0.0", "handlebars": "^4.7.3", @@ -155,7 +156,6 @@ "@types/tar": "^6.1.1", "@types/terser-webpack-plugin": "^5.0.4", "@types/yarnpkg__lockfile": "^1.1.4", - "cross-fetch": "^3.1.5", "del": "^6.0.0", "mock-fs": "^5.1.0", "msw": "^0.49.0", diff --git a/packages/cli/src/__fixtures__/test-project/app-config.yaml b/packages/cli/src/__fixtures__/test-project/app-config.yaml new file mode 100644 index 0000000000..c4d4705aa5 --- /dev/null +++ b/packages/cli/src/__fixtures__/test-project/app-config.yaml @@ -0,0 +1,6 @@ +app: + baseUrl: http://localhost:${PORT}/test + title: test + +backend: + baseUrl: http://localhost:${BACKEND_PORT} \ No newline at end of file diff --git a/packages/cli/e2e-tests/test-project/backstage.json b/packages/cli/src/__fixtures__/test-project/backstage.json similarity index 100% rename from packages/cli/e2e-tests/test-project/backstage.json rename to packages/cli/src/__fixtures__/test-project/backstage.json diff --git a/packages/cli/e2e-tests/test-project/package.json b/packages/cli/src/__fixtures__/test-project/package.json similarity index 100% rename from packages/cli/e2e-tests/test-project/package.json rename to packages/cli/src/__fixtures__/test-project/package.json diff --git a/packages/cli/e2e-tests/test-project/packages/app/package.json b/packages/cli/src/__fixtures__/test-project/packages/app/package.json similarity index 100% rename from packages/cli/e2e-tests/test-project/packages/app/package.json rename to packages/cli/src/__fixtures__/test-project/packages/app/package.json diff --git a/packages/cli/e2e-tests/test-project/packages/app/public/index.html b/packages/cli/src/__fixtures__/test-project/packages/app/public/index.html similarity index 100% rename from packages/cli/e2e-tests/test-project/packages/app/public/index.html rename to packages/cli/src/__fixtures__/test-project/packages/app/public/index.html diff --git a/packages/cli/e2e-tests/test-project/packages/app/src/index.tsx b/packages/cli/src/__fixtures__/test-project/packages/app/src/index.tsx similarity index 96% rename from packages/cli/e2e-tests/test-project/packages/app/src/index.tsx rename to packages/cli/src/__fixtures__/test-project/packages/app/src/index.tsx index 145adc1f48..3c96b4476d 100644 --- a/packages/cli/e2e-tests/test-project/packages/app/src/index.tsx +++ b/packages/cli/src/__fixtures__/test-project/packages/app/src/index.tsx @@ -17,6 +17,5 @@ import '@backstage/cli/asset-types'; import React from 'react'; import ReactDOM from 'react-dom'; -import App from './App'; ReactDOM.render(
, document.getElementById('root')); diff --git a/packages/cli/e2e-tests/test-project/packages/backend/package.json b/packages/cli/src/__fixtures__/test-project/packages/backend/package.json similarity index 100% rename from packages/cli/e2e-tests/test-project/packages/backend/package.json rename to packages/cli/src/__fixtures__/test-project/packages/backend/package.json diff --git a/packages/cli/e2e-tests/test-project/packages/backend/src/index.ts b/packages/cli/src/__fixtures__/test-project/packages/backend/src/index.ts similarity index 98% rename from packages/cli/e2e-tests/test-project/packages/backend/src/index.ts rename to packages/cli/src/__fixtures__/test-project/packages/backend/src/index.ts index b61d59e88d..8b9b6bd586 100644 --- a/packages/cli/e2e-tests/test-project/packages/backend/src/index.ts +++ b/packages/cli/src/__fixtures__/test-project/packages/backend/src/index.ts @@ -13,3 +13,4 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +export {}; diff --git a/packages/cli/e2e-tests/test-project/yarn.lock b/packages/cli/src/__fixtures__/test-project/yarn.lock similarity index 100% rename from packages/cli/e2e-tests/test-project/yarn.lock rename to packages/cli/src/__fixtures__/test-project/yarn.lock diff --git a/packages/cli/src/commands/build/buildFrontend.ts b/packages/cli/src/commands/build/buildFrontend.ts index 7f1687ca03..92e42a8d64 100644 --- a/packages/cli/src/commands/build/buildFrontend.ts +++ b/packages/cli/src/commands/build/buildFrontend.ts @@ -18,8 +18,7 @@ import fs from 'fs-extra'; import { resolve as resolvePath } from 'path'; import { buildBundle } from '../../lib/bundler'; import { getEnvironmentParallelism } from '../../lib/parallel'; -import { loadCliConfig } from '../../lib/config'; -import { CliConfigOptions } from '@backstage/config-loader/src/lib/cli'; +import { loadCliConfig, CliConfigOptions } from '../../lib/config'; interface BuildAppOptions { targetDir: string; diff --git a/packages/config-loader/src/lib/cli.test.ts b/packages/cli/src/lib/config.test.ts similarity index 98% rename from packages/config-loader/src/lib/cli.test.ts rename to packages/cli/src/lib/config.test.ts index 4f4e76142a..bc6d8b6931 100644 --- a/packages/config-loader/src/lib/cli.test.ts +++ b/packages/cli/src/lib/config.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { readCliConfig } from './cli'; +import { readCliConfig } from './config'; describe('readCliConfig', () => { it('should return empty config for empty cli', () => { diff --git a/packages/cli/src/lib/config.ts b/packages/cli/src/lib/config.ts index e28f2d7f6f..1220586ce0 100644 --- a/packages/cli/src/lib/config.ts +++ b/packages/cli/src/lib/config.ts @@ -19,11 +19,17 @@ import { loadConfig, loadConfigSchema, } from '@backstage/config-loader'; -import { ConfigReader } from '@backstage/config'; +import { AppConfig, ConfigReader } from '@backstage/config'; import { paths } from './paths'; import { isValidUrl } from './urls'; import { getPackages } from '@manypkg/get-packages'; import { PackageGraph } from './monorepo'; +import { JsonObject } from '@backstage/types'; + +export type CliConfigOptions = { + publicPath?: string; + backendUrl?: string; +}; type Options = { args: string[]; @@ -32,12 +38,37 @@ type Options = { withFilteredKeys?: boolean; withDeprecatedKeys?: boolean; fullVisibility?: boolean; - cliOptions?: { - publicPath?: string; - backendUrl?: string; - }; + cliOptions?: CliConfigOptions; }; +/** + * Read specific parameters from the CLI and add them to the build config. + * @param opts CLI passed parameters. + * @returns Array of config, empty if there is no relevant passed in cli options. + * + * @public + */ +export function readCliConfig(opts?: CliConfigOptions): AppConfig[] { + if (!opts || Object.keys(opts).length === 0) return []; + const data: JsonObject = {}; + + if (opts.publicPath) { + data.app = { + baseUrl: opts.publicPath, + }; + } + + if (opts.backendUrl) { + data.backend = { + baseUrl: opts.backendUrl, + }; + } + + if (Object.keys(data).length === 0) return []; + + return [{ data, context: 'cli' }]; +} + export async function loadCliConfig(options: Options) { const configTargets: ConfigTarget[] = []; options.args.forEach(arg => { @@ -76,15 +107,19 @@ export async function loadCliConfig(options: Options) { packagePaths: [paths.resolveTargetRoot('package.json')], }); + const cliConfigs = readCliConfig(options.cliOptions); + const { appConfigs } = await loadConfig({ experimentalEnvFunc: options.mockEnv ? async name => process.env[name] || 'x' : undefined, - cliOptions: options.cliOptions, configRoot: paths.targetRoot, configTargets: configTargets, }); + // Add the cliConfigs to the end of the appConfigs array for final overriding. + appConfigs.push(...cliConfigs); + // printing to stderr to not clobber stdout in case the cli command // outputs structured data (e.g. as config:schema does) process.stderr.write( diff --git a/packages/cli/e2e-tests/serve.test.ts b/packages/cli/src/serve.test.ts similarity index 77% rename from packages/cli/e2e-tests/serve.test.ts rename to packages/cli/src/serve.test.ts index b18ff64251..f43ea8053d 100644 --- a/packages/cli/e2e-tests/serve.test.ts +++ b/packages/cli/src/serve.test.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import { execSync, spawn, SpawnOptionsWithoutStdio } from 'child_process'; +import { spawn, SpawnOptionsWithoutStdio } from 'child_process'; import EventEmitter from 'events'; -import mock from 'mock-fs'; import fetch from 'node-fetch'; import path from 'path'; +import getPort from 'get-port'; const executeCommand = ( command: string, @@ -48,6 +48,11 @@ const executeCommand = ( proc.stderr?.on('data', data => { stderr.push(Buffer.from(data)); }); + + /** + * Set an interval to check if we should kill the process. + * This was the easiest way I could think of of testing across two processes. + */ let intervalId: NodeJS.Timer | undefined = undefined; if (eventConfig) { intervalId = setInterval(() => { @@ -62,8 +67,7 @@ const executeCommand = ( }, 1000); } - events?.on('stop', signal => { - console.log(signal); + const clearEventInterval = () => { if (intervalId) { try { clearInterval(intervalId); @@ -71,27 +75,22 @@ const executeCommand = ( console.error(err); } } + }; + + /** + * Need a way to kill the process from another process. + */ + events?.on('stop', signal => { + clearEventInterval(); proc.kill(signal); }); proc.on('error', (...errorArgs) => { - if (intervalId) { - try { - clearInterval(intervalId); - } catch (err) { - console.error(err); - } - } + clearEventInterval(); reject(errorArgs); }); proc.on('exit', code => { - if (intervalId) { - try { - clearInterval(intervalId); - } catch (err) { - console.error(err); - } - } + clearEventInterval(); resolve({ exit: code ?? 0, stdout: Buffer.concat(stdout).toString('utf8'), @@ -101,43 +100,41 @@ const executeCommand = ( }); }; -const timeout = 40000; +const timeout = 100000; +// Builds initially (with no cache) take a loooong time. jest.setTimeout(timeout * 2); +const testProjectDir = path.resolve( + __dirname, + '__fixtures__/test-project/packages/app', +); + describe('end-to-end', () => { const entryPoint = path.resolve(__dirname, '../bin/backstage-cli'); - it.skip('shows help text', async () => { - const proc = await executeCommand(entryPoint, ['--help']); - expect(proc.stdout).toContain('Usage: backstage-cli [options]'); - expect(proc.exit).toEqual(0); - }); - it('builds frontend with correct url overrides', async () => { - const cwd = path.resolve(__dirname, 'test-project/packages/app'); const buildProc = await executeCommand( entryPoint, ['package', 'build', '--public-path', '/test', '--backend-url', '/api'], { - cwd, + cwd: testProjectDir, }, ); expect(buildProc.stderr).toContain( 'Loaded config from app-config.yaml, cli', ); - console.log(buildProc.stderr, buildProc.stdout); - expect(buildProc.exit).toEqual(0); }); it('starts frontend on correct url', async () => { - const cwd = path.resolve(__dirname, 'test-project/packages/app'); - const startEmitter = new EventEmitter(); + const frontendPort = await getPort(); startEmitter.on('hit', async () => { - const response = await fetch('http://localhost:3000/test/catalog'); + const response = await fetch( + `http://localhost:3000/${frontendPort}/catalog`, + ); const text = await response.text(); startEmitter.emit('stop', 'SIGINT'); expect(response.status).toBe(200); @@ -147,7 +144,12 @@ describe('end-to-end', () => { entryPoint, ['package', 'start'], { - cwd, + cwd: testProjectDir, + env: { + ...process.env, + PORT: `${frontendPort}`, + BACKEND_PORT: `${await getPort()}`, + }, }, startEmitter, { diff --git a/packages/config-loader/src/lib/cli.ts b/packages/config-loader/src/lib/cli.ts deleted file mode 100644 index 5e6eacf293..0000000000 --- a/packages/config-loader/src/lib/cli.ts +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright 2022 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 { AppConfig } from '@backstage/config'; -import { JsonObject } from '@backstage/types'; - -export type CliConfigOptions = { - publicPath?: string; - backendUrl?: string; -}; - -/** - * Read specific parameters from the CLI and add them to the build config. - * @param opts CLI passed parameters. - * @returns Array of config, empty if there is no relevant passed in cli options. - * - * @public - */ -export function readCliConfig(opts?: CliConfigOptions): AppConfig[] { - if (!opts || Object.keys(opts).length === 0) return []; - const data: JsonObject = {}; - - if (opts.publicPath) { - data.app = { - baseUrl: opts.publicPath, - }; - } - - if (opts.backendUrl) { - data.backend = { - baseUrl: opts.backendUrl, - }; - } - - if (Object.keys(data).length === 0) return []; - - return [{ data, context: 'cli' }]; -} diff --git a/packages/config-loader/src/loader.ts b/packages/config-loader/src/loader.ts index 44bd4f94e8..f5ce0eb80c 100644 --- a/packages/config-loader/src/loader.ts +++ b/packages/config-loader/src/loader.ts @@ -28,7 +28,6 @@ import { readEnvConfig, } from './lib'; import fetch from 'node-fetch'; -import { CliConfigOptions, readCliConfig } from './lib/cli'; /** @public */ export type ConfigTarget = { path: string } | { url: string }; @@ -82,11 +81,6 @@ export type LoadConfigOptions = { * An optional configuration that enables watching of config files. */ watch?: LoadConfigOptionsWatch; - - /** - * New options from the CLI that affect the build config. - */ - cliOptions?: CliConfigOptions; }; /** @@ -236,8 +230,6 @@ export async function loadConfig( } } - const cliConfigs = readCliConfig(options.cliOptions); - const envConfigs = readEnvConfig(process.env); const watchConfigFile = (watchProp: LoadConfigOptionsWatch) => { @@ -326,7 +318,7 @@ export async function loadConfig( return { appConfigs: remote - ? [...remoteConfigs, ...fileConfigs, ...envConfigs, ...cliConfigs] - : [...fileConfigs, ...envConfigs, ...cliConfigs], + ? [...remoteConfigs, ...fileConfigs, ...envConfigs] + : [...fileConfigs, ...envConfigs], }; } diff --git a/yarn.lock b/yarn.lock index 7fa022e4c6..634623d6b4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3653,7 +3653,6 @@ __metadata: chalk: ^4.0.0 chokidar: ^3.3.1 commander: ^9.1.0 - cross-fetch: ^3.1.5 css-loader: ^6.5.1 del: ^6.0.0 diff: ^5.0.0 @@ -3673,6 +3672,7 @@ __metadata: express: ^4.17.1 fork-ts-checker-webpack-plugin: ^7.0.0-alpha.8 fs-extra: 10.1.0 + get-port: ^6.1.2 glob: ^7.1.7 global-agent: ^3.0.0 handlebars: ^4.7.3