-
Notifications
You must be signed in to change notification settings - Fork 4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor(diff): make dedicated file and class for incorporating chang…
…eset to templateDiff (#30332) ### Reason for this change I am making this change as part of #30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done. ### Description of changes * A ton of unit tests and moved changeset diff logic into a dedicated class and file. ### Description of how you validated changes * Many unit tests, integration tests, and manual tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
- Loading branch information
Showing
8 changed files
with
1,563 additions
and
715 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
157 changes: 157 additions & 0 deletions
157
packages/@aws-cdk/cloudformation-diff/lib/diff/template-and-changeset-diff-merger.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| // The SDK is only used to reference `DescribeChangeSetOutput`, so the SDK is added as a devDependency. | ||
| // The SDK should not make network calls here | ||
| import type { DescribeChangeSetOutput as DescribeChangeSet, ResourceChangeDetail as RCD } from '@aws-sdk/client-cloudformation'; | ||
| import * as types from '../diff/types'; | ||
|
|
||
| export type DescribeChangeSetOutput = DescribeChangeSet; | ||
| type ChangeSetResourceChangeDetail = RCD; | ||
|
|
||
| interface TemplateAndChangeSetDiffMergerOptions { | ||
| /* | ||
| * Only specifiable for testing. Otherwise, this is the datastructure that the changeSet is converted into so | ||
| * that we only pay attention to the subset of changeSet properties that are relevant for computing the diff. | ||
| * | ||
| * @default - the changeSet is converted into this datastructure. | ||
| */ | ||
| readonly changeSetResources?: types.ChangeSetResources; | ||
| } | ||
|
|
||
| export interface TemplateAndChangeSetDiffMergerProps extends TemplateAndChangeSetDiffMergerOptions { | ||
| /* | ||
| * The changeset that will be read and merged into the template diff. | ||
| */ | ||
| readonly changeSet: DescribeChangeSetOutput; | ||
| } | ||
|
|
||
| /** | ||
| * The purpose of this class is to include differences from the ChangeSet to differences in the TemplateDiff. | ||
| */ | ||
| export class TemplateAndChangeSetDiffMerger { | ||
|
|
||
| public static determineChangeSetReplacementMode(propertyChange: ChangeSetResourceChangeDetail): types.ReplacementModes { | ||
| if (propertyChange.Target?.RequiresRecreation === undefined) { | ||
| // We can't determine if the resource will be replaced or not. That's what conditionally means. | ||
| return 'Conditionally'; | ||
| } | ||
|
|
||
| if (propertyChange.Target.RequiresRecreation === 'Always') { | ||
| switch (propertyChange.Evaluation) { | ||
| case 'Static': | ||
| return 'Always'; | ||
| case 'Dynamic': | ||
| // If Evaluation is 'Dynamic', then this may cause replacement, or it may not. | ||
| // see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html | ||
| return 'Conditionally'; | ||
| } | ||
| } | ||
|
|
||
| return propertyChange.Target.RequiresRecreation as types.ReplacementModes; | ||
| } | ||
|
|
||
| // If we somehow cannot find the resourceType, then we'll mark it as UNKNOWN, so that can be seen in the diff. | ||
| private static UNKNOWN_RESOURCE_TYPE = 'UNKNOWN_RESOURCE_TYPE'; | ||
|
|
||
| public changeSet: DescribeChangeSetOutput | undefined; | ||
| public changeSetResources: types.ChangeSetResources; | ||
|
|
||
| constructor(props: TemplateAndChangeSetDiffMergerProps) { | ||
| this.changeSet = props.changeSet; | ||
| this.changeSetResources = props.changeSetResources ?? this.convertDescribeChangeSetOutputToChangeSetResources(this.changeSet); | ||
| } | ||
|
|
||
| /** | ||
| * Read resources from the changeSet, extracting information into ChangeSetResources. | ||
| */ | ||
| private convertDescribeChangeSetOutputToChangeSetResources(changeSet: DescribeChangeSetOutput): types.ChangeSetResources { | ||
| const changeSetResources: types.ChangeSetResources = {}; | ||
| for (const resourceChange of changeSet.Changes ?? []) { | ||
| if (resourceChange.ResourceChange?.LogicalResourceId === undefined) { | ||
| continue; // Being defensive, here. | ||
| } | ||
|
|
||
| const propertyReplacementModes: types.PropertyReplacementModeMap = {}; | ||
| for (const propertyChange of resourceChange.ResourceChange.Details ?? []) { // Details is only included if resourceChange.Action === 'Modify' | ||
| if (propertyChange.Target?.Attribute === 'Properties' && propertyChange.Target.Name) { | ||
| propertyReplacementModes[propertyChange.Target.Name] = { | ||
| replacementMode: TemplateAndChangeSetDiffMerger.determineChangeSetReplacementMode(propertyChange), | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| changeSetResources[resourceChange.ResourceChange.LogicalResourceId] = { | ||
| resourceWasReplaced: resourceChange.ResourceChange.Replacement === 'True', | ||
| resourceType: resourceChange.ResourceChange.ResourceType ?? TemplateAndChangeSetDiffMerger.UNKNOWN_RESOURCE_TYPE, // DescribeChangeSet doesn't promise to have the ResourceType... | ||
| propertyReplacementModes: propertyReplacementModes, | ||
| }; | ||
| } | ||
|
|
||
| return changeSetResources; | ||
| } | ||
|
|
||
| /** | ||
| * This is writing over the "ChangeImpact" that was computed from the template difference, and instead using the ChangeImpact that is included from the ChangeSet. | ||
| * Using the ChangeSet ChangeImpact is more accurate. The ChangeImpact tells us what the consequence is of changing the field. If changing the field causes resource | ||
| * replacement (e.g., changing the name of an IAM role requires deleting and replacing the role), then ChangeImpact is "Always". | ||
| */ | ||
| public overrideDiffResourceChangeImpactWithChangeSetChangeImpact(logicalId: string, change: types.ResourceDifference) { | ||
| // resourceType getter throws an error if resourceTypeChanged | ||
| if ((change.resourceTypeChanged === true) || change.resourceType?.includes('AWS::Serverless')) { | ||
| // CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources | ||
| return; | ||
| } | ||
| change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => { | ||
| if (type === 'Property') { | ||
| if (!this.changeSetResources[logicalId]) { | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
| (value as types.PropertyDifference<any>).isDifferent = false; | ||
| return; | ||
| } | ||
|
|
||
| const changingPropertyCausesResourceReplacement = (this.changeSetResources[logicalId].propertyReplacementModes ?? {})[name]?.replacementMode; | ||
| switch (changingPropertyCausesResourceReplacement) { | ||
| case 'Always': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE; | ||
| break; | ||
| case 'Never': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE; | ||
| break; | ||
| case 'Conditionally': | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE; | ||
| break; | ||
| case undefined: | ||
| (value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
| (value as types.PropertyDifference<any>).isDifferent = false; | ||
| break; | ||
| // otherwise, defer to the changeImpact from the template diff | ||
| } | ||
| } else if (type === 'Other') { | ||
| switch (name) { | ||
| case 'Metadata': | ||
| // we want to ignore metadata changes in the diff, so compare newValue against newValue. | ||
| change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue)); | ||
| break; | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public addImportInformationFromChangeset(resourceDiffs: types.DifferenceCollection<types.Resource, types.ResourceDifference>) { | ||
| const imports = this.findResourceImports(); | ||
| resourceDiffs.forEachDifference((logicalId: string, change: types.ResourceDifference) => { | ||
| if (imports.includes(logicalId)) { | ||
| change.isImport = true; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| public findResourceImports(): (string | undefined)[] { | ||
| const importedResourceLogicalIds = []; | ||
| for (const resourceChange of this.changeSet?.Changes ?? []) { | ||
| if (resourceChange.ResourceChange?.Action === 'Import') { | ||
| importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId); | ||
| } | ||
| } | ||
|
|
||
| return importedResourceLogicalIds; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.