-
Notifications
You must be signed in to change notification settings - Fork 69
feat(cli): count resources/stacks with UNKNOWN drift status as unchecked
#747
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
Changes from all commits
f1665f9
fba9bf5
f4774b6
77a45ec
6ec14fe
578ff50
febc8c5
666ad44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,7 @@ interface DriftFormatterOutput { | |
| readonly unchanged?: string; | ||
|
|
||
| /** | ||
| * Resources that were not checked for drift | ||
| * Resources that were not checked for drift or have an UNKNOWN drift status | ||
| */ | ||
| readonly unchecked?: string; | ||
|
|
||
|
|
@@ -98,12 +98,10 @@ export class DriftFormatter { | |
| public formatStackDrift(): DriftFormatterOutput { | ||
| const formatterOutput = this.formatStackDriftChanges(this.buildLogicalToPathMap()); | ||
|
|
||
| // we are only interested in actual drifts and always ignore the metadata resource | ||
| // we are only interested in actual drifts (and ignore the metadata resource) | ||
| const actualDrifts = this.resourceDriftResults.filter(d => | ||
| d.StackResourceDriftStatus === 'MODIFIED' || | ||
| d.StackResourceDriftStatus === 'DELETED' || | ||
| d.ResourceType === 'AWS::CDK::Metadata', | ||
| ); | ||
| (d.StackResourceDriftStatus === 'MODIFIED' || d.StackResourceDriftStatus === 'DELETED') | ||
| && d.ResourceType !== 'AWS::CDK::Metadata'); | ||
|
|
||
| // must output the stack name if there are drifts | ||
| const stackHeader = format(`Stack ${chalk.bold(this.stackName)}\n`); | ||
|
|
@@ -114,6 +112,7 @@ export class DriftFormatter { | |
| numResourcesWithDrift: 0, | ||
| numResourcesUnchecked: this.allStackResources.size - this.resourceDriftResults.length, | ||
| stackHeader, | ||
| unchecked: formatterOutput.unchecked, | ||
|
Contributor
Author
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. Include unchecked resources even when the count of drifted resources is 0, for the user's awareness. |
||
| summary: finalResult, | ||
| }; | ||
| } | ||
|
|
@@ -140,11 +139,8 @@ export class DriftFormatter { | |
| } | ||
|
|
||
| /** | ||
| * Renders stack drift information to the given stream | ||
| * Renders stack drift information | ||
| * | ||
| * @param driftResults - The stack resource drifts from CloudFormation | ||
| * @param allStackResources - A map of all stack resources | ||
| * @param verbose - Whether to output more verbose text (include undrifted resources) | ||
|
Comment on lines
-143
to
-147
Contributor
Author
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. Removed params that are not present in the function signature. |
||
| * @param logicalToPathMap - A map from logical ID to construct path | ||
| */ | ||
| private formatStackDriftChanges( | ||
|
|
@@ -167,35 +163,35 @@ export class DriftFormatter { | |
|
|
||
| for (const drift of unchangedResources) { | ||
| if (!drift.LogicalResourceId || !drift.ResourceType) continue; | ||
| unchanged += `${CONTEXT} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| unchanged += `${CONTEXT} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| } | ||
| unchanged += this.printSectionFooter(); | ||
| } | ||
|
|
||
| // Process all unchecked resources | ||
| if (this.allStackResources) { | ||
|
Contributor
Author
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. This is a JS Map, so always truthy. Removed.
Contributor
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. Nice! |
||
| const uncheckedResources = Array.from(this.allStackResources.keys()).filter((logicalId) => { | ||
| return !drifts.find((drift) => drift.LogicalResourceId === logicalId); | ||
| }); | ||
| if (uncheckedResources.length > 0) { | ||
| unchecked = this.printSectionHeader('Unchecked Resources'); | ||
| for (const logicalId of uncheckedResources) { | ||
| const resourceType = this.allStackResources.get(logicalId); | ||
| unchecked += `${CONTEXT} ${this.formatValue(resourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, logicalId)}\n`; | ||
| } | ||
| unchecked += this.printSectionFooter(); | ||
| // Process all unchecked and unknown resources | ||
| const uncheckedResources = Array.from(this.allStackResources.keys()).filter((logicalId) => { | ||
| const drift = drifts.find((d) => d.LogicalResourceId === logicalId); | ||
| return !drift || drift.StackResourceDriftStatus === StackResourceDriftStatus.UNKNOWN; | ||
| }); | ||
| if (uncheckedResources.length > 0) { | ||
| unchecked = this.printSectionHeader('Unchecked Resources'); | ||
| for (const logicalId of uncheckedResources) { | ||
| const resourceType = this.allStackResources.get(logicalId); | ||
| unchecked += `${CONTEXT} ${chalk.cyan(resourceType)} ${this.formatLogicalId(logicalToPathMap, logicalId)}\n`; | ||
| } | ||
| unchecked += this.printSectionFooter(); | ||
| } | ||
|
|
||
| // Process modified resources | ||
| const modifiedResources = drifts.filter(d => d.StackResourceDriftStatus === StackResourceDriftStatus.MODIFIED); | ||
| // Process modified resources (exclude AWS::CDK::Metadata) | ||
| const modifiedResources = drifts.filter(d => | ||
| d.StackResourceDriftStatus === StackResourceDriftStatus.MODIFIED | ||
| && d.ResourceType !== 'AWS::CDK::Metadata'); | ||
|
Contributor
Author
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. Ensure the count is correct, by excluding metadata resources here. |
||
| if (modifiedResources.length > 0) { | ||
| modified = this.printSectionHeader('Modified Resources'); | ||
|
|
||
| for (const drift of modifiedResources) { | ||
| if (!drift.LogicalResourceId || !drift.ResourceType) continue; | ||
| if (modified === undefined) modified = ''; | ||
|
Contributor
Author
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. Dead code. |
||
| modified += `${UPDATE} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| modified += `${UPDATE} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| if (drift.PropertyDifferences) { | ||
| const propDiffs = drift.PropertyDifferences; | ||
| for (let i = 0; i < propDiffs.length; i++) { | ||
|
|
@@ -209,13 +205,15 @@ export class DriftFormatter { | |
| modified += this.printSectionFooter(); | ||
| } | ||
|
|
||
| // Process deleted resources | ||
| const deletedResources = drifts.filter(d => d.StackResourceDriftStatus === StackResourceDriftStatus.DELETED); | ||
| // Process deleted resources (exclude AWS::CDK::Metadata) | ||
| const deletedResources = drifts.filter(d => | ||
| d.StackResourceDriftStatus === StackResourceDriftStatus.DELETED | ||
| && d.ResourceType !== 'AWS::CDK::Metadata'); | ||
|
Contributor
Author
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. Ensure the count is correct, by excluding metadata resources here. |
||
| if (deletedResources.length > 0) { | ||
| deleted = this.printSectionHeader('Deleted Resources'); | ||
| for (const drift of deletedResources) { | ||
| if (!drift.LogicalResourceId || !drift.ResourceType) continue; | ||
| deleted += `${REMOVAL} ${this.formatValue(drift.ResourceType, chalk.cyan)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| deleted += `${REMOVAL} ${chalk.cyan(drift.ResourceType)} ${this.formatLogicalId(logicalToPathMap, drift.LogicalResourceId)}\n`; | ||
| } | ||
| deleted += this.printSectionFooter(); | ||
| } | ||
|
|
@@ -250,16 +248,6 @@ export class DriftFormatter { | |
| return `${normalizedPath} ${chalk.gray(logicalId)}`; | ||
| } | ||
|
|
||
| private formatValue(value: any, colorFn: (str: string) => string): string { | ||
|
Contributor
Author
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. We don't actually need this. It was also never called on anything that isn't a string. |
||
| if (value == null) { | ||
| return ''; | ||
| } | ||
| if (typeof value === 'string') { | ||
| return colorFn(value); | ||
| } | ||
| return colorFn(JSON.stringify(value)); | ||
| } | ||
|
|
||
| private printSectionHeader(title: string): string { | ||
| return `${chalk.underline(chalk.bold(title))}\n`; | ||
| } | ||
|
|
@@ -268,16 +256,16 @@ export class DriftFormatter { | |
| return '\n'; | ||
| } | ||
|
|
||
| private formatTreeDiff(propertyPath: string, difference: Difference<any>, isLast: boolean): string { | ||
| private formatTreeDiff(propertyPath: string, difference: Difference<string>, isLast: boolean): string { | ||
| let result = format(' %s─ %s %s\n', isLast ? '└' : '├', | ||
| difference.isAddition ? ADDITION : | ||
| difference.isRemoval ? REMOVAL : | ||
| UPDATE, | ||
| propertyPath, | ||
| ); | ||
| if (difference.isUpdate) { | ||
| result += format(' ├─ %s %s\n', REMOVAL, this.formatValue(difference.oldValue, chalk.red)); | ||
| result += format(' └─ %s %s\n', ADDITION, this.formatValue(difference.newValue, chalk.green)); | ||
|
Comment on lines
-279
to
-280
Contributor
Author
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.
|
||
| result += format(' ├─ %s %s\n', REMOVAL, chalk.red(difference.oldValue)); | ||
| result += format(' └─ %s %s\n', ADDITION, chalk.green(difference.newValue)); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
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.
This line was dead code, removed as it was wrong. We do NOT want to include metadata changes, but this line, if it worked, WOULD include them.
But metadata results are filtered out before we get to this point, anyway, so we can just remove the code.
Uh oh!
There was an error while loading. Please reload this page.
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.
question (blocking): Just to triple check:
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.
My bad. It's filtered out on line 88, but this is a different variable. Good catch. I've added tests for this as well.
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.
You still caught the incorrect filter though! Good job!