Implement set detailedDiff without protocol changes #2200
Description
Hello!
- Vote on this issue by adding a 👍 reaction
- If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)
Issue details
This proposal outlines an idea for how set element difference presentation in detailed diff can be improved without making any changes to the protocol buffer layer between Pulumi CLI and the providers.
A quick recap from the protobuf declarations:
DiffRequest {
string id = 1; // the ID of the resource to diff.
string urn = 2; // the Pulumi URN for this resource.
google.protobuf.Struct olds = 3; // the old output values of resource to diff.
google.protobuf.Struct news = 4; // the new input values of resource to diff.
repeated string ignoreChanges = 5; // a set of property paths that should be treated as unchanged.
google.protobuf.Struct old_inputs = 6; // the old input values of the resource to diff.
}
DiffResponse {
map<string, PropertyDiff> detailedDiff = 6; // a detailed diff appropriate for display.
bool hasDetailedDiff = 7; // true if this response contains a detailed diff.
}
message PropertyDiff {
Kind kind = 1; // The kind of diff associated with this property.
bool inputDiff = 2; // The difference is between old and new inputs, not old and new state.
enum Kind {
ADD = 0; // this property was added
ADD_REPLACE = 1; // this property was added, and this change requires a replace
DELETE = 2; // this property was removed
DELETE_REPLACE = 3; // this property was removed, and this change requires a replace
UPDATE = 4; // this property's value was changed
UPDATE_REPLACE = 5; // this property's value was changed, and this change requires a replace
}
}
The difficulty with set elements is how to refer to set elements correctly in the key of map<string, PropertyDiff> detailedDiff = 6; property paths, where Pulumi CLI treats these as numeric array paths, under reordering.
The proposal is as follows.
For Kind={ADD,ADD_REPLACE}, diffs should refer to the indices relative to news. For example:
olds = set(["A", "B", "C"])
news = set(["A", "B", "X", "C"]) # "X" added at index 2
detailedDiff = {"setProp[2]": {"kind": "ADD"}}
For 'Kind={DELETE,DELETE_REPLACE}`, diffs should refer to the indices relative to olds or oldInputs. For example:
olds = set(["A", "B", "C"]) # remove "B" at index 1
news = set(["A", "C"])
detailedDiff = {"setProp[1]": {"kind": "DELETE"}}
As to 'Kind={UPDATE,UPDATE_REPLACE}` diffs, these should never be emitted for set elements.
#2198 outlines some background on Set support in TF. It seems difficult to construct examples where fully known set elements would be equated and updated in place. This would require the elements to return the same identity or compare as Equals, at which point any nested changes probably should be ignored. Hypothesize that this case does not arise in practice.
If any of the config set elements contain unkowns, such elements have undecidable identity and the change should fall under Kind={ADD,ADD_REPLACE} case.
Affected area/feature
Confused index corner-case
As specified, it may happen that the diff will want to clobber the same index for adding and replacing.
olds = set(["A", "B", "C"]) # remove "B" at index 1
news = set(["A", "X", "C"]) # insert "X" at index 1
One possibility is to ignore the deleteDiff and only return the addDiff:
detailedDiff = {"setProp[1]": {"kind": "ADD"}}
Another possibility is to disambiguate the paths in some form, if this still displays reasonably in the CLI.
detailedDiff = {
"setProp[1].old": {"kind": "DELETE"},
"setProp[1]": {"kind": "Add"}
}
A small change to Pulumi CLI may be suggested here to permit multiple entries to the detailedDiff map, perhaps:
detailedDiff = {
"setProp[1]#1": {"kind": "DELETE"},
"setProp[1]#2": {"kind": "Add"}
}
ignoreChanges
Per #1756 it is difficult to use ignore changes on set elements at the moment. This proposal will not alleviate this, but not make it particularly worse. With this proposal there will be no nested set element changes, all will be either element additions or deletions. An ignoreChanges implementation that copies state to config by path could continue working using set-unaware array indices prior to engaging PlanResourceChange and have the support remain at that level.
Analyzing OpenTofu code Set ignoreChanges is not particularly tested or handled specially, and the support is implemented by index-based copying prior to planning with the provider.
- https://github.com/opentofu/opentofu/blob/5079292cb29acb7ca0707e4eec436331ce147b60/internal/tofu/reduce_plan_test.go#L438
- https://github.com/opentofu/opentofu/blob/main/internal/tofu/node_resource_abstract_instance.go#L808
Fixes
This should fix #186