chore(cli): explicit defaults to yargs definition and cli arguments #32596

kaizencc · 2024-12-19T21:48:02Z

This makes sure we have a default value (even if its undefined) for every option in yargs. There is a special case for arrays where the default will be [].

I also took the liberty to simplify the eslint exemptions by changing the prettier options.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

tools/@aws-cdk/cli-args-gen/test/yargs-gen.test.ts

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

codecov · 2024-12-19T22:00:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.64%. Comparing base (0e1854d) to head (7d2475e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32596   +/-   ##
=======================================
  Coverage   80.64%   80.64%           
=======================================
  Files         107      107           
  Lines        6996     6996           
  Branches     1290     1290           
=======================================
  Hits         5642     5642           
  Misses       1175     1175           
  Partials      179      179

Flag	Coverage Δ
suite.unit	`80.64% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.64% <ø> (ø)`

HBobertz · 2024-12-24T00:11:31Z

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

@@ -2,7 +2,7 @@
 // GENERATED FROM packages/aws-cdk/lib/config.ts.
 // Do not edit by hand; all changes will be overwritten at build time from the config file.
 // -------------------------------------------------------------------------------------------
-/* eslint-disable @stylistic/comma-dangle, @stylistic/comma-spacing, @stylistic/max-len, @stylistic/quotes, @stylistic/quote-props */


We don't have to do it for this pr but in the future can we keep non-functional changes in a separate pr. It makes it a bit difficult to review when they're mixed together

while in principle you are right i don't think that the linter changes really affect your reading of the diff in this file. imo this is about leaving the codebase better than when you found it, and i'd rather not have to try to remember to do it retroactively especially for something as small as this. i think its a judgement call, and my judgement here is that i'd rather include it in this PR than give myself additional busy work. but if you feel differently i will move it to a different PR.

It's not a major impact for this pr but it's a bad habit to have. Mixing non-functional and functional changes in the same pr was a callout for the v3 migration regressions story as well.

But yeah, fine here. Just lets try to avoid this in the future

tools/@aws-cdk/cli-args-gen/lib/cli-type-gen.ts

HBobertz · 2024-12-24T00:21:38Z

tools/@aws-cdk/cli-args-gen/lib/cli-type-gen.ts

-    return String(defaultValue);
+function normalizeDefault(defaultValue: any, type: string): string {
+  const validDefaults = ['boolean', 'string', 'number', 'object'];
+  if (validDefaults.includes(typeof defaultValue)) {


99% sure typeof default here isn't nullsafe since the type of null is 'object' in typescript https://dev.to/_ravo_lution/why-typeof-null-is-object-181

We should make sure this is null/undefined safe.

JSON.stringify(null) = 'null' which seems reasonable to me. I also don't think there's any way that defaultValue actually is null.

HBobertz · 2024-12-24T00:25:32Z

tools/@aws-cdk/cli-args-gen/lib/yargs-gen.ts

  }).render(scope);

  return prettier.format(ts, {
    parser: 'typescript',
    printWidth: 150,
    singleQuote: true,
+    trailingComma: 'all',


same comment as above that I'd prefer these be in seperate prs so we're not crossing the streams but it's fine for now

tools/@aws-cdk/cli-args-gen/lib/cli-type-gen.ts

mrgrain · 2024-12-24T15:08:18Z

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

@@ -143,14 +143,14 @@ export interface GlobalOptions {
  /**
   * Add contextual string parameter (KEY=VALUE)
   *
-   * @default - undefined
+   * @default - []


There's a function somewhere that defines rules for array options:

undefined => not provided

empty array => deliberately an empty list

That might be why your tests are failing.

yep. I changed the default to [] because providing undefined was also causing problems. elsewhere we are expecting the context field to at least exist, which it doesn't if we default to undefined. I know why this is happening, but still thinking through what the best path forward is.

this PR is not meant to change any functionality. since yargs normally defaults to undefined, even for arrays, I will update this PR to reflect that and fix the unit tests that are failing in that case.

ok i got to the bottom of this and essentially this bug in yargs is the problem: yargs/yargs#2443

i'm back to defaulting arrays as [], except for notification-arns which should be defaulted to undefined, but right now that leads to notification-arns being [undefined].

going forward, it should be the case that unless otherwise specified, arrays are defaulted as []

kaizencc · 2024-12-24T21:40:11Z

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

   */
  readonly context?: Array<string>;

  /**
   * Name or path of a node package that extend the CDK features. Can be specified multiple times
   *
-   * @default - undefined
+   * @default - []


arrays now default to []

kaizencc · 2024-12-24T21:40:33Z

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

@@ -515,28 +515,28 @@ export interface GcOptions {
  /**
   * The action (or sub-action) you want to perform. Valid entires are "print", "tag", "delete-tagged", "full".
   *
-   * @default - full
+   * @default - "full"


this represents an improvement for string defaults

kaizencc · 2024-12-24T21:40:52Z

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

   */
  readonly type?: string;

  /**
   * Delete assets that have been marked as isolated for this many days
   *
-   * @default - undefined
+   * @default - 0


this fixes a bug where numbers did not render the right default

kaizencc · 2024-12-24T21:41:08Z

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

@@ -645,7 +645,7 @@ export interface DeployOptions {
  /**
   * Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)
   *
-   * @default - undefined
+   * @default - {}


this is probably a bug in config. either way i will look at it in a separate PR

kaizencc · 2024-12-24T21:46:30Z

tools/@aws-cdk/cli-args-gen/lib/yargs-gen.ts

+      // prevents us from doing so. This should be changed if the issue is resolved.
+      ...(option === 'notification-arns' ? {} : { default: generateDefault(options[option].type) }),
+      ...options[option],
+    };


yargs/yargs#2443 -> this issue prevents me from specifying and undefined default

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

Lines 374 to 378 in 61626dc

// Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK)

//

// - undefined => cdk ignores it, as if it wasn't supported (allows external management).

// - []: => cdk manages it, and the user wants to wipe it out.

// - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1'].

-> this is relevant for why notification-arns is a special case.

so we have notification-arns defaulting to undefined and other arrays defaulting to []. the yargs issue is why i cannot currently default to undefined, as yargs inexplicably turns that into [undefined].

aws-cdk-automation · 2024-12-24T21:52:04Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

HBobertz · 2024-12-26T15:51:53Z

tools/@aws-cdk/cli-args-gen/lib/cli-type-gen.ts

+    case 'string':
+    case 'number':
+    case 'object':
+      return JSON.stringify(defaultValue);


Evaluating nulls as 'object' still feels wrong to me but whatever I don't think it'd ever be a problem

mergify · 2024-12-26T15:52:28Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-12-26T16:23:08Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 7d2475e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-12-26T16:23:38Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-12-26T16:23:57Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

explicit default

1b6a563

kaizencc requested a review from a team as a code owner December 19, 2024 21:48

github-actions bot added the p2 label Dec 19, 2024

aws-cdk-automation requested a review from a team December 19, 2024 21:48

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 19, 2024

kaizencc commented Dec 19, 2024

View reviewed changes

tools/@aws-cdk/cli-args-gen/test/yargs-gen.test.ts Outdated Show resolved Hide resolved

kaizencc mentioned this pull request Dec 19, 2024

refactor(cli): docs, doctor, context commands do not use configuration directly anymore #32578

Merged

1 task

kaizencc requested a deployment to test-pipeline December 19, 2024 21:49 — with GitHub Actions Waiting

aws-cdk-automation previously requested changes Dec 19, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Dec 19, 2024

Merge branch 'main' into conroy/explicit-default

741676a

kaizencc temporarily deployed to test-pipeline December 20, 2024 14:30 — with GitHub Actions Inactive

formatting

ec4358c

kaizencc requested a deployment to test-pipeline December 20, 2024 18:39 — with GitHub Actions Waiting

fix defaults

2a6c637

kaizencc requested a deployment to test-pipeline December 23, 2024 20:53 — with GitHub Actions Waiting

remove prints

dc6f1f9

kaizencc requested a deployment to test-pipeline December 23, 2024 20:56 — with GitHub Actions Waiting

fix cli-args-gen defaults

66cfea0

kaizencc changed the title ~~chore(cli): add explicit defaults to yargs definition~~ chore(cli): explicit defaults to yargs definition and cli arguments Dec 23, 2024

kaizencc requested a deployment to test-pipeline December 23, 2024 21:04 — with GitHub Actions Waiting

refactor

339abe1

kaizencc requested a deployment to test-pipeline December 23, 2024 21:14 — with GitHub Actions Waiting

Merge branch 'main' into conroy/explicit-default

8aaa26d

kaizencc temporarily deployed to test-pipeline December 23, 2024 21:16 — with GitHub Actions Inactive

HBobertz requested changes Dec 24, 2024

View reviewed changes

mrgrain reviewed Dec 24, 2024

View reviewed changes

kaizencc added 2 commits December 24, 2024 15:33

finally the right config

7deb217

special cli-argument too

499f3da

update cli-type-gen

1c6d25a

kaizencc requested a deployment to test-pipeline December 24, 2024 21:39 — with GitHub Actions Waiting

Merge branch 'main' into conroy/explicit-default

b643089

kaizencc requested a deployment to test-pipeline December 24, 2024 21:40 — with GitHub Actions Waiting

spacing

bb976ee

kaizencc commented Dec 24, 2024

View reviewed changes

spacing

2702326

kaizencc temporarily deployed to test-pipeline December 24, 2024 21:49 — with GitHub Actions Inactive

kaizencc added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Dec 25, 2024

kaizencc requested a review from HBobertz December 25, 2024 00:51

HBobertz reviewed Dec 26, 2024

View reviewed changes

HBobertz approved these changes Dec 26, 2024

View reviewed changes

Merge branch 'main' into conroy/explicit-default

7d2475e

mergify bot requested a deployment to test-pipeline December 26, 2024 15:53 Waiting

mergify bot merged commit 0ce42a5 into main Dec 26, 2024
16 of 17 checks passed

mergify bot deleted the conroy/explicit-default branch December 26, 2024 16:23

github-actions bot locked as resolved and limited conversation to collaborators Dec 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): explicit defaults to yargs definition and cli arguments #32596

chore(cli): explicit defaults to yargs definition and cli arguments #32596

kaizencc commented Dec 19, 2024 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Dec 19, 2024 •

edited

Loading

HBobertz Dec 24, 2024

kaizencc Dec 24, 2024

HBobertz Dec 26, 2024

HBobertz Dec 26, 2024

HBobertz Dec 24, 2024

kaizencc Dec 24, 2024

HBobertz Dec 24, 2024

mrgrain Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024

kaizencc Dec 24, 2024 •

edited

Loading

aws-cdk-automation commented Dec 24, 2024

HBobertz Dec 26, 2024

mergify bot commented Dec 26, 2024

aws-cdk-automation commented Dec 26, 2024

mergify bot commented Dec 26, 2024

github-actions bot commented Dec 26, 2024

	// Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK)
	//
	// - undefined => cdk ignores it, as if it wasn't supported (allows external management).
	// - []: => cdk manages it, and the user wants to wipe it out.
	// - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1'].

chore(cli): explicit defaults to yargs definition and cli arguments #32596

chore(cli): explicit defaults to yargs definition and cli arguments #32596

Conversation

kaizencc commented Dec 19, 2024 • edited Loading

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaizencc Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 24, 2024

Choose a reason for hiding this comment

mergify bot commented Dec 26, 2024

aws-cdk-automation commented Dec 26, 2024

AWS CodeBuild CI Report

mergify bot commented Dec 26, 2024

github-actions bot commented Dec 26, 2024

kaizencc commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

kaizencc Dec 24, 2024 •

edited

Loading