Remove cli options parsing from @backstage/config-loader. Update fixtures.

Signed-off-by: Aramis Sennyey <sennyeya@amazon.com>
2022-11-29 12:56:54 -05:00
parent 09d930c2eb
commit 1a845e86a9
19 changed files with 89 additions and 134 deletions
@@ -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'),
-}));
@@ -1,6 +0,0 @@
-app:
-  baseUrl: http://localhost:3000/test
-  title: test
-
-backend:
-  baseUrl: http://localhost:7007
@@ -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",
@@ -0,0 +1,6 @@
+app:
+  baseUrl: http://localhost:${PORT}/test
+  title: test
+
+backend:
+  baseUrl: http://localhost:${BACKEND_PORT}
@@ -17,6 +17,5 @@
 import '@backstage/cli/asset-types';
 import React from 'react';
 import ReactDOM from 'react-dom';
-import App from './App';

 ReactDOM.render(<div />, document.getElementById('root'));
@@ -13,3 +13,4 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
+export {};
@@ -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;
@@ -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', () => {
@@ -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(
@@ -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,
      {
@@ -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' }];
-}
@@ -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],
  };
 }
@@ -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