feat: cdk diagnose displays the root cause of a past stack deployment failure#1378
feat: cdk diagnose displays the root cause of a past stack deployment failure#1378
cdk diagnose displays the root cause of a past stack deployment failure#1378Conversation
…nt failure Useful when you use a CI/CD system to deploy stacks, but the failure reason is hard to parse. This PR unifies makes the error analysis and printing paths of `cdk deploy` accessible as a separate subcommand, `cdk diagnose`. This allows you to get the same interpretation of CloudFormation error reporting strategies, as well as source localization for a CI/CD deployment as is possible today for a deployment performed directly via the CLI. In the future, we will further extend the diagnosis that the CLI performs to get you more information. This PR extends the bootstrap template permissions with `List` permissions for the `ChangeSets`, turns them into `List*` and `Describe*` so that if there are future read-only operations we don't have to keep adding them. It also turns on `IncludeNestedStack: true` for CreateChangeSet so that early validation is run for nested stacks.
67b63bc to
6fdb79a
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1378 +/- ##
==========================================
- Coverage 88.14% 87.93% -0.21%
==========================================
Files 74 74
Lines 10481 10530 +49
Branches 1432 1435 +3
==========================================
+ Hits 9238 9260 +22
- Misses 1216 1243 +27
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Total lines changed 2870 is greater than 1000. Please consider breaking this PR down. |
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…to huijbers/cdk-diagnose
| }); | ||
|
|
||
| expect(stdErr).toContain('Import of existing resources failed'); | ||
| expect(stdErr).toContain('needs a DeletionPolicy'); |
There was a problem hiding this comment.
shrug. The message changed slightly. This is enough to confirm that the message is in the output.
mrgrain
left a comment
There was a problem hiding this comment.
Mostly minor stuff.
Bigger points for for me are:
- Should the new command be
--unstablefor now? - The naming and jsdocs around
OldestEventdon't click with me at all. Would love to see an iteration on the API to make the intent more clear.
| /** | ||
| * Optionally a source trace | ||
| * | ||
| * (Not optional on purpose so we are not allowed to forget to call the code that should fill it) |
There was a problem hiding this comment.
Nice! Although this feels like a linter rules/TS config trying to come out 😱
There was a problem hiding this comment.
I used to have branded types here but I also didn't want to expose those publicly, so this is the middle ground.
| /* c8 ignore stop */ | ||
|
|
||
| type TypeUnderlyingBrand<A> = A extends Branded<infer T, any> ? T : never; | ||
| type TypeUnderlyingBrand<A> = Omit<A, keyof Brand<any>>; |
There was a problem hiding this comment.
Seems risky to go from never to any... Can you help explain this?
There was a problem hiding this comment.
Sure.
The goal is, given Branded<A>, to find A. For whatever reason, the A extends Branded<infer T> never worked. I forget what it resolved to, but it was either any or unknown.
Since Branded<A> does A & Something, we can also try to remove the intersection. Googling told me that the way to de-intersect a type is to do Omit<A, keyof Something>. In our case the Something is a Brand<B>, but we don't care about the B because the keys are always the same, so B=any.
TL;DR: the never and any have nothing to do with each other, they just both happened to occur in 2 type manipulation signatures trying to do the same thing in a different way.
| // Some custom resource types that the CDK standard library creates that we | ||
| // would like to see it if they fail. |
There was a problem hiding this comment.
thought (non-blocking): What a shame we don't have a consistent naming pattern for these 🤦🏻
| shouldStop(event: ResourceEvent): 'stop-include' | 'stop-exclude' | 'continue'; | ||
| } | ||
|
|
||
| export abstract class OldestEvent { |
There was a problem hiding this comment.
issue: The naming of this doesn't click with me.
| public static timestamp(startTime: number): IOldestEvent { | ||
| return { | ||
| shouldStop(event) { | ||
| return event.event.Timestamp!.valueOf() < startTime ? 'stop-exclude' : 'continue'; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
fromTimestamp or withStartingPoint ?
OldestEvent.timestamp(...) just doesn't speak to me at all.
| after-the-fact. This can be useful to refresh your memory, ask a colleague to | ||
| help diagnose a deployment problem, or try to diagnose a deployment problem if | ||
| your way of working dictates that you perform stack deployment via a CI/CD | ||
| system (instead of directly using the CLI). |
There was a problem hiding this comment.
| system (instead of directly using the CLI). | |
| system instead of directly using the CDK CLI. |
Co-authored-by: Momo Kornher <kornherm@amazon.co.uk>
Useful when you use a CI/CD system to deploy stacks, but the failure reason is hard to parse.
This PR unifies and makes the error analysis and printing paths of
cdk deployaccessible as a separate subcommand,cdk diagnose. This allows you to get the same interpretation of CloudFormation error reporting strategies, as well as source localization for a CI/CD deployment as is possible today for a deployment performed directly via the CLI.In the future, we will further extend the diagnosis that the CLI performs to get you more information.
This is what errors look like now (both for
cdk deployandcdk diagnose):Broad design of this code:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license