fix(toolkit): only fail if errors are on selected stacks (#1807)

If we report synthesis errors, they should only prevent the
affected stack from deploying; right now, since they are checked
during the synthesis step, they would abort the entire toolkit
run regardless of whether they would be selected or not.

Makes it possible to do region-dependent verification.

Fixes #1784.

ALSO IN THIS COMMIT

If the 'cdk synth' output goes to the screen, don't automatically
select upstream stacks for synthesis (as that would lead to an
immediate error).

Fixes #1783.

packages/aws-cdk/bin/cdk.ts

@@ -9,6 +9,7 @@ import yargs = require('yargs');
 
 import { bootstrapEnvironment, deployStack, destroyStack, loadToolkitInfo, Mode, SDK } from '../lib';
 import { environmentsFromDescriptors, globEnvironmentsFromStacks } from '../lib/api/cxapp/environments';
+import { execProgram } from '../lib/api/cxapp/exec';
 import { AppStacks, ExtendedStackSelection, listStackNames } from '../lib/api/cxapp/stacks';
 import { leftPad } from '../lib/api/util/string-manipulation';
 import { printSecurityDiff, printStackDiff, RequireApproval } from '../lib/diff';
@@ -107,7 +108,11 @@ async function initCommandLine() {
   await configuration.load();
   configuration.logDefaults();
 
-  const appStacks = new AppStacks(argv, configuration, aws);
+  const appStacks = new AppStacks({
+    verbose: argv.trace || argv.verbose,
+    ignoreErrors: argv.ignoreErrors,
+    strict: argv.strict,
+    configuration, aws, synthesizer: execProgram });
 
   const renames = parseRenames(argv.rename);
 
@@ -250,7 +255,10 @@ async function initCommandLine() {
                                outputDir: string|undefined,
                                json: boolean,
                                numbered: boolean): Promise<any> {
-    const stacks = await appStacks.selectStacks(stackNames, exclusively ? ExtendedStackSelection.None : ExtendedStackSelection.Upstream);
+    // Only autoselect dependencies if it doesn't interfere with user request or output options
+    const autoSelectDependencies = !exclusively && outputDir !== undefined;
+
+    const stacks = await appStacks.selectStacks(stackNames, autoSelectDependencies ? ExtendedStackSelection.Upstream : ExtendedStackSelection.None);
     renames.validateSelectedStacks(stacks);
 
     if (doInteractive) {

packages/aws-cdk/lib/api/cxapp/stacks.ts

@@ -1,14 +1,52 @@
 import cxapi = require('@aws-cdk/cx-api');
 import colors = require('colors/safe');
 import minimatch = require('minimatch');
-import yargs = require('yargs');
 import contextproviders = require('../../context-providers');
 import { debug, error, print, warning } from '../../logging';
-import { Configuration } from '../../settings';
+import { Configuration, Settings } from '../../settings';
 import cdkUtil = require('../../util');
 import { SDK } from '../util/sdk';
 import { topologicalSort } from '../util/toposort';
-import { execProgram } from './exec';
+
+type Synthesizer = (aws: SDK, config: Settings) => Promise<cxapi.SynthesizeResponse>;
+
+export interface AppStacksProps {
+  /**
+   * Whether to be verbose
+   *
+   * @default false
+   */
+  verbose?: boolean;
+
+  /**
+   * Don't stop on error metadata
+   *
+   * @default false
+   */
+  ignoreErrors?: boolean;
+
+  /**
+   * Treat warnings in metadata as errors
+   *
+   * @default false
+   */
+  strict?: boolean;
+
+  /**
+   * Application configuration (settings and context)
+   */
+  configuration: Configuration;
+
+  /**
+   * AWS object (used by synthesizer and contextprovider)
+   */
+  aws: SDK;
+
+  /**
+   * Callback invoked to synthesize the actual stacks
+   */
+  synthesizer: Synthesizer;
+}
 
 /**
  * Routines to get stacks from an app
@@ -22,7 +60,7 @@ export class AppStacks {
    */
   private cachedResponse?: cxapi.SynthesizeResponse;
 
-  constructor(private readonly argv: yargs.Arguments, private readonly configuration: Configuration, private readonly aws: SDK) {
+  constructor(private readonly props: AppStacksProps) {
   }
 
   /**
@@ -76,7 +114,11 @@ export class AppStacks {
     }
 
     // Filter original array because it is in the right order
-    return stacks.filter(s => selectedStacks.has(s.name));
+    const selectedList = stacks.filter(s => selectedStacks.has(s.name));
+
+    // Only check selected stacks for errors
+    this.processMessages(selectedList);
+    return selectedList;
   }
 
   /**
@@ -112,34 +154,24 @@ export class AppStacks {
       return this.cachedResponse;
     }
 
-    const trackVersions: boolean = this.configuration.combined.get(['versionReporting']);
+    const trackVersions: boolean = this.props.configuration.combined.get(['versionReporting']);
 
     // We may need to run the cloud executable multiple times in order to satisfy all missing context
     while (true) {
-      const response: cxapi.SynthesizeResponse = await execProgram(this.aws, this.configuration.combined);
+      const response: cxapi.SynthesizeResponse = await this.props.synthesizer(this.props.aws, this.props.configuration.combined);
       const allMissing = cdkUtil.deepMerge(...response.stacks.map(s => s.missing));
 
       if (!cdkUtil.isEmpty(allMissing)) {
         debug(`Some context information is missing. Fetching...`);
 
-        await contextproviders.provideContextValues(allMissing, this.configuration.projectConfig, this.aws);
+        await contextproviders.provideContextValues(allMissing, this.props.configuration.projectConfig, this.props.aws);
 
         // Cache the new context to disk
-        await this.configuration.saveProjectConfig();
+        await this.props.configuration.saveProjectConfig();
 
         continue;
       }
 
-      const { errors, warnings } = this.processMessages(response);
-
-      if (errors && !this.argv.ignoreErrors) {
-        throw new Error('Found errors');
-      }
-
-      if (this.argv.strict && warnings) {
-        throw new Error('Found warnings (--strict mode)');
-      }
-
       if (trackVersions && response.runtime) {
         const modules = formatModules(response.runtime);
         for (const stack of response.stacks) {
@@ -181,10 +213,10 @@ export class AppStacks {
   /**
    * Extracts 'aws:cdk:warning|info|error' metadata entries from the stack synthesis
    */
-  private processMessages(stacks: cxapi.SynthesizeResponse): { errors: boolean, warnings: boolean } {
+  private processMessages(stacks: cxapi.SynthesizedStack[]) {
     let warnings = false;
     let errors = false;
-    for (const stack of stacks.stacks) {
+    for (const stack of stacks) {
       for (const id of Object.keys(stack.metadata)) {
         const metadata = stack.metadata[id];
         for (const entry of metadata) {
@@ -204,13 +236,20 @@ export class AppStacks {
         }
       }
     }
-    return { warnings, errors };
+
+    if (errors && !this.props.ignoreErrors) {
+      throw new Error('Found errors');
+    }
+
+    if (this.props.strict && warnings) {
+      throw new Error('Found warnings (--strict mode)');
+    }
   }
 
   private printMessage(logFn: (s: string) => void, prefix: string, id: string, entry: cxapi.MetadataEntry) {
     logFn(`[${prefix} at ${id}] ${entry.data}`);
 
-    if (this.argv.trace || this.argv.verbose) {
+    if (this.props.verbose) {
       logFn(`  ${entry.trace.join('\n  ')}`);
     }
   }

packages/aws-cdk/lib/api/util/sdk.ts

@@ -50,7 +50,7 @@ export class SDK {
   private readonly credentialsCache: CredentialsCache;
   private readonly profile?: string;
 
-  constructor(options: SDKOptions) {
+  constructor(options: SDKOptions = {}) {
     this.profile = options.profile;
 
     const defaultCredentialProvider = makeCLICompatibleCredentialProvider(options.profile, options.ec2creds);

packages/aws-cdk/test/api/test.stacks.ts

@@ -0,0 +1,69 @@
+import cxapi = require('@aws-cdk/cx-api');
+import { Test } from 'nodeunit';
+import { SDK } from '../../lib';
+import { AppStacks, ExtendedStackSelection } from '../../lib/api/cxapp/stacks';
+import { Configuration } from '../../lib/settings';
+
+const FIXED_RESULT: cxapi.SynthesizeResponse = {
+  version: '1',
+  stacks: [
+    {
+      name: 'withouterrors',
+      template: { resource: 'noerrorresource' },
+      environment: { name: 'dev', account: '12345', region: 'here' },
+      metadata: {},
+    },
+    {
+      name: 'witherrors',
+      template: { resource: 'errorresource' },
+      environment: { name: 'dev', account: '12345', region: 'here' },
+      metadata: {
+        '/resource': [
+          {
+            type: cxapi.ERROR_METADATA_KEY,
+            data: 'this is an error',
+            trace: []
+          }
+        ]
+      }
+    }
+  ]
+};
+
+export = {
+  async 'do not throw when selecting stack without errors'(test: Test) {
+    // GIVEN
+    const stacks = new AppStacks({
+      configuration: new Configuration(),
+      aws: new SDK(),
+      synthesizer: async () => FIXED_RESULT,
+    });
+
+    // WHEN
+    const selected = await stacks.selectStacks(['withouterrors'], ExtendedStackSelection.None);
+
+    // THEN
+    test.equal(selected[0].template.resource, 'noerrorresource');
+
+    test.done();
+  },
+
+  async 'do throw when selecting stack with errors'(test: Test) {
+    // GIVEN
+    const stacks = new AppStacks({
+      configuration: new Configuration(),
+      aws: new SDK(),
+      synthesizer: async () => FIXED_RESULT,
+    });
+
+    // WHEN
+    try {
+      await stacks.selectStacks(['witherrors'], ExtendedStackSelection.None);
+      test.ok(false, 'Did not get exception');
+    } catch (e) {
+      test.ok(/Found errors/.test(e.toString()), 'Wrong error');
+    }
+
+    test.done();
+  },
+};