-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cli): cdk destroy does not throw error for a non-existent stack #27293
Changes from 6 commits
56293ef
bec1d03
16b0bab
c3963b1
85284b2
6563a5f
27a39fc
6b4dbe2
3dbc266
b16eb1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import * as cxapi from '@aws-cdk/cx-api'; | |
import * as chalk from 'chalk'; | ||
import * as chokidar from 'chokidar'; | ||
import * as fs from 'fs-extra'; | ||
import { minimatch } from 'minimatch'; | ||
import * as promptly from 'promptly'; | ||
import { DeploymentMethod } from './api'; | ||
import { SdkProvider } from './api/aws-auth'; | ||
|
@@ -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))); | ||
if (notExistPatterns.length > 0) { | ||
throw new Error(`Stacks not exist: ${notExistPatterns.join(', ')}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, if you specify stacks In this code, the error message is to be following,
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A tad more context here might be helpful, if we can provide it. Maybe something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the format is difficult. This My understanding is that 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?:
|
||
} | ||
|
||
return stacks; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?