aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.71k stars 3.94k forks source link

CLI: Duplicate `concurrency` argument causes the CLI to silently exit with code 0 after the synth phase #30752

Open cmaster11 opened 4 months ago

cmaster11 commented 4 months ago

Describe the bug

Running the cdk deploy command and passing multiple instances of the --concurrency argument causes the CLI to halt after the synth phase and exit with code 0 and no output.

This causes deployments to silently fail. Nothing happens after the synth phase, which means the CDK does not communicate with CloudFormation, and stacks are not deployed. When used in a CI/CD context, this can cause transparent failures: exit code 0, the CI job is marked as successful, but no software has been deployed.

I fully understand this problem can be avoided by not using the same parameter multiple times, but the lack of validation can cause unexpected behaviors.

Expected Behavior

Either:

  1. The parsed args would "ignore" the first entry of the argument and use the second entry as the final value.
  2. The CLI would throw an error because of an invalid argument type.

Current Behavior

The CLI exits with code 0 and no error output straight after the synth phase.

Reproduction Steps

On any CDK project I've tried, run:

npm run cdk -- deploy --concurrency 1 --concurrency 10  --all

The last line of the output is:

✨  Synthesis time: 14.7s

The exit code is 0, and nothing else happens. With verbose mode enabled, I inspected the parsed arguments, and I can see the concurrency arg is turned into an array (this is the default behavior of the yargs library):

 concurrency: [ 1, 10 ],

Possible Solution

Validation

The yargs library lacks proper validation for the arguments (from what I've seen), but this issue could arise with an unpredictable amount of other CLI arguments. I feel like there should be a degree of validation of the arguments directly in the AWS CDK CLI.

It could also be a per-command Joi validation done on the incoming command arguments (e.g. put the validation of a Joi schema here).

This specific issue could be solved by disabling the duplicate-arguments-array config option, but this would also disable the ability to specify the same parameter multiple times to populate an array, which does not sound like the right way to go.

Another lib?

Have you considered using commander.js? It has a similar approach to yargs but a more predictable behavior.

I ran a simple test with both libraries, and commander.js works as I'd normally expect an args-parsing library to do.

# node index.js --str "a string" --arr "array string 1"
### yargs
args: { str: 'a string', arr: [ 'array string 1' ] }
### commander
args: { str: 'a string', arr: [ 'array string 1' ] }

# node index.js --str "a string" --arr "array string 1" --str "another string" --arr "array string 2"
### yargs
args: {
  str: [ 'a string', 'another string' ],
  arr: [ 'array string 1', 'array string 2' ]
}
### commander
args: { 
    str: 'another string', 
    arr: [ 'array string 1', 'array string 2' ] 
}

Additional Information/Context

There are some open issues about the lack of validation in the yargs repo:

https://github.com/yargs/yargs/issues/110 https://github.com/yargs/yargs/issues/1079

CDK CLI Version

2.147.3 (build 32f0fdb)

Framework Version

No response

Node.js Version

Node.js v20.10.0

OS

MacOS 14.4.1 (23E224)

Language

TypeScript

Language Version

TypeScript 4.9.5

ashishdhingra commented 4 months ago

Reproducible. Perhaps throwing error in case of duplicate --concurrency is a better option since we should not blindly consume 1st or 2nd option. May be this validation should be expanded to other parameters accepting a single value (this could be as simple check if the argument is an array rather than single value).