Fix a few issues with diff #100
Merged
Conversation
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
The diff routine has three issues: 1. It does not correctly handled changes in nested proeprties 2. It inverts adds/deletes in the diffs it reports, leading to confusing diff presentation 3. It is untested. This PR fixes these issues. The first issue was introduced by the use of `ignoreKeys` callbacks on PropertyValue Diff. Unfortunately, that feature in pulumi/pulumi is not really safe to use, because it doesn't return full property paths, only the final key name in the nested path. So the caller cannot determine which nested key is really being referenced. The only other place it is used in pulumi/pulumi happens to only need to be correct for top-level properties, so happens to work. But it does not work for the way it was being used here. The second issue is an inversion of the order of arguments to PropertyMap#Diff. Test coverage for all the core of Diff is added. Additional coverage for ignoreChanges, CustomDiff, and Replacements should also be added in the future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The diff routine has three issues:
This PR fixes these issues.
The first issue was introduced by the use of
ignoreKeyscallbacks on PropertyValue Diff. Unfortunately, that feature in pulumi/pulumi is not really safe to use, because it doesn't return full property paths, only the final key name in the nested path. So the caller cannot determine which nested key is really being referenced. The only other place it is used in pulumi/pulumi happens to only need to be correct for top-level properties, so happens to work. But it does not work for the way it was being used here.The second issue is an inversion of the order of arguments to PropertyMap#Diff.
Test coverage for all the core of Diff is added. Additional coverage for ignoreChanges, CustomDiff, and Replacements should also be added in the future.
Fixes pulumi/pulumi-command#231.