chore(cli): explicit defaults to yargs definition and cli arguments by kaizencc · Pull Request #32596 · aws/aws-cdk

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

 // 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

    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

   * 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

   * 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

   * 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

   * 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

   * 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 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 had a problem deploying to test-pipeline December 19, 2024 21:49 — with GitHub Actions Failure

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 had a problem deploying to test-pipeline December 20, 2024 18:39 — with GitHub Actions Failure

fix defaults

2a6c637

kaizencc had a problem deploying to test-pipeline December 23, 2024 20:53 — with GitHub Actions Failure

remove prints

dc6f1f9

kaizencc had a problem deploying to test-pipeline December 23, 2024 20:56 — with GitHub Actions Failure

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 had a problem deploying to test-pipeline December 23, 2024 21:04 — with GitHub Actions Failure

refactor

339abe1

kaizencc had a problem deploying to test-pipeline December 23, 2024 21:14 — with GitHub Actions Failure

Merge branch 'main' into conroy/explicit-default

8aaa26d

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

HBobertz suggested 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 had a problem deploying to test-pipeline December 24, 2024 21:39 — with GitHub Actions Failure

Merge branch 'main' into conroy/explicit-default

b643089

kaizencc had a problem deploying to test-pipeline December 24, 2024 21:40 — with GitHub Actions Failure

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 had a problem deploying to test-pipeline December 26, 2024 15:53 Failure

mergify bot merged commit 0ce42a5 into main Dec 26, 2024

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

	// 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'].

Conversation

kaizencc commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Dec 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 26, 2024

Uh oh!

aws-cdk-automation commented Dec 26, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 26, 2024

Uh oh!

github-actions bot commented Dec 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaizencc commented Dec 19, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

kaizencc Dec 24, 2024 •

edited

Loading