From 2c5ecf52bebba75f13617b0c43f2739ec6d1665d Mon Sep 17 00:00:00 2001 From: blam Date: Thu, 3 Oct 2024 16:58:08 +0200 Subject: [PATCH] feat: support --max-warnings in the cli Signed-off-by: blam --- .changeset/rotten-camels-deny.md | 5 ++ packages/cli/cli-report.md | 1 + packages/cli/src/commands/index.ts | 4 ++ packages/cli/src/commands/lint.ts | 13 +++- packages/cli/src/commands/repo/build.ts | 52 +------------- packages/cli/src/commands/repo/lint.ts | 46 ++++++++---- .../cli/src/commands/repo/optionsParser.ts | 70 +++++++++++++++++++ 7 files changed, 126 insertions(+), 65 deletions(-) create mode 100644 .changeset/rotten-camels-deny.md create mode 100644 packages/cli/src/commands/repo/optionsParser.ts diff --git a/.changeset/rotten-camels-deny.md b/.changeset/rotten-camels-deny.md new file mode 100644 index 0000000000..f5660d953a --- /dev/null +++ b/.changeset/rotten-camels-deny.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +Support `--max-warnings` flag for package linting diff --git a/packages/cli/cli-report.md b/packages/cli/cli-report.md index 2eb9bb8dea..70dc2614cc 100644 --- a/packages/cli/cli-report.md +++ b/packages/cli/cli-report.md @@ -237,6 +237,7 @@ Usage: backstage-cli package lint [options] [directories...] Options: --format --fix + --max-warnings -h, --help ``` diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index 7cc23300af..b54536cd55 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -156,6 +156,10 @@ export function registerScriptCommand(program: Command) { 'eslint-formatter-friendly', ) .option('--fix', 'Attempt to automatically fix violations') + .option( + '--max-warnings ', + 'Fail if more than this number of warnings (default: 0)', + ) .description('Lint a package') .action(lazy(() => import('./lint').then(m => m.default))); diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index 87c353f56f..8816221fb4 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -29,6 +29,13 @@ export default async (directories: string[], opts: OptionValues) => { directories.length ? directories : ['.'], ); + const maxWarnings = opts.maxWarnings ?? 0; + + const failed = + results.some(r => r.errorCount > 0) || + results.reduce((current, next) => current + next.warningCount, 0) > + maxWarnings; + if (opts.fix) { await ESLint.outputFixes(results); } @@ -39,12 +46,14 @@ export default async (directories: string[], opts: OptionValues) => { if (opts.format === 'eslint-formatter-friendly') { process.chdir(paths.targetRoot); } + const resultText = formatter.format(results); - // If there is any feedback at all, we treat it as a lint failure. This should be - // consistent with our old behavior of passing `--max-warnings=0` when invoking eslint. if (resultText) { console.log(resultText); + } + + if (failed) { process.exit(1); } }; diff --git a/packages/cli/src/commands/repo/build.ts b/packages/cli/src/commands/repo/build.ts index 3ed528cd50..7e6d235e7d 100644 --- a/packages/cli/src/commands/repo/build.ts +++ b/packages/cli/src/commands/repo/build.ts @@ -27,57 +27,7 @@ import { import { runParallelWorkers } from '../../lib/parallel'; import { buildFrontend } from '../build/buildFrontend'; import { buildBackend } from '../build/buildBackend'; - -function createScriptOptionsParser(anyCmd: Command, commandPath: string[]) { - // Regardless of what command instance is passed in we want to find - // the root command and resolve the path from there - let rootCmd = anyCmd; - while (rootCmd.parent) { - rootCmd = rootCmd.parent; - } - - // Now find the command that was requested - let targetCmd = rootCmd as Command | undefined; - for (const name of commandPath) { - targetCmd = targetCmd?.commands.find(c => c.name() === name) as - | Command - | undefined; - } - - if (!targetCmd) { - throw new Error( - `Could not find package command '${commandPath.join(' ')}'`, - ); - } - const cmd = targetCmd; - - const expectedScript = `backstage-cli ${commandPath.join(' ')}`; - - return (scriptStr?: string) => { - if (!scriptStr || !scriptStr.startsWith(expectedScript)) { - return undefined; - } - - const argsStr = scriptStr.slice(expectedScript.length).trim(); - - // Can't clone or copy or even use commands as prototype, so we mutate - // the necessary members instead, and then reset them once we're done - const currentOpts = (cmd as any)._optionValues; - const currentStore = (cmd as any)._storeOptionsAsProperties; - - const result: Record = {}; - (cmd as any)._storeOptionsAsProperties = false; - (cmd as any)._optionValues = result; - - // Triggers the writing of options to the result object - cmd.parseOptions(argsStr.split(' ')); - - (cmd as any)._storeOptionsAsProperties = currentOpts; - (cmd as any)._optionValues = currentStore; - - return result; - }; -} +import { createScriptOptionsParser } from './optionsParser'; export async function command(opts: OptionValues, cmd: Command): Promise { let packages = await PackageGraph.listTargetPackages(); diff --git a/packages/cli/src/commands/repo/lint.ts b/packages/cli/src/commands/repo/lint.ts index 05fbf7b87e..d0d6ee8f06 100644 --- a/packages/cli/src/commands/repo/lint.ts +++ b/packages/cli/src/commands/repo/lint.ts @@ -15,11 +15,12 @@ */ import chalk from 'chalk'; -import { OptionValues } from 'commander'; +import { Command, OptionValues } from 'commander'; import { relative as relativePath } from 'path'; import { PackageGraph, BackstagePackageJson } from '@backstage/cli-node'; import { paths } from '../../lib/paths'; import { runWorkerQueueThreads } from '../../lib/parallel'; +import { createScriptOptionsParser } from './optionsParser'; function depCount(pkg: BackstagePackageJson) { const deps = pkg.dependencies ? Object.keys(pkg.dependencies).length : 0; @@ -29,7 +30,7 @@ function depCount(pkg: BackstagePackageJson) { return deps + devDeps; } -export async function command(opts: OptionValues): Promise { +export async function command(opts: OptionValues, cmd: Command): Promise { let packages = await PackageGraph.listTargetPackages(); if (opts.since) { @@ -54,22 +55,30 @@ export async function command(opts: OptionValues): Promise { process.env.FORCE_COLOR = '1'; } + const parseLintScript = createScriptOptionsParser(cmd, ['package', 'lint']); + const resultsList = await runWorkerQueueThreads({ items: packages.map(pkg => ({ fullDir: pkg.dir, relativeDir: relativePath(paths.targetRoot, pkg.dir), + lintOptions: parseLintScript(pkg.packageJson.scripts?.lint), })), workerData: { fix: Boolean(opts.fix), format: opts.format as string | undefined, }, workerFactory: async ({ fix, format }) => { - const { ESLint } = require('eslint'); + const { ESLint } = require('eslint') as typeof import('eslint'); return async ({ fullDir, relativeDir, - }): Promise<{ relativeDir: string; resultText: string }> => { + lintOptions, + }): Promise<{ + relativeDir: string; + resultText: string; + failed: boolean; + }> => { // Bit of a hack to make file resolutions happen from the correct directory // since some lint rules don't respect the cwd of ESLint process.cwd = () => fullDir; @@ -92,21 +101,34 @@ export async function command(opts: OptionValues): Promise { await ESLint.outputFixes(results); } - const resultText = formatter.format(results); + const maxWarnings = lintOptions?.maxWarnings ?? 0; + const resultText = formatter.format(results) as string; + const failed = + results.some(r => r.errorCount > 0) || + results.reduce((current, next) => current + next.warningCount, 0) > + maxWarnings; - return { relativeDir, resultText }; + return { + relativeDir, + resultText, + failed, + }; }; }, }); let failed = false; - for (const { relativeDir, resultText } of resultsList) { - if (resultText) { - console.log(); - console.log(chalk.red(`Lint failed in ${relativeDir}:`)); - console.log(resultText.trimStart()); - + for (const { relativeDir, resultText, failed: runFailed } of resultsList) { + if (runFailed) { + console.log(chalk.red(`Lint failed in ${relativeDir}`)); failed = true; + + // When doing repo lint, only list the results if the lint failed to avoid a log + // dump of all warnings that might be irrelevant + if (resultText) { + console.log(); + console.log(resultText.trimStart()); + } } } diff --git a/packages/cli/src/commands/repo/optionsParser.ts b/packages/cli/src/commands/repo/optionsParser.ts new file mode 100644 index 0000000000..45c7eac0aa --- /dev/null +++ b/packages/cli/src/commands/repo/optionsParser.ts @@ -0,0 +1,70 @@ +/* + * Copyright 2024 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 { Command } from 'commander'; + +export function createScriptOptionsParser( + anyCmd: Command, + commandPath: string[], +) { + // Regardless of what command instance is passed in we want to find + // the root command and resolve the path from there + let rootCmd = anyCmd; + while (rootCmd.parent) { + rootCmd = rootCmd.parent; + } + + // Now find the command that was requested + let targetCmd = rootCmd as Command | undefined; + for (const name of commandPath) { + targetCmd = targetCmd?.commands.find(c => c.name() === name) as + | Command + | undefined; + } + + if (!targetCmd) { + throw new Error( + `Could not find package command '${commandPath.join(' ')}'`, + ); + } + const cmd = targetCmd; + + const expectedScript = `backstage-cli ${commandPath.join(' ')}`; + + return (scriptStr?: string) => { + if (!scriptStr || !scriptStr.startsWith(expectedScript)) { + return undefined; + } + + const argsStr = scriptStr.slice(expectedScript.length).trim(); + + // Can't clone or copy or even use commands as prototype, so we mutate + // the necessary members instead, and then reset them once we're done + const currentOpts = (cmd as any)._optionValues; + const currentStore = (cmd as any)._storeOptionsAsProperties; + + const result: Record = {}; + (cmd as any)._storeOptionsAsProperties = false; + (cmd as any)._optionValues = result; + + // Triggers the writing of options to the result object + cmd.parseOptions(argsStr.split(' ')); + + (cmd as any)._storeOptionsAsProperties = currentOpts; + (cmd as any)._optionValues = currentStore; + + return result; + }; +}