chore(cli): use typed errors `ToolkitError` and `AuthenticationError` in CLI by sumupitchayan · Pull Request #32548 · aws/aws-cdk

sumupitchayan · 2024-12-16T20:19:29Z

Closes #32347

This PR creates two new error types, ToolkitError and AuthenticationError and uses them in aws-cdk.

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

Signed-off-by: Sumu <sumughan@amazon.com>

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.

Signed-off-by: Sumu <sumughan@amazon.com>

…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit

Signed-off-by: Sumu <sumughan@amazon.com>

codecov · 2024-12-17T00:18:01Z

Codecov Report

Attention: Patch coverage is 54.16667% with 66 lines in your changes missing coverage. Please review.

Project coverage is 80.64%. Comparing base (5735e9e) to head (ee8204b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32548      +/-   ##
==========================================
+ Coverage   80.54%   80.64%   +0.10%     
==========================================
  Files         106      107       +1     
  Lines        6954     6996      +42     
  Branches     1287     1290       +3     
==========================================
+ Hits         5601     5642      +41     
  Misses       1175     1175              
- Partials      178      179       +1

Flag	Coverage Δ
suite.unit	`80.64% <54.16%> (+0.10%)`	⬆️

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

Components	Coverage Δ
packages/aws-cdk	`80.64% <54.16%> (+0.10%)`	⬆️

Signed-off-by: Sumu <sumughan@amazon.com>

…onError func on the ToolkitError class Signed-off-by: Sumu <sumughan@amazon.com>

Signed-off-by: Sumu <sumughan@amazon.com>

…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit

Signed-off-by: Sumu <sumughan@amazon.com>

kaizencc

Apologies if some of these questions were already asked/responded to by momo.

Also please configure the eslint rule for this package only to enforce we don't accidentally add new untyped errors.

I don't see where you've resolved this request

kaizencc · 2024-12-23T17:25:36Z

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

+  /**
+   * The type of the error, defaults to "toolkit".
+   */
+  public readonly type: string;


is this being used anywhere other than tests?

Nope (not yet at least) - I am also copying the implementation from ConstructError again here:

aws-cdk/packages/aws-cdk-lib/core/lib/errors.ts

Line 56 in 8718767

public abstract get type(): string;

Adding a type property is also the third instruction on Momo's ticket: #32347

again i wish you had a real reason beyond Momo telling you to. but this is fine, i suppose

kaizencc · 2024-12-23T17:26:12Z

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

+   */
+  public readonly type: string;
+
+  constructor(message: string, type: string = 'toolkit') {


should we restrict type to either toolkit or authentication? what was the decision making behind typing it as string?

also, should the constructor be private?

Both ConstructError and ValidationError have public constructors:

aws-cdk/packages/aws-cdk-lib/core/lib/errors.ts

Line 140 in 8718767

constructor(msg: string, scope: IConstruct) {

I could make it an enum with string values? But I'm pretty sure Momo wanted a type property that is a string (third bullet point here): #32347

for all of these questions its my hope that you answer them with your reasons for why you've done something, not because Momo said to or because a prior implementation did so. Both of those things can be true, but you should at least know why you're doing it.

now for this case my question was a silly one. you have a public constructor because you are calling new ToolkitError everywhere

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

stale, and i am the captain now

sumupitchayan · 2024-12-23T18:45:54Z

Also please configure the eslint rule for this package only to enforce we don't accidentally add new untyped errors.

I don't see where you've resolved this request

cdklabs/eslint-rules#74

kaizencc

I've approved. I will leave it to you to ensure that the CLI integ tests pass + to override the codecov failures

aws-cdk-automation · 2024-12-24T14:34:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: ee8204b
Result: FAILED
Build Logs (available for 30 days)

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

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

github-actions · 2024-12-24T20:18:28Z

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.

sumupitchayan added 8 commits December 15, 2024 12:46

WIP - initial toolkit error work

96ae9a3

Signed-off-by: Sumu <sumughan@amazon.com>

WIP - use toolkit errors for standalone files at aws-cdk/lib

f720602

Signed-off-by: Sumu <sumughan@amazon.com>

WIP - replace in aws-cdk/lib/api/aws-auth/

bc9cc21

Signed-off-by: Sumu <sumughan@amazon.com>

WIP: aws-cdk/lib/api/bootstrap/

a4f0d75

Signed-off-by: Sumu <sumughan@amazon.com>

WIP: aws-cdk/lib/api/cxapp/

e47623f

Signed-off-by: Sumu <sumughan@amazon.com>

WIP: aws-cdk/lib/api/garbage-collection/

6b4820e

Signed-off-by: Sumu <sumughan@amazon.com>

WIP: aws-cdk/lib/api/hotswap/

7a84bc1

Signed-off-by: Sumu <sumughan@amazon.com>

WIP: aws-cdk/lib/api/plugin/

9d8603b

Signed-off-by: Sumu <sumughan@amazon.com>

aws-cdk-automation requested a review from a team December 16, 2024 20:19

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

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

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 20:20 — with GitHub Actions Failure

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

View reviewed changes

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

Merge branch 'main' into sumughan/typed-errors-cli-toolkit

9114280

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 20:25 — with GitHub Actions Failure

sumupitchayan added 2 commits December 16, 2024 16:22

WIP - fix build

7e071d5

Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'sumughan/typed-errors-cli-toolkit' of https://github.co…

7b75daa

…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 21:38 — with GitHub Actions Failure

WIP: fix build x2

ec4d56a

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan marked this pull request as ready for review December 16, 2024 23:22

sumupitchayan requested a review from a team as a code owner December 16, 2024 23:22

sumupitchayan changed the title ~~chore(cli): use typed ToolkitError and AuthenticationError in CLI~~ chore(cli): use typed errors ToolkitError and AuthenticationError in CLI Dec 16, 2024

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 23:22 — with GitHub Actions Failure

WIP: fix build x3

ac81f5c

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 23:34 — with GitHub Actions Failure

WIP: imports ordering

2452ccf

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 16, 2024 23:58 — with GitHub Actions Failure

Merge branch 'main' into sumughan/typed-errors-cli-toolkit

ad88949

WIP: fix import error on build

7054ea2

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 19, 2024 15:55 — with GitHub Actions Failure

WIP: momo suggestion, use CDK symbol pattern and write isAuthenticati…

68fb1db

…onError func on the ToolkitError class Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 19, 2024 16:07 — with GitHub Actions Failure

WIP: fix build

262d8c4

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 19, 2024 16:17 — with GitHub Actions Failure

Merge branch 'main' into sumughan/typed-errors-cli-toolkit

dc7bf3d

sumupitchayan had a problem deploying to test-pipeline December 19, 2024 17:33 — with GitHub Actions Failure

sumupitchayan added 2 commits December 19, 2024 14:11

momo comment: error message in appsync file

9dd78e4

Signed-off-by: Sumu <sumughan@amazon.com>

Merge branch 'sumughan/typed-errors-cli-toolkit' of https://github.co…

990434d

…m/aws/aws-cdk into sumughan/typed-errors-cli-toolkit

sumupitchayan had a problem deploying to test-pipeline December 19, 2024 19:12 — with GitHub Actions Failure

Merge branch 'main' into sumughan/typed-errors-cli-toolkit

7d1209c

sumupitchayan had a problem deploying to test-pipeline December 20, 2024 14:29 — with GitHub Actions Failure

WIP: fix build

8b87dc2

Signed-off-by: Sumu <sumughan@amazon.com>

sumupitchayan had a problem deploying to test-pipeline December 20, 2024 14:31 — with GitHub Actions Failure

kaizencc requested changes Dec 23, 2024

View reviewed changes

sumupitchayan requested a review from kaizencc December 23, 2024 19:04

kaizencc approved these changes Dec 23, 2024

View reviewed changes

Merge branch 'main' into sumughan/typed-errors-cli-toolkit

ee8204b

sumupitchayan had a problem deploying to test-pipeline December 24, 2024 14:20 — with GitHub Actions Failure

sumupitchayan 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 24, 2024

sumupitchayan merged commit d48d77a into main Dec 24, 2024

sumupitchayan deleted the sumughan/typed-errors-cli-toolkit branch December 24, 2024 20:18

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

Conversation

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

Uh oh!

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

sumupitchayan Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

sumupitchayan Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

kaizencc Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sumupitchayan commented Dec 23, 2024

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Dec 24, 2024

AWS CodeBuild CI Report

Uh oh!

github-actions bot commented Dec 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sumupitchayan commented Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading