-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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(diff): properties from ChangeSet diff were ignored #30093
Changes from all commits
9878273
eba549a
0e9e901
e38b3b0
9c08eff
99c87a1
a8c90d9
4d7f958
f1f0bc7
0d65781
8e7737b
dc85713
64e3a77
0dad158
8552b74
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 |
---|---|---|
|
@@ -55,7 +55,7 @@ export function fullDiff( | |
normalize(newTemplate); | ||
const theDiff = diffTemplate(currentTemplate, newTemplate); | ||
if (changeSet) { | ||
filterFalsePositives(theDiff, changeSet); | ||
refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources); | ||
addImportInformation(theDiff, changeSet); | ||
} else if (isImport) { | ||
makeAllResourceChangesImports(theDiff); | ||
|
@@ -143,13 +143,6 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl | |
return new types.TemplateDiff(differences); | ||
} | ||
|
||
/** | ||
* Compare two CloudFormation resources and return semantic differences between them | ||
*/ | ||
export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference { | ||
return impl.diffResource(oldValue, newValue); | ||
} | ||
|
||
/** | ||
* Replace all references to the given logicalID on the given template, in-place | ||
* | ||
|
@@ -229,45 +222,103 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) { | |
}); | ||
} | ||
|
||
function filterFalsePositives(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) { | ||
const replacements = findResourceReplacements(changeSet); | ||
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => { | ||
if (change.resourceType.includes('AWS::Serverless')) { | ||
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources | ||
return; | ||
function refineDiffWithChangeSet(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput, newTemplateResources: {[logicalId: string]: any}) { | ||
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. What do you think of breaking this down into two helper function calls? This is a pretty long function now and it will be a clearer for future contributors if
(the parameters will probably be different than what I've put here) 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 can do that if that's the style we prefer, but I'm not of the opinion that smaller functions lead to more readable code. If the code will never be executed in more than one location, then I think having the code inlined makes it more readable, since there is less indirection. For example, John Carmack wrote
(http://number-none.com/blow/john_carmack_on_inlined_code.html?utm_source=substack&utm_medium=email) Does that make sense? Do you agree or disagree? I do prefer having the single function with comments that explain what each code block is doing, as I think that's more readable 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. also, I think that's a great article ^. A book that endorses this opinion and that explicitly disagrees with Uncle Bob clean code style (which argues for functions no longer than 4-6 lines) is A Philosophy of Software Design 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 an interesting perspective. That post is generally agreeable; in particular, this:
The rest of the post makes a solid argument that one function called in 20 places is more likely to lead to bugs that are hard to fix than if you just inlined some code in that one spot you need it. That seems to be the general, overarching theme of the whole post; a function written in one place, with one set of assumptions, can do subtle yet catastrophic damage when called in a place with different assumptions. This is fair. For the code here, you could argue that it's very unclear if The only thing I'm not convinced of is this:
There's two sides of this for me:
This is from my personal experience, so I'm curious what your perspective is on this. I've reviewed code that uses very obscure variable names, but had a brief comment above explaining what was really happening. This isn't about functions or inlining anymore; this is just a short section of code that made an API call and did something with the response. Without the comment, there would be no way of knowing what that code was supposed to do because the variable names chosen communicated what the property was in AWS, but not at all what it was being used for.
Changing gears a bit, I think the clearest way to organize functions is based on their input and output types. For this function, we need changesets and the diff the entire time, and there's no clear way to break this by type. For me, a quick glance through both halves of this function is not very readable, but this was true both before and after this PR; it's a consequence of mutating the diff object in this way. I see the value of keeping it all in one function. I can see one other way that combines both worlds a bit, which we sometimes use in the CDK; create some local-scoped functions that only exist within this function. This means that they can't be called anywhere else, but we can still call them in here to indicate the two halves of this function at a glance. Either way (leaving it as you have it, or creating the local helper functions) is fine with me, I'll leave that up to you. 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 1 It also makes sense to have a centralized location for comments/documentation, so that you don't have multiple "comment states" to synchronize -- so ideally comments on implementation go right above what's being commented on or on a function/class signature that describes high level (say, business logic) details that are unlikely to change. Also, comments shouldn't explain implementation details but rather business logic that cannot be discerned from the code -- information that can be captured in code ought to be captured in the code itself, since code has lower odds of going stale. And I do think comment readers should always keep in mind the possibility that a comment they're reading is stale. Nonetheless, I am of the opinion that comments can communicate context that otherwise cannot be captured in code, and its precisely that information (cannot be captured in code) that should be commented. 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 ended up doing the private methods suggestion, as I think that makes both of us happy :) 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 agree with this sentiment, which runs a bit counter to the idea of comment blocks explaining what is happening; but I also see the danger of exposing too many functions that can make non-obvious state mutations. |
||
const replacements = _findResourceReplacements(changeSet); | ||
|
||
_addChangeSetResourcesToDiff(replacements, newTemplateResources); | ||
_enhanceChangeImpacts(replacements); | ||
return; | ||
|
||
function _findResourceReplacements(_changeSet: DescribeChangeSetOutput): types.ResourceReplacements { | ||
const _replacements: types.ResourceReplacements = {}; | ||
for (const resourceChange of _changeSet.Changes ?? []) { | ||
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {}; | ||
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) { | ||
if (propertyChange.Target?.Attribute === 'Properties') { | ||
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always'; | ||
if (requiresReplacement && propertyChange.Evaluation === 'Static') { | ||
propertiesReplaced[propertyChange.Target.Name!] = 'Always'; | ||
} else if (requiresReplacement && propertyChange.Evaluation === '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 | ||
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally'; | ||
} else { | ||
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement; | ||
} | ||
} | ||
} | ||
_replacements[resourceChange.ResourceChange?.LogicalResourceId!] = { | ||
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True', | ||
propertiesReplaced, | ||
}; | ||
} | ||
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => { | ||
if (type === 'Property') { | ||
if (!replacements[logicalId]) { | ||
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
(value as types.PropertyDifference<any>).isDifferent = false; | ||
return; | ||
|
||
return _replacements; | ||
} | ||
|
||
function _addChangeSetResourcesToDiff(_replacements: types.ResourceReplacements, _newTemplateResources: {[logicalId: string]: any}) { | ||
const resourceDiffLogicalIds = diff.resources.logicalIds; | ||
for (const logicalId of Object.keys(_replacements)) { | ||
if (!(resourceDiffLogicalIds.includes(logicalId))) { | ||
const noChangeResourceDiff = impl.diffResource(_newTemplateResources[logicalId], _newTemplateResources[logicalId]); | ||
diff.resources.add(logicalId, noChangeResourceDiff); | ||
} | ||
|
||
for (const propertyName of Object.keys(_replacements[logicalId].propertiesReplaced)) { | ||
if (propertyName in diff.resources.get(logicalId).propertyUpdates) { | ||
// If the property is already marked to be updated, then we don't need to do anything. | ||
continue; | ||
} | ||
switch (replacements[logicalId].propertiesReplaced[name]) { | ||
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: | ||
|
||
const newProp = new types.PropertyDifference( | ||
// these fields will be decided below | ||
{}, {}, { changeImpact: undefined }, | ||
); | ||
newProp.isDifferent = true; | ||
diff.resources.get(logicalId).setPropertyChange(propertyName, newProp); | ||
} | ||
}; | ||
} | ||
|
||
function _enhanceChangeImpacts(_replacements: types.ResourceReplacements) { | ||
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => { | ||
if (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 (!_replacements[logicalId]) { | ||
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE; | ||
(value as types.PropertyDifference<any>).isDifferent = false; | ||
break; | ||
return; | ||
} | ||
switch (_replacements[logicalId].propertiesReplaced[name]) { | ||
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 `diffTemplate` | ||
} | ||
} else if (type === 'Other') { | ||
switch (name) { | ||
case 'Metadata': | ||
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue)); | ||
break; | ||
} | ||
} | ||
} else if (type === 'Other') { | ||
switch (name) { | ||
case 'Metadata': | ||
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue)); | ||
break; | ||
} | ||
} | ||
}); | ||
}); | ||
}); | ||
} | ||
} | ||
|
||
function findResourceImports(changeSet: DescribeChangeSetOutput): string[] { | ||
|
@@ -281,33 +332,6 @@ function findResourceImports(changeSet: DescribeChangeSetOutput): string[] { | |
return importedResourceLogicalIds; | ||
} | ||
|
||
function findResourceReplacements(changeSet: DescribeChangeSetOutput): types.ResourceReplacements { | ||
const replacements: types.ResourceReplacements = {}; | ||
for (const resourceChange of changeSet.Changes ?? []) { | ||
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {}; | ||
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) { | ||
if (propertyChange.Target?.Attribute === 'Properties') { | ||
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always'; | ||
if (requiresReplacement && propertyChange.Evaluation === 'Static') { | ||
propertiesReplaced[propertyChange.Target.Name!] = 'Always'; | ||
} else if (requiresReplacement && propertyChange.Evaluation === '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 | ||
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally'; | ||
} else { | ||
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement; | ||
} | ||
} | ||
} | ||
replacements[resourceChange.ResourceChange?.LogicalResourceId!] = { | ||
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True', | ||
propertiesReplaced, | ||
}; | ||
} | ||
|
||
return replacements; | ||
} | ||
|
||
function normalize(template: any) { | ||
if (typeof template === 'object') { | ||
for (const key of (Object.keys(template ?? {}))) { | ||
|
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 was never used, so I removed it