fix: notices key in cdk.json is not respected (#821)

Fixes #724

## Description
Setting `"notices": false` in cdk.json doesn't suppress notices.

The root cause is that we handle logic to determine whether we should
print notices at *yargs*. Yargs had a default value determined by a
helper function (`YARGS_HELPERS.shouldDisplayNotices()`) that returned
true when not in CI, or when in a safe CI.

This meant that any setting in our configuration would be overwritten by
the output of this helper, because it looks like it's coming from the
command line!

## Solution

Centralize logic for notices in `cli.ts`.

Made the notices option in yargs `undefined` by default. Moved the logic
from `YARGS_HELPERS.shouldDisplayNotices()` into the `cli.ts`. This has
several implications.
- Setting `notices` in config now functions as expected; it will still
not override the command line setting
- If a CDK app is running in a CI environment where it would not be safe
to print notices
- NEW BEHAVIOR: Notices WILL be printed if `notices: true` or `notices:
[truthy value]` is set in config
- Notices WILL be printed if flag `--notices` is passed, and NOT printed
if `--no-notices` / `--notices=false` is passed at the command line (was
already the case)

**Caveat: `"notices": "false"` in the config will suppress notices, but
other "truthy" values will cause them to be printed.**

### Testing
Added unit tests to verify the hierarchy of behavior:
1. Flags in CLI (`--notices` or `--no-notices`)
2. Setting in configuration (`"notices": false` in cdk.json)
3. Print notices if not in CI or in CI where it is safe

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

---------

Signed-off-by: github-actions <[email protected]>
Co-authored-by: github-actions <[email protected]>

Author: iankhou

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry-with-errors.integtest.ts

@@ -32,7 +32,6 @@ integTest(
               json: false,
               debug: false,
               staging: true,
-              notices: true,
               ['no-color']: false,
               ci: expect.anything(), // changes based on where this is called
               validation: true,
@@ -81,7 +80,6 @@ integTest(
               json: false,
               debug: false,
               staging: true,
-              notices: true,
               ['no-color']: false,
               ci: expect.anything(), // changes based on where this is called
               validation: true,

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry.integtest.ts

@@ -24,7 +24,6 @@ integTest(
               json: false,
               debug: false,
               staging: true,
-              notices: true,
               ['no-color']: false,
               ci: expect.anything(), // changes based on where this is called
               validation: true,
@@ -74,7 +73,6 @@ integTest(
               json: false,
               debug: false,
               staging: true,
-              notices: true,
               ['no-color']: false,
               ci: expect.anything(), // changes based on where this is called
               validation: true,

packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts

@@ -150,7 +150,7 @@ export class Notices {
    * Refresh the list of notices this instance is aware of.
    *
    * This method throws an error if the data source fails to fetch notices.
-   * When using, consider if execution should halt immdiately or if catching the rror and continuing is more appropriate
+   * When using, consider if execution should halt immdiately or if catching the error and continuing is more appropriate
    *
    * @throws on failure to refresh the data source
    */

packages/aws-cdk/lib/cli/cli-config.ts

@@ -39,7 +39,7 @@ export async function makeConfig(): Promise<CliConfig> {
       'role-arn': { type: 'string', alias: 'r', desc: 'ARN of Role to use when invoking CloudFormation', default: undefined, requiresArg: true },
       '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 },
       'output': { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true },
-      'notices': { type: 'boolean', desc: 'Show relevant notices', default: YARGS_HELPERS.shouldDisplayNotices() },
+      'notices': { type: 'boolean', desc: 'Show relevant notices' },
       'no-color': { type: 'boolean', desc: 'Removes colors and other style from console output', default: false },
       'ci': { type: 'boolean', desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr', default: YARGS_HELPERS.isCI() },
       'unstable': { type: 'array', desc: 'Opt in to unstable features. The flag indicates that the scope and API of a feature might still change. Otherwise the feature is generally production ready and fully supported. Can be specified multiple times.', default: [] },

packages/aws-cdk/lib/cli/cli.ts

@@ -4,6 +4,7 @@ import type { ChangeSetDeployment, DeploymentMethod, DirectDeployment } from '@a
 import { ToolkitError, Toolkit } from '@aws-cdk/toolkit-lib';
 import * as chalk from 'chalk';
 import { CdkToolkit, AssetBuildTime } from './cdk-toolkit';
+import { ciSystemIsStdErrSafe } from './ci-systems';
 import { displayVersionMessage } from './display-version';
 import type { IoMessageLevel } from './io-host';
 import { CliIoHost } from './io-host';
@@ -33,6 +34,7 @@ import type { StackSelector, Synthesizer } from '../cxapp';
 import { ProxyAgentProvider } from './proxy-agent';
 import { cdkCliErrorName } from './telemetry/error';
 import type { ErrorDetails } from './telemetry/schema';
+import { isCI } from './util/ci';
 import { isDeveloperBuildVersion, versionWithBuild, versionNumber } from './version';
 import { getLanguageFromAlias } from '../commands/language';
 
@@ -112,7 +114,34 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
     await ioHost.asIoHelper().defaults.trace(`Telemetry instantiation failed: ${e.message}`);
   }
 
-  const shouldDisplayNotices = configuration.settings.get(['notices']);
+  /**
+   * The default value for displaying (and refreshing) notices on all commands.
+   *
+   * If the user didn't supply either `--notices` or `--no-notices`, we do
+   * autodetection. The autodetection currently is: do write notices if we are
+   * not on CI, or are on a CI system where we know that writing to stderr is
+   * safe. We fail "closed"; that is, we decide to NOT print for unknown CI
+   * systems, even though technically we maybe could.
+   */
+  const isSafeToWriteNotices = !isCI() || Boolean(ciSystemIsStdErrSafe());
+
+  // Determine if notices should be displayed based on CLI args and configuration
+  let shouldDisplayNotices: boolean;
+  if (argv.notices !== undefined) {
+    // CLI argument takes precedence
+    shouldDisplayNotices = argv.notices;
+  } else {
+    // Fall back to configuration file setting, then autodetection
+    const configNotices = configuration.settings.get(['notices']);
+    if (configNotices !== undefined) {
+      // Consider string "false" to be falsy in this context
+      shouldDisplayNotices = configNotices !== 'false' && Boolean(configNotices);
+    } else {
+      // Default autodetection behavior
+      shouldDisplayNotices = isSafeToWriteNotices;
+    }
+  }
+
   // Notices either go to stderr, or nowhere
   ioHost.noticesDestination = shouldDisplayNotices ? 'stderr' : 'drop';
   const notices = Notices.create({
@@ -196,7 +225,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
         includeAcknowledged: !argv.unacknowledged,
         showTotal: argv.unacknowledged,
       });
-    } else if (cmd !== 'version') {
+    } else if (shouldDisplayNotices && cmd !== 'version') {
       await notices.display();
     }
   }

packages/aws-cdk/lib/cli/parse-command-line-arguments.ts

@@ -135,7 +135,7 @@ export function parseCommandLineArguments(args: Array<string>): any {
       requiresArg: true,
     })
     .option('notices', {
-      default: helpers.shouldDisplayNotices(),
+      default: undefined,
       type: 'boolean',
       desc: 'Show relevant notices',
     })

packages/aws-cdk/lib/cli/util/yargs-helpers.ts

@@ -1,5 +1,3 @@
-import { ciSystemIsStdErrSafe } from '../ci-systems';
-import { isCI } from '../util/ci';
 import { versionWithBuild } from '../version';
 
 export { isCI } from '../util/ci';
@@ -48,16 +46,3 @@ export function browserForPlatform(): string {
       return 'xdg-open %u';
   }
 }
-
-/**
- * The default value for displaying (and refreshing) notices on all commands.
- *
- * If the user didn't supply either `--notices` or `--no-notices`, we do
- * autodetection. The autodetection currently is: do write notices if we are
- * not on CI, or are on a CI system where we know that writing to stderr is
- * safe. We fail "closed"; that is, we decide to NOT print for unknown CI
- * systems, even though technically we maybe could.
- */
-export function shouldDisplayNotices(): boolean {
-  return !isCI() || Boolean(ciSystemIsStdErrSafe());
-}

packages/aws-cdk/test/cli/cli-arguments.test.ts

@@ -33,7 +33,7 @@ describe('yargs', () => {
         lookups: true,
         trace: undefined,
         unstable: [],
-        notices: expect.any(Boolean),
+        notices: undefined,
         output: undefined,
       },
       deploy: {