Aws-cdk: Tags not working after version 0.34.0

[AlexRex]

Describe the bug
After adding this PR (https://github.com/awslabs/aws-cdk/pull/2185) into the library we cannot deploy Tags anymore. We get a validation error

To Reproduce
With version:

$ cdk --version
0.34.0 (build 523807c)

Create new sample App:

$ cdk init sample-app --language=typescript

Add tags:

#!/usr/bin/env node
import cdk = require('@aws-cdk/cdk');
import { CdkHacksStack } from '../lib/cdk-hacks-stack';

const app = new cdk.App();
new CdkHacksStack(app, 'CdkHacksStack', {
  env: {
    region: 'eu-west-1'
  },
  tags: {
    hack: 'nope!'
  }
});

Run deploy:

$ npx cdk deploy

Result:

 ❌  CdkHacksStack failed: MultipleValidationErrors: There were 3 validation errors:
* MissingRequiredParameter: Missing required key 'Key' in params.Tags[0]
* MissingRequiredParameter: Missing required key 'Value' in params.Tags[0]
* UnexpectedParameter: Unexpected key '0' found in params.Tags[0]
There were 3 validation errors:
* MissingRequiredParameter: Missing required key 'Key' in params.Tags[0]
* MissingRequiredParameter: Missing required key 'Value' in params.Tags[0]
* UnexpectedParameter: Unexpected key '0' found in params.Tags[0]

Expected behavior
It should deploy the app without validation errors.

Version:

• OS: OS X
• Programming Language: Typescript
• CDK Version: 0.34.0

I've been debugging a bit and it looks like in the class param_validator the Tags object is an array of arrays [ [ { Key: 'hack', Value: 'nope!' } ] ]. Manually changing this file to flatten the Tags object solves the problem:

    if (params.Tags) {
      params.Tags = params.Tags[0];

    }

The unique way I found the feature working is when passing the parameters in the CLI.

[skinny85]

Confirming I was able to reproduce the issue locally. Will be working on a fix.

[IsmaelMartinez]

Hi @AlexRex ,

I might be wrong but I think that it seems that there has been an issue with a refactoring of the code:

I think this:
https://github.com/awslabs/aws-cdk/commit/b2e196446be7693d991ce725d8e506d74a6c5d3c#diff-a8a1e8d6c0687920790eb29fc6ea3eabL241

does not produce the same as:
https://github.com/awslabs/aws-cdk/commit/b2e196446be7693d991ce725d8e506d74a6c5d3c#diff-d36d206d318a0ef98e48d1789880b6e9R143

So I think is related to https://github.com/awslabs/aws-cdk/pull/2731

Might be worth checking with @eladb about the refactoring from https://github.com/awslabs/aws-cdk/pull/2731.

I am really happy to help, and will probably spend a bit of time tomorrow morning trying to understand the cruds of the refactoring done, but my knowledge of cdk code/core is limited to a month trying to get that cdk deploy tags working ;)

Thanks and apologies again @AlexRex

[IsmaelMartinez]

Modifying this line:
https://github.com/awslabs/aws-cdk/blob/master/packages/aws-cdk/lib/api/cxapp/stacks.ts#L221

from this:
return stack.findMetadataByType(cxapi.STACK_TAGS_METADATA_KEY).map(x => x.data);
to this (basically, removing the second array in there):
return stack.findMetadataByType(cxapi.STACK_TAGS_METADATA_KEY).map(x => x.data)[0];

does the job.

All 3 then work mostly as expected. It does look like it doesn't detect that much the change on the stacks when removing the tags.

I believe it was doing that before but it might be also related to us not sending anymore an empty array around when there are no tags.

[skinny85]

Thanks for the research @IsmaelMartinez . I'll have a PR with the change ready.

Before that though, can clarify some of your comments for me?

All 3 then work mostly as expected.

What 3, exactly?

It does look like it doesn't detect that much the change on the stacks when removing the tags.

I believe it was doing that before but it might be also related to us not sending anymore an empty array around when there are no tags.

I don't understand this point. I think we still send an empty array, no? stack.findMetadataByType(cxapi.STACK_TAGS_METADATA_KEY) returns an empty array, and then map returns an empty array? What am I missing here?

[IsmaelMartinez]

@skinny85

Regarding all 3, I mean via constructor, applyAspect and parameter (cdk deploy --tags)

For the second part, yeah, I thought it will send an empty array, but they might be other changes that is affecting this (or it might have been not working before. I will confirm that tomorrow).

Basically, if you got tags and then you remove all of them (via constructor or aspect), the cdk don't detect the changes.

[IsmaelMartinez]

Hi @skinny85 , using the flatmap does the job (as it returns and empty array), so your pr works. I was been more "brute" on my code change.

I notice another "issue" with the constructor part.

#!/usr/bin/env node
import cdk = require('@aws-cdk/cdk');
import { CdkHacksStack } from '../lib/cdk-hacks-stack';

const app = new cdk.App();
new CdkHacksStack(app, 'CdkHacksStack', {
  env: {
    region: 'eu-west-1'
  },
  tags: {
    hack: 'nope!'
  }
});

That will not synth the tags. It will apply them, with your change, but not synth them.

If you do applyAspect, it does synth fine.

This might be either a think that I didn't pick up in my tests, or something todo with the division of the synth and deploy steps.