-
Notifications
You must be signed in to change notification settings - Fork 61
fix: notices key in cdk.json is not respected #821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #821 +/- ##
==========================================
- Coverage 82.82% 82.42% -0.41%
==========================================
Files 65 65
Lines 9509 9523 +14
Branches 1119 1109 -10
==========================================
- Hits 7876 7849 -27
- Misses 1599 1643 +44
+ Partials 34 31 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/aws-cdk/lib/cli/cli.ts
Outdated
| const configNotices = configuration.settings.get(['notices']); | ||
| if (configNotices !== undefined) { | ||
| // Consider string "false" to be falsy in this context | ||
| shouldDisplayNotices = configNotices !== "false" && Boolean(configNotices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle "false" as falsy.
Handle "", 0, null, and undefined as falsy.
| export function shouldDisplayNotices(): boolean { | ||
| return !isCI() || Boolean(ciSystemIsStdErrSafe()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this logic to cli code.
Signed-off-by: github-actions <[email protected]>
Signed-off-by: github-actions <[email protected]>
Fixes #724
Description
Setting
"notices": falsein 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
undefinedby default. Moved the logic fromYARGS_HELPERS.shouldDisplayNotices()into thecli.ts. This has several implications.noticesin config now functions as expected; it will still not override the command line settingnotices: trueornotices: [truthy value]is set in config--noticesis passed, and NOT printed if--no-notices/--notices=falseis 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:
--noticesor--no-notices)"notices": falsein cdk.json)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license