-
Notifications
You must be signed in to change notification settings - Fork 90
feat(cli): warn on unrecognized CLI options #1514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6389343
14c05c2
9326598
a8d7d4b
9d21c60
91904c0
d28ab2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| /** | ||
| * Detect unrecognized CLI options and emit warnings. | ||
| * | ||
| * Yargs does not enable strict option checking by default, so unknown flags | ||
| * like `--region` (before it was added) are silently swallowed. This function | ||
| * compares the parsed argv keys against the known global and command options | ||
| * from the CLI type registry and warns for any that don't match. | ||
| */ | ||
| export function findUnknownOptions(argv: any): string[] { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const config = require('../cli-type-registry.json'); | ||
| const command = argv._[0]; | ||
|
|
||
| const globalOptions = new Set<string>(Object.keys(config.globalOptions)); | ||
| const commandOptions = new Set<string>(Object.keys(config.commands[command]?.options ?? {})); | ||
|
|
||
| // Collect all known aliases and negativeAliases | ||
| for (const [, optDef] of Object.entries<any>(config.globalOptions)) { | ||
| if (optDef.alias) { | ||
| const aliases = Array.isArray(optDef.alias) ? optDef.alias : [optDef.alias]; | ||
| for (const a of aliases) { | ||
| globalOptions.add(a); | ||
| } | ||
| } | ||
| if (optDef.negativeAlias) { | ||
| globalOptions.add(optDef.negativeAlias); | ||
| } | ||
| } | ||
| for (const [, optDef] of Object.entries<any>(config.commands[command]?.options ?? {})) { | ||
| if (optDef.alias) { | ||
| const aliases = Array.isArray(optDef.alias) ? optDef.alias : [optDef.alias]; | ||
| for (const a of aliases) { | ||
| commandOptions.add(a); | ||
| } | ||
| } | ||
| if (optDef.negativeAlias) { | ||
| commandOptions.add(optDef.negativeAlias); | ||
| } | ||
| } | ||
|
|
||
| // yargs internal keys to ignore | ||
| const yargsInternals = new Set(['_', '$0', 'help', 'h', 'version']); | ||
|
|
||
| // The command's positional arg name | ||
| const commandArg = config.commands[command]?.arg?.name; | ||
| if (commandArg) { | ||
| yargsInternals.add(commandArg); | ||
| } | ||
|
|
||
| const unknown: string[] = []; | ||
| for (const key of Object.keys(argv)) { | ||
| if (argv[key] === undefined) continue; | ||
| if (yargsInternals.has(key)) continue; | ||
| if (globalOptions.has(key)) continue; | ||
| if (commandOptions.has(key)) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be one
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually removed this entirely since it's the same issue as the first comment. The negation check was solving a problem that doesn't exist with how yargs works. |
||
|
|
||
| // yargs creates camelCase versions of kebab-case options — skip those | ||
| const kebab = camelToKebab(key); | ||
| if (kebab !== key && (globalOptions.has(kebab) || commandOptions.has(kebab))) continue; | ||
|
|
||
| // yargs creates "noFoo" keys for --no-foo boolean negations — skip those | ||
| if (key.startsWith('no') && key.length > 2 && key[2] === key[2].toUpperCase()) { | ||
| const positiveKey = key[2].toLowerCase() + key.slice(3); | ||
| const positiveKebab = camelToKebab(positiveKey); | ||
| if (globalOptions.has(positiveKey) || commandOptions.has(positiveKey) || | ||
| globalOptions.has(positiveKebab) || commandOptions.has(positiveKebab)) continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should enforce this as a linter rule, but imho these lines are hard to parse and read. Prefer always using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, changed it |
||
| } | ||
|
|
||
| // yargs .env('CDK') injects CDK_* environment variables as camelCase argv | ||
| // keys (e.g. CDK_INTEG_ATMOSPHERE_POOL -> integAtmospherePool). These are | ||
| // intentional configuration from the environment, not user typos. | ||
| if (isFromEnvPrefix(key, 'CDK')) continue; | ||
|
|
||
| unknown.push(key); | ||
| } | ||
|
|
||
| return unknown; | ||
| } | ||
|
|
||
| function camelToKebab(str: string): string { | ||
| return str.replace(/[A-Z]/g, (m) => `-${m.toLowerCase()}`); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably happens to work in this case, but is likely not working in the general case. For your code, it would be much easier to go the other way: From kebab to camel.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah okay, switched it |
||
|
|
||
| /** | ||
| * Checks whether a camelCase argv key was injected by yargs' .env(PREFIX) | ||
| * feature. yargs converts PREFIX_FOO_BAR env vars into camelCase keys | ||
| * (fooBar). We reverse the mapping and check if the env var exists. | ||
| */ | ||
| function isFromEnvPrefix(key: string, prefix: string): boolean { | ||
| const screamingSnake = key.replace(/[A-Z]/g, (m) => `_${m}`).toUpperCase(); | ||
| return process.env[`${prefix}_${screamingSnake}`] !== undefined; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import { findUnknownOptions } from '../../../lib/cli/util/check-unknown-options'; | ||
|
|
||
| describe('findUnknownOptions', () => { | ||
| test('returns empty array for known global options', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| profile: 'my-profile', | ||
| verbose: 1, | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('returns empty array for known command options', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| force: true, | ||
| all: false, | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('detects unknown options', () => { | ||
| const argv = { | ||
| _: ['bootstrap'], | ||
| $0: 'cdk', | ||
| profile: 'my-profile', | ||
| fakeOption: 'value', | ||
| }; | ||
| const unknown = findUnknownOptions(argv); | ||
| expect(unknown).toContain('fakeOption'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can be stricter with this check. It shouldn't just contain
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, changed to |
||
| }); | ||
|
|
||
| test('does not report camelCase variants of known kebab-case options', () => { | ||
| const argv = { | ||
| '_': ['deploy'], | ||
| '$0': 'cdk', | ||
| 'ca-bundle-path': '/tmp/ca.pem', | ||
| 'caBundlePath': '/tmp/ca.pem', | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('does not report yargs internal keys', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| help: false, | ||
| h: false, | ||
| version: false, | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('does not report aliases', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| v: 1, | ||
| j: false, | ||
| a: 'node bin/app.js', | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('does not report yargs boolean negation keys (noFoo for --no-foo)', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| rollback: false, | ||
| noRollback: true, | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('does not report negativeAlias keys', () => { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| R: true, | ||
| rollback: false, | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| }); | ||
|
|
||
| test('does not report keys injected by yargs .env("CDK") from environment variables', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but why?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dug into it and the test was actually wrong. I ran the actual CDK parser and |
||
| process.env.CDK_INTEG_ATMOSPHERE_POOL = 'test-pool'; | ||
| process.env.CDK_MAJOR_VERSION = '2'; | ||
| try { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| integAtmospherePool: 'test-pool', | ||
| majorVersion: '2', | ||
| }; | ||
| expect(findUnknownOptions(argv)).toEqual([]); | ||
| } finally { | ||
| delete process.env.CDK_INTEG_ATMOSPHERE_POOL; | ||
| delete process.env.CDK_MAJOR_VERSION; | ||
| } | ||
| }); | ||
|
|
||
| test('still reports truly unknown options even when CDK_ env vars exist', () => { | ||
| process.env.CDK_INTEG_ATMOSPHERE_POOL = 'test-pool'; | ||
| try { | ||
| const argv = { | ||
| _: ['deploy'], | ||
| $0: 'cdk', | ||
| integAtmospherePool: 'test-pool', | ||
| totallyFakeOption: 'value', | ||
| }; | ||
| const unknown = findUnknownOptions(argv); | ||
| expect(unknown).not.toContain('integAtmospherePool'); | ||
| expect(unknown).toContain('totallyFakeOption'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this test, I would recommend comparing unknown to an exact object.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same change, updated to |
||
| } finally { | ||
| delete process.env.CDK_INTEG_ATMOSPHERE_POOL; | ||
| } | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this doesn't change, so can be outside the function as a "global" variable. In practice this doesn't matter much because
findUnknownOptionsis only called once andrequireis cached, but you should start to thinking about these kind of things.The same applies to much of the "setup" code later on: sorting global and command options etc.
A good question to ask is: Which parts of the code are actually dependent on the
argvinput? Which parts only depend on the staticcli-type-registry.json? This will give you an idea how to split it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, thank you! Moved the registry load and global options set to module scope since they never change. Only the command-specific options are computed inside the function now since those depend on which command was invoked.