fix(cli): correctly handle tags when deploying multiple stacks (#3455)

Fixes a bug where stacks being declared with different tags end up with
identical tags when deployed to CloudFormation, due to the first tagged stack's
tags overriding the value of `options.tags`.

This change makes it so that the `options.tags` value is not overridden, and the
actual tag list to be used is determined in the same way for each stack that
will be deployed.

Fixes #3471

Author: Simon-Pierre Gingras

Diff for: packages/aws-cdk/lib/cdk-toolkit.ts

@@ -112,8 +112,9 @@ export class CdkToolkit {
         print('%s: deploying...', colors.bold(stack.name));
       }
 
-      if (!options.tags || options.tags.length === 0) {
-        options.tags = this.appStacks.getTagsFromStackMetadata(stack);
+      let tags = options.tags;
+      if (!tags || tags.length === 0) {
+        tags = this.appStacks.getTagsFromStackMetadata(stack);
       }
 
       try {
@@ -124,7 +125,7 @@ export class CdkToolkit {
           ci: options.ci,
           toolkitStackName: options.toolkitStackName,
           reuseAssets: options.reuseAssets,
-          tags: options.tags
+          tags
         });
 
         const message = result.noOp

Diff for: packages/aws-cdk/test/test.cdk-toolkit.ts

@@ -0,0 +1,127 @@
+import cxapi = require('@aws-cdk/cx-api');
+import nodeunit = require('nodeunit');
+import { AppStacks, Tag } from '../lib/api/cxapp/stacks';
+import { DeployStackResult } from '../lib/api/deploy-stack';
+import { DeployStackOptions, IDeploymentTarget, Template } from '../lib/api/deployment-target';
+import { CdkToolkit } from '../lib/cdk-toolkit';
+
+export = nodeunit.testCase({
+  deploy: {
+    'makes correct CloudFormation calls': {
+      'without options'(test: nodeunit.Test) {
+        // GIVEN
+        const toolkit = new CdkToolkit({
+          appStacks: new TestAppStacks(test),
+          provisioner: new TestProvisioner(test, {
+            'Test-Stack-A': { Foo: 'Bar' },
+            'Test-Stack-B': { Baz: 'Zinga!' },
+          }),
+        });
+
+        // WHEN
+        toolkit.deploy({ stackNames: ['Test-Stack-A', 'Test-Stack-B'] });
+
+        // THEN
+        test.done();
+      },
+    },
+  },
+});
+
+class MockStack {
+  constructor(
+    public readonly name: string,
+    public readonly originalName: string = name,
+    public readonly template: any = { Resources: { TempalteName: name } },
+    public readonly templateFile: string = `fake/stack/${name}.json`,
+    public readonly assets: cxapi.AssetMetadataEntry[] = [],
+    public readonly parameters: { [id: string]: string } = {},
+    public readonly environment: cxapi.Environment = { name: 'MockEnv', account: '123456789012', region: 'bermuda-triangle-1' },
+  ) {}
+}
+
+class TestAppStacks extends AppStacks {
+  public static readonly MOCK_STACK_A = new MockStack('Test-Stack-A');
+  public static readonly MOCK_STACK_B = new MockStack('Test-Stack-B');
+
+  constructor(private readonly test: nodeunit.Test) {
+    super(undefined as any);
+  }
+
+  public getTagsFromStackMetadata(stack: cxapi.CloudFormationStackArtifact): Tag[] {
+    switch (stack.name) {
+      case TestAppStacks.MOCK_STACK_A.name:
+        return [{ Key: 'Foo', Value: 'Bar' }];
+      case TestAppStacks.MOCK_STACK_B.name:
+        return [{ Key: 'Baz', Value: 'Zinga!' }];
+      default:
+        throw new Error(`Not an expected mock stack: ${stack.name}`);
+    }
+  }
+
+  public selectStacks(selectors: string[]): Promise<cxapi.CloudFormationStackArtifact[]> {
+    this.test.deepEqual(selectors, ['Test-Stack-A', 'Test-Stack-B']);
+    return Promise.resolve([
+      // Cheating the type system here (intentionally, so we have to stub less!)
+      TestAppStacks.MOCK_STACK_A as cxapi.CloudFormationStackArtifact,
+      TestAppStacks.MOCK_STACK_B as cxapi.CloudFormationStackArtifact,
+    ]);
+  }
+
+  public processMetadata(stacks: cxapi.CloudFormationStackArtifact[]): void {
+    stacks.forEach(stack =>
+      this.test.ok(stack === TestAppStacks.MOCK_STACK_A || stack === TestAppStacks.MOCK_STACK_B,
+        `Not an expected mock stack: ${stack.name}`));
+  }
+
+  public listStacks(): never {
+    throw new Error('Not Implemented');
+  }
+
+  public synthesizeStack(): never {
+    throw new Error('Not Implemented');
+  }
+
+  public synthesizeStacks(): never {
+    throw new Error('Not Implemented');
+  }
+}
+
+class TestProvisioner implements IDeploymentTarget {
+  private readonly expectedTags: { [sytackName: string]: Tag[] } = {};
+
+  constructor(
+    private readonly test: nodeunit.Test,
+    expectedTags: { [sytackName: string]: { [kay: string]: string } } = {},
+  ) {
+    for (const [stackName, tags] of Object.entries(expectedTags)) {
+      this.expectedTags[stackName] =
+        Object.entries(tags).map(([Key, Value]) => ({ Key, Value }))
+          .sort((l, r) =>  l.Key.localeCompare(r.Key));
+    }
+  }
+
+  public deployStack(options: DeployStackOptions): Promise<DeployStackResult> {
+    this.test.ok(
+      options.stack.name === TestAppStacks.MOCK_STACK_A.name || options.stack.name === TestAppStacks.MOCK_STACK_B.name,
+      `Not an expected mock stack: ${options.stack.name}`
+    );
+    this.test.deepEqual(options.tags, this.expectedTags[options.stack.name]);
+    return Promise.resolve({
+      stackArn: `arn:aws:cloudformation:::stack/${options.stack.name}/MockedOut`,
+      noOp: false,
+      outputs: { StackName: options.stack.name },
+    });
+  }
+
+  public readCurrentTemplate(stack: cxapi.CloudFormationStackArtifact): Promise<Template> {
+    switch (stack.name) {
+      case TestAppStacks.MOCK_STACK_A.name:
+        return Promise.resolve({});
+      case TestAppStacks.MOCK_STACK_B.name:
+        return Promise.resolve({});
+      default:
+        return Promise.reject(`Not an expected mock stack: ${stack.name}`);
+    }
+  }
+}