Skip to content

Commit

Permalink
fix(cli): default command line args override cdk.json when not specified
Browse files Browse the repository at this point in the history
Previously, pathMetadata, assetMetadata, debug and staging could not be set in `cdk.json`
as command-line parameters defaults would always overwrite this. In particular, this
`cdk.json` would be ignored:

```
{
  "app": "npx ts-node --prefer-ts-exts bin/example",
  ...
  "pathMetadata": false,
  "assetMetadata": false
}
```

The issue is this piece of code:
```
 this.settings = this.defaultConfig
            .merge(userConfig)
            .merge(this.projectConfig)
            .merge(this.commandLineArguments)
            .makeReadOnly();
```

`commandLineArguments` returns true for the above mentioned options. This commit is a copy
of #21891 which fell through the cracks, so I'm picking
up as this can potentially cause outages to resources that have drift under the hood, and
adding metadata looks like an `Update` operation to CloudFormation.

There are already unit tests around the cli which show that the same behavior works, located
here:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

fixes #3573
shoutout to @berenddeboer who did the work, I'm just reviving it.
  • Loading branch information
berenddeboer authored and iph committed Mar 9, 2023
1 parent 21ad7a7 commit ea849cb
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ async function parseCommandLineArguments(args: string[]) {
.option('ca-bundle-path', { type: 'string', desc: 'Path to CA certificate to use when validating HTTPS requests. Will read from AWS_CA_BUNDLE environment variable if not specified', requiresArg: true })
.option('ec2creds', { type: 'boolean', alias: 'i', default: undefined, desc: 'Force trying to fetch EC2 instance credentials. Default: guess EC2 instance status' })
.option('version-reporting', { type: 'boolean', desc: 'Include the "AWS::CDK::Metadata" resource in synthesized templates (enabled by default)', default: undefined })
.option('path-metadata', { type: 'boolean', desc: 'Include "aws:cdk:path" CloudFormation metadata for each resource (enabled by default)', default: true })
.option('asset-metadata', { type: 'boolean', desc: 'Include "aws:asset:*" CloudFormation metadata for resources that uses assets (enabled by default)', default: true })
.option('path-metadata', { type: 'boolean', desc: 'Include "aws:cdk:path" CloudFormation metadata for each resource (enabled by default)', default: undefined })
.option('asset-metadata', { type: 'boolean', desc: 'Include "aws:asset:*" CloudFormation metadata for resources that uses assets (enabled by default)', default: undefined })
.option('role-arn', { type: 'string', alias: 'r', desc: 'ARN of Role to use when invoking CloudFormation', default: undefined, requiresArg: true })
.option('staging', { type: 'boolean', desc: 'Copy assets to the output directory (use --no-staging to disable the copy of assets which allows local debugging via the SAM CLI to reference the original source files)', default: true })
.option('output', { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true })
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/lib/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export class Configuration {

public readonly defaultConfig = new Settings({
versionReporting: true,
assetMetadata: true,
pathMetadata: true,
output: 'cdk.out',
});
Expand Down

0 comments on commit ea849cb

Please sign in to comment.