fix(cli): error message for destroy action on non-existent stack #30374

slxsh · 2024-05-29T17:31:19Z

Reason for this change

Destroy command reports success even if a stack doesn't exist.

Description of changes

While running destroy command if a stack is not deployed then an error message will be displayed but the operation won't be interrupted.

In deploy-stack.ts added an exception if the stack doesn't exist.
In cdk-toolkit.ts added error message if the above exception is thrown.

Description of how you validated changes

I need some help with testing. I couldn't find a way to test the changes using real stacks/cli. Also, not sure if I need to add integration test too (didn't see any existing relevant test where I could make some changes).

Have added unit tests.

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

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.

slxsh · 2024-05-30T17:38:29Z

Clarification Request - regarding integration tests. Also, change in CLI code.

aws-cdk-automation · 2024-05-30T18:08:16Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2024-06-20T00:09:55Z

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.

comcalvi

You will need a CLI integration test for this feature. The CLI integration tests all live here: https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk-testing/cli-integ. The readme provides guidance in authoring and running them locally.

comcalvi · 2024-06-20T16:36:46Z

packages/aws-cdk/lib/api/deploy-stack.ts

-    return;
+    throw new StackNotFoundError(`Failed to destroy ${deployName}. The stack is not deployed.`);


This is a breaking change, because we are changing the exit code for this command. We will not accept breaking changes to any CLI commands. Instead please add a new flag, something like --fail-on-missing, to destroy, and in this if ensure we check both currentStack.exists and options.failOnMissing, or a different name if you think a different one would be better.

Would this be considered a breaking change even though the exception is being caught? The exit code of destroy command is not changed. The command doesn't fail if the stack doesn't exist.

The issue states destroy returns success even if the stack doesn't exist, if we add the flag this problem will persist with the destroy command.

comcalvi · 2024-06-20T16:37:36Z

packages/aws-cdk/lib/api/deploy-stack.ts

+
+export class StackNotFoundError extends Error {
+  constructor(message: string) {
+    super(message);
+  }
+}


We don't need this error type here, please use throw new Error('message');

Sure. Any reason why you feel this is unnecessary? Saw some custom errors in the code. Thought it would be make it clear which exception is being thrown.

comcalvi · 2024-06-20T16:39:21Z

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

@@ -620,8 +620,12 @@ export class CdkToolkit {
        });
        success(`\n ✅  %s: ${action}ed`, chalk.blue(stack.displayName));
      } catch (e) {
-        error(`\n ❌  %s: ${action} failed`, chalk.blue(stack.displayName), e);
-        throw e;
+        if (e instanceof StackNotFoundError) {


Don't use instanceof, instead check with e.message.includes().

comcalvi · 2024-06-20T16:39:35Z

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

@@ -1525,4 +1529,4 @@ function buildParameterMap(parameters: {
  }

  return parameterMap;
-}
+}


Please keep the newline at the end of the file.

aws-cdk-automation · 2024-06-20T18:22:36Z

➡️ 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.

aws-cdk-automation · 2024-07-12T00:09:47Z

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 · 2024-07-20T00:09:41Z

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.

github-actions · 2024-07-20T00:09:59Z

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.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

aws-cdk-automation · 2024-07-20T00:11:34Z

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

Error message for destroy action on non-existent stack

262c58a

aws-cdk-automation requested a review from a team May 29, 2024 17:31

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels May 29, 2024

Merge branch 'main' into destroy-skip-fix

51d73ed

slxsh had a problem deploying to test-pipeline May 29, 2024 17:32 — with GitHub Actions Failure

aws-cdk-automation requested changes May 29, 2024

View reviewed changes

slxsh changed the title ~~fix(cli): Error message for destroy action on non-existent stack~~ fix(cli): error message for destroy action on non-existent stack May 29, 2024

Merge branch 'main' into destroy-skip-fix

4d1c81e

slxsh had a problem deploying to test-pipeline May 29, 2024 18:36 — with GitHub Actions Failure

Merge branch 'main' into destroy-skip-fix

bc1db8f

slxsh temporarily deployed to test-pipeline May 30, 2024 17:39 — with GitHub Actions Inactive

This was referenced Jun 1, 2024

Monthly PR metrics report #30414

Closed

Weekly PR metrics report #30434

Closed

comcalvi requested changes Jun 20, 2024

View reviewed changes

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

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

aws-cdk-automation closed this Jul 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): error message for destroy action on non-existent stack #30374

fix(cli): error message for destroy action on non-existent stack #30374

slxsh commented May 29, 2024

aws-cdk-automation left a comment

slxsh commented May 30, 2024

aws-cdk-automation commented May 30, 2024

aws-cdk-automation commented Jun 20, 2024

comcalvi left a comment

comcalvi Jun 20, 2024 •

edited

Loading

slxsh Jul 6, 2024

comcalvi Jun 20, 2024

slxsh Jul 6, 2024

comcalvi Jun 20, 2024

comcalvi Jun 20, 2024

aws-cdk-automation commented Jun 20, 2024

aws-cdk-automation commented Jul 12, 2024

aws-cdk-automation commented Jul 20, 2024

github-actions bot commented Jul 20, 2024

aws-cdk-automation commented Jul 20, 2024

		return;
		throw new StackNotFoundError(`Failed to destroy ${deployName}. The stack is not deployed.`);

fix(cli): error message for destroy action on non-existent stack #30374

fix(cli): error message for destroy action on non-existent stack #30374

Conversation

slxsh commented May 29, 2024

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

slxsh commented May 30, 2024

aws-cdk-automation commented May 30, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 20, 2024

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

slxsh Jul 6, 2024

Choose a reason for hiding this comment

comcalvi Jun 20, 2024

Choose a reason for hiding this comment

slxsh Jul 6, 2024

Choose a reason for hiding this comment

comcalvi Jun 20, 2024

Choose a reason for hiding this comment

comcalvi Jun 20, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 20, 2024

aws-cdk-automation commented Jul 12, 2024

aws-cdk-automation commented Jul 20, 2024

github-actions bot commented Jul 20, 2024

aws-cdk-automation commented Jul 20, 2024

comcalvi Jun 20, 2024 •

edited

Loading