fix(cli): cdk destroy does not throw error for a non-existent stack #27293

go-to-k · 2023-09-26T12:15:46Z

This PR for cli is to throw an error if stacks with wrong cases (=not exist) specified in cdk destroy.

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

This reverts commit 56293ef.

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.

go-to-k · 2023-09-26T12:31:47Z

Exemption Request: this should be covered with cli-integ tests. In my environment, the behavior was confirmed as expected.

SankyRed · 2023-10-11T21:48:22Z

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

-    // No validation
+    const notExistPatterns = selector.patterns.filter(p => !stacks.stackArtifacts.find(s => minimatch(s.hierarchicalId, p)));
+    if (notExistPatterns.length > 0) {
+      throw new Error(`Stacks not exist: ${notExistPatterns.join(', ')}`);


Can you please change the error message to be something more informative as to what does not exist and how they might be able to fix it?

@SankyRed

For example, if you specify stacks stack-A, stack-X, and stack-Y in cdk destroy, and only stack-A exists in your app.

In this code, the error message is to be following, Stacks not exist: stack-X, stack-Y. So you can know which of stacks do not exist.

how they might be able to fix it?

I think it is to difficult to know this. If you enter non-existent stacks, CDK does not know the way how to fix it unless it gets all the stacks and does a fuzzy search on all of them. Even if we did that, you probably wouldn't get the expected and accurate output.

A tad more context here might be helpful, if we can provide it. Maybe something like Cannot run cdk destroy on stack(s) ${stacks.join(', ')}. ${notExistPatterns.join(', ')} are not deployed in account: ${account}, region: ${region}.

@TheRealAmazonKendra

A tad more context here might be helpful, if we can provide it. Maybe something like Cannot run cdk destroy on stack(s) ${stacks.join(', ')}. ${notExistPatterns.join(', ')} are not deployed in account: ${account}, region: ${region}.

I think the format is difficult.

This notExistPattern handling and cdk destroy check the non-existence stacks by looking not the AWS account but stack instance definitions in the cdk app (that is cloud-assembly).

My understanding is that cdk destroy does not run the command by specifying an account or region, but looks at the stack list (CloudFormationStackArtifact[]) in cloud-assembly and if each stack has a region/account, it is used; otherwise, the default region/account is used. Actually there is no --region option in cdk destroy.

So we can see that the stack entered does not exist in the CDK app in the first place, but we cannot know WHICH account/region it does not exist in. (We may know that "the stack does not exist in the DEFAULT or ANY region and account", but perhaps that is not the information the user wants to know.)

Then, how about the following message?:

Cannot run cdk destroy on stack(s) ${stacks.join(', ')}. ${notExistPatterns.join(', ')} not exist.

SankyRed · 2023-10-11T21:53:13Z

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

@@ -769,7 +770,10 @@ export class CdkToolkit {
      defaultBehavior: DefaultSelection.OnlySingle,
    });

-    // No validation
+    const notExistPatterns = selector.patterns.filter(p => !stacks.stackArtifacts.find(s => minimatch(s.hierarchicalId, p)));


Could you also add code such that we error out before it we even try deleting any stack, if at least one of the stacks provided does not exist?

@SankyRed

In the current code, if any of the stacks are not existing, none of the stacks are deleted. And the cdk occurs an error.

Is this not what you are assuming?

TheRealAmazonKendra · 2023-10-12T06:44:40Z

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

-    // No validation
+    const notExistPatterns = selector.patterns.filter(p => !stacks.stackArtifacts.find(s => minimatch(s.hierarchicalId, p)));
+    if (notExistPatterns.length > 0) {
+      throw new Error(`Stacks not exist: ${notExistPatterns.join(', ')}`);


A tad more context here might be helpful, if we can provide it. Maybe something like Cannot run cdk destroy on stack(s) ${stacks.join(', ')}. ${notExistPatterns.join(', ')} are not deployed in account: ${account}, region: ${region}.

TheRealAmazonKendra · 2023-10-12T06:45:35Z

Exemption Request: this should be covered with cli-integ tests. In my environment, the behavior was confirmed as expected.

I would like to see this added as a failure case in the cli integ tests. Could you please add a test case there?

go-to-k · 2023-10-12T08:29:55Z

@TheRealAmazonKendra

I added cli integ tests and changed an error message to preliminary one for now.

This is my first time doing cli integ tests, so please let me know if I am missing something.

3dbc266

aws-cdk-automation · 2023-10-20T00:08:19Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2023-10-20T06:10:45Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/27293/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation · 2023-10-20T06:38:45Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2023-10-22T15:38:09Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/27293/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

go-to-k · 2023-11-02T09:39:28Z

@TheRealAmazonKendra

Could you please review my changes and answers?

#27293 (comment)

aws-cdk-automation · 2023-11-03T00:08:19Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2023-11-10T00:21:42Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation · 2023-11-10T00:23:45Z

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/27293/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

mergify · 2023-11-10T03:34:49Z

⚠️ The sha of the head commit of this PR conflicts with #27921. Mergify cannot evaluate rules on this PR. ⚠️

go-to-k added 5 commits September 24, 2023 01:42

fix(cli): No exception when stack with wrong cases is deployed

56293ef

Revert "fix(cli): No exception when stack with wrong cases is deployed"

bec1d03

This reverts commit 56293ef.

add a test

16b0bab

using matchingPattern

c3963b1

update for selectStacksForDestroy

85284b2

aws-cdk-automation requested a review from a team September 26, 2023 12:16

github-actions bot added p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Sep 26, 2023

go-to-k marked this pull request as draft September 26, 2023 12:16

aws-cdk-automation requested changes Sep 26, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Sep 26, 2023

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort labels Sep 26, 2023

go-to-k marked this pull request as ready for review September 26, 2023 12:22

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 26, 2023

go-to-k marked this pull request as draft September 26, 2023 12:37

fix order of import

6563a5f

go-to-k marked this pull request as ready for review September 27, 2023 03:15

go-to-k changed the title ~~fix(cli): No exception when stack with wrong cases is deployed~~ fix(cli): No exception when a non-existent stack is specified in cdk destroy Sep 27, 2023

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Sep 27, 2023

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

SankyRed self-assigned this Oct 11, 2023

vinayak-kukreja changed the title ~~fix(cli): No exception when a non-existent stack is specified in cdk destroy~~ fix(cli): cdk destroy does not throw error for a non-existent stack Oct 11, 2023

SankyRed reviewed Oct 11, 2023

View reviewed changes

search for v1

27a39fc

go-to-k had a problem deploying to test-pipeline October 12, 2023 05:31 — with GitHub Actions Failure

change import order

6b4dbe2

go-to-k had a problem deploying to test-pipeline October 12, 2023 05:39 — with GitHub Actions Failure

TheRealAmazonKendra requested changes Oct 12, 2023

View reviewed changes

TheRealAmazonKendra removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 12, 2023

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 12, 2023

cli-integ-tests

3dbc266

go-to-k had a problem deploying to test-pipeline October 12, 2023 08:26 — with GitHub Actions Failure

Merge branch 'main' into fix/destroy-not-exits

b16eb1f

go-to-k had a problem deploying to test-pipeline October 20, 2023 06:09 — with GitHub Actions Failure

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 10, 2023

aws-cdk-automation closed this Nov 10, 2023

go-to-k had a problem deploying to test-pipeline November 10, 2023 03:35 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): cdk destroy does not throw error for a non-existent stack #27293

fix(cli): cdk destroy does not throw error for a non-existent stack #27293

go-to-k commented Sep 26, 2023 •

edited

aws-cdk-automation left a comment

go-to-k commented Sep 26, 2023

SankyRed Oct 11, 2023

go-to-k Oct 12, 2023 •

edited

TheRealAmazonKendra Oct 12, 2023

go-to-k Oct 22, 2023 •

edited

SankyRed Oct 11, 2023

go-to-k Oct 12, 2023

TheRealAmazonKendra Oct 12, 2023

TheRealAmazonKendra commented Oct 12, 2023

go-to-k commented Oct 12, 2023 •

edited

aws-cdk-automation commented Oct 20, 2023

aws-cdk-automation commented Oct 20, 2023

aws-cdk-automation commented Oct 20, 2023

aws-cdk-automation commented Oct 22, 2023

go-to-k commented Nov 2, 2023 •

edited

aws-cdk-automation commented Nov 3, 2023

aws-cdk-automation commented Nov 10, 2023

aws-cdk-automation commented Nov 10, 2023

mergify bot commented Nov 10, 2023

fix(cli): cdk destroy does not throw error for a non-existent stack #27293

fix(cli): cdk destroy does not throw error for a non-existent stack #27293

Conversation

go-to-k commented Sep 26, 2023 • edited

aws-cdk-automation left a comment

Choose a reason for hiding this comment

go-to-k commented Sep 26, 2023

SankyRed Oct 11, 2023

Choose a reason for hiding this comment

go-to-k Oct 12, 2023 • edited

Choose a reason for hiding this comment

TheRealAmazonKendra Oct 12, 2023

Choose a reason for hiding this comment

go-to-k Oct 22, 2023 • edited

Choose a reason for hiding this comment

SankyRed Oct 11, 2023

Choose a reason for hiding this comment

go-to-k Oct 12, 2023

Choose a reason for hiding this comment

TheRealAmazonKendra Oct 12, 2023

Choose a reason for hiding this comment

TheRealAmazonKendra commented Oct 12, 2023

go-to-k commented Oct 12, 2023 • edited

aws-cdk-automation commented Oct 20, 2023

aws-cdk-automation commented Oct 20, 2023

aws-cdk-automation commented Oct 20, 2023

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 22, 2023

go-to-k commented Nov 2, 2023 • edited

aws-cdk-automation commented Nov 3, 2023

aws-cdk-automation commented Nov 10, 2023

aws-cdk-automation commented Nov 10, 2023

mergify bot commented Nov 10, 2023

go-to-k commented Sep 26, 2023 •

edited

go-to-k Oct 12, 2023 •

edited

go-to-k Oct 22, 2023 •

edited

go-to-k commented Oct 12, 2023 •

edited

go-to-k commented Nov 2, 2023 •

edited