Repository: pulumi/pulumi-terraform-bridge
Pull request #304 (2020-12-07 19:19:40)

Fix Set diffing to exclude items whose values are unchanged
`tfDiff` includes each item of the set, even when that specific item hasn't changed (i.e. d.Old == d.New). So if we just check for and exclude those cases, we get the results we expect.

Fixes: #296

github-actions[bot]: Diff for [pulumi-azuread](https://github.com/pulumi-bot/pulumi-azuread/compare/267289d..9417c33) with merge commit 15e36ec8c5f8edcbb65acbb5957fe861be4c654a
github-actions[bot]: Diff for [pulumi-random](https://github.com/pulumi-bot/pulumi-random/compare/0019607..e1594c7) with merge commit 15e36ec8c5f8edcbb65acbb5957fe861be4c654a
github-actions[bot]: Diff for [pulumi-random](https://github.com/pulumi-bot/pulumi-random/compare/b2063ff..f95ca87) with merge commit dca927d76a835ffceb2ac03852a060e4e3a07feb
github-actions[bot]: Diff for [pulumi-azuread](https://github.com/pulumi-bot/pulumi-azuread/compare/d176455..0b2c576) with merge commit dca927d76a835ffceb2ac03852a060e4e3a07feb
github-actions[bot]: Diff for [pulumi-gcp](https://github.com/pulumi-bot/pulumi-gcp/compare/7db761a1..b62463d3) with merge commit 15e36ec8c5f8edcbb65acbb5957fe861be4c654a
github-actions[bot]: Diff for [pulumi-gcp](https://github.com/pulumi-bot/pulumi-gcp/compare/3b4be405..8486b047) with merge commit dca927d76a835ffceb2ac03852a060e4e3a07feb
github-actions[bot]: Diff for [pulumi-azure](https://github.com/pulumi-bot/pulumi-azure/compare/5161ab20c..ec241ffdf) with merge commit 15e36ec8c5f8edcbb65acbb5957fe861be4c654a
github-actions[bot]: Diff for [pulumi-aws](https://github.com/pulumi-bot/pulumi-aws/compare/5947f8d90..3288cf558) with merge commit 15e36ec8c5f8edcbb65acbb5957fe861be4c654a
github-actions[bot]: Diff for [pulumi-azure](https://github.com/pulumi-bot/pulumi-azure/compare/5d650c4f8..59f7c4b69) with merge commit dca927d76a835ffceb2ac03852a060e4e3a07feb
github-actions[bot]: Diff for [pulumi-aws](https://github.com/pulumi-bot/pulumi-aws/compare/9cf218a54..531f0adea) with merge commit dca927d76a835ffceb2ac03852a060e4e3a07feb
pgavlin: > DiffKind is Update for a set item when there is actually no diff

Can you elaborate on this? Do we understand exactly the circumstances under which this occurs?
komalali: @pgavlin Thanks for asking that question, it helped me figure out the last remaining piece of the puzzle. It turns out that tfDiff includes each item of the set, even when that specific item hasn't changed (i.e. d.Old == d.New). So if we just check for and exclude those cases, we get the results we expect. 
github-actions[bot]: Diff for [pulumi-azuread](https://github.com/pulumi-bot/pulumi-azuread/compare/9e56003..f7c0614) with merge commit ff6b7182c3f75acf635e8eb7c9d0c5101ce83efc
github-actions[bot]: Diff for [pulumi-random](https://github.com/pulumi-bot/pulumi-random/compare/3049afe..d8cd0f3) with merge commit ff6b7182c3f75acf635e8eb7c9d0c5101ce83efc
komalali: Actually, I think we don't need joe's fix at all. If we are just excluding the diffs where `d.Old == d.New` we don't ever end up in a situation where the paths get overwritten.
github-actions[bot]: Diff for [pulumi-random](https://github.com/pulumi-bot/pulumi-random/compare/f08bb95..c43f64a) with merge commit 883f4e87519bf9a9fc7bef1b59ca2af6fe8b89da
github-actions[bot]: Diff for [pulumi-azuread](https://github.com/pulumi-bot/pulumi-azuread/compare/1495840..8078633) with merge commit 883f4e87519bf9a9fc7bef1b59ca2af6fe8b89da
github-actions[bot]: Diff for [pulumi-gcp](https://github.com/pulumi-bot/pulumi-gcp/compare/75d2bf15..8aaea28d) with merge commit ff6b7182c3f75acf635e8eb7c9d0c5101ce83efc
github-actions[bot]: Diff for [pulumi-azuread](https://github.com/pulumi-bot/pulumi-azuread/compare/d5f2737..eee8934) with merge commit 5af207f0da583b3f43ccb495a1200111682ee0fc
github-actions[bot]: Diff for [pulumi-random](https://github.com/pulumi-bot/pulumi-random/compare/56dfbe2..5038525) with merge commit 5af207f0da583b3f43ccb495a1200111682ee0fc
github-actions[bot]: Diff for [pulumi-gcp](https://github.com/pulumi-bot/pulumi-gcp/compare/bb3033ff..851f34ea) with merge commit 883f4e87519bf9a9fc7bef1b59ca2af6fe8b89da
github-actions[bot]: Diff for [pulumi-azure](https://github.com/pulumi-bot/pulumi-azure/compare/863b76a40..fb7f71213) with merge commit ff6b7182c3f75acf635e8eb7c9d0c5101ce83efc
github-actions[bot]: Diff for [pulumi-aws](https://github.com/pulumi-bot/pulumi-aws/compare/8268ce711..e9e428583) with merge commit ff6b7182c3f75acf635e8eb7c9d0c5101ce83efc
github-actions[bot]: Diff for [pulumi-gcp](https://github.com/pulumi-bot/pulumi-gcp/compare/403f0539..7fcaf123) with merge commit 5af207f0da583b3f43ccb495a1200111682ee0fc
github-actions[bot]: Diff for [pulumi-azure](https://github.com/pulumi-bot/pulumi-azure/compare/e21c899b1..13f2c31b5) with merge commit 883f4e87519bf9a9fc7bef1b59ca2af6fe8b89da
github-actions[bot]: Diff for [pulumi-aws](https://github.com/pulumi-bot/pulumi-aws/compare/a57874457..04bc9a9e0) with merge commit 883f4e87519bf9a9fc7bef1b59ca2af6fe8b89da
github-actions[bot]: Diff for [pulumi-azure](https://github.com/pulumi-bot/pulumi-azure/compare/89ee0093d..82aa8630f) with merge commit 5af207f0da583b3f43ccb495a1200111682ee0fc
github-actions[bot]: Diff for [pulumi-aws](https://github.com/pulumi-bot/pulumi-aws/compare/65465a761..59a352d77) with merge commit 5af207f0da583b3f43ccb495a1200111682ee0fc

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 26613fa7..f0cdcb2d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,9 @@ CHANGELOG
 
 ## HEAD (Unreleased)
 
+- Improve Set diffs to correctly identify replacements.
+  [#304](https://github.com/pulumi/pulumi-terraform-bridge/pull/304)
+
 - Deprecate UsesIOClasses bool.
   [#300](https://github.com/pulumi/pulumi-terraform-bridge/pull/300)
 
diff --git a/pkg/tfbridge/diff.go b/pkg/tfbridge/diff.go
index 2bd03a12..8f9020f4 100644
--- a/pkg/tfbridge/diff.go
+++ b/pkg/tfbridge/diff.go
@@ -52,14 +52,14 @@ func containsComputedValues(v resource.PropertyValue) bool {
 // the value.
 type propertyVisitor func(attributeKey, propertyPath string, value resource.PropertyValue) bool
 
-// visitPropertyDiff checks the given property for diffs and invokes the given callback if a diff is found.
+// visitPropertyValue checks the given property for diffs and invokes the given callback if a diff is found.
 //
 // This function contains the core logic used to convert a terraform.InstanceDiff to a list of Pulumi property paths
 // that have changed and the type of change for each path.
 //
 // If not for the presence of sets and the fact that they are mapped to Pulumi array properties, this process would be
 // rather straightforward: we could loop over each path in the instance diff, convert its path to a Pulumi property
-// path, and convert the diff kind to a Pulumi diff kind. Unforunately, the set mapping complicates this process, as
+// path, and convert the diff kind to a Pulumi diff kind. Unfortunately, the set mapping complicates this process, as
 // the array index that corresponds to a set element cannot be derived from the set element's path: part of the path of
 // a set element is the element's hash code, and we cannot derive the corresponding array element's index from this
 // hash code.
@@ -181,7 +181,7 @@ func makePropertyDiff(name, path string, v resource.PropertyValue, tfDiff shim.I
 				name += ".%"
 			}
 		}
-		if d := tfDiff.Attribute(name); d != nil {
+		if d := tfDiff.Attribute(name); d != nil && d.Old != d.New {
 			other, hasOtherDiff := diff[path]
 
 			// If we're finalizing the diff, we want to remove any ADD diffs that were only present in the state.


Test patch:
diff --git a/pkg/tfbridge/diff_test.go b/pkg/tfbridge/diff_test.go
index ce2499fb..787565a2 100644
--- a/pkg/tfbridge/diff_test.go
+++ b/pkg/tfbridge/diff_test.go
@@ -1301,3 +1301,176 @@ func TestRawElementNames(t *testing.T) {
 			"prop.variables.DYNAMODB_ROUTE_TABLE_NAME": U,
 		})
 }
+
+// SETS AND LISTS WITH MULTIPLE ITEMS
+
+func TestCollectionsWithMultipleItems(t *testing.T) {
+	testCases := []struct {
+		name                string
+		state               []interface{}
+		input               []interface{}
+		expectedDiffForSet  map[string]DiffKind
+		expectedDiffForList map[string]DiffKind
+	}{
+		{
+			"NoChanges",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			map[string]DiffKind{},
+			map[string]DiffKind{},
+		},
+		{
+			"Reordered",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"tineke", "burgundy", "ruby"},
+			map[string]DiffKind{},
+			map[string]DiffKind{
+				"prop[0]": UR,
+				"prop[1]": UR,
+				"prop[2]": UR,
+			},
+		},
+		{
+			"RemoveFirst",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"ruby", "tineke"},
+			map[string]DiffKind{
+				"prop[0]": DR,
+			},
+			map[string]DiffKind{
+				"prop[0]": UR,
+				"prop[1]": UR,
+				"prop[2]": DR,
+			},
+		},
+		{
+			"RemoveMiddle",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"burgundy", "tineke"},
+			map[string]DiffKind{
+				"prop[1]": DR,
+			},
+			map[string]DiffKind{
+				"prop[1]": UR,
+				"prop[2]": DR,
+			},
+		},
+		{
+			"RemoveLast",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"burgundy", "ruby"},
+			map[string]DiffKind{
+				"prop[2]": DR,
+			},
+			map[string]DiffKind{
+				"prop[2]": DR,
+			},
+		},
+		{
+			"AddFirst",
+			[]interface{}{"ruby", "tineke"},
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			map[string]DiffKind{
+				"prop[0]": AR,
+			},
+			map[string]DiffKind{
+				"prop[0]": UR,
+				"prop[1]": UR,
+				"prop[2]": AR,
+			},
+		},
+		{
+			"AddMiddle",
+			[]interface{}{"burgundy", "tineke"},
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			map[string]DiffKind{
+				"prop[1]": AR,
+			},
+			map[string]DiffKind{
+				"prop[1]": UR,
+				"prop[2]": AR,
+			},
+		},
+		{
+			"AddLast",
+			[]interface{}{"burgundy", "ruby"},
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			map[string]DiffKind{
+				"prop[2]": AR,
+			},
+			map[string]DiffKind{
+				"prop[2]": AR,
+			},
+		},
+		{
+			"UpdateFirst",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"robusta", "ruby", "tineke"},
+			map[string]DiffKind{
+				"prop[0]": UR,
+			},
+			map[string]DiffKind{
+				"prop[0]": UR,
+			},
+		},
+		{
+			"UpdateMiddle",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"burgundy", "robusta", "tineke"},
+			map[string]DiffKind{
+				"prop[1]": UR,
+			},
+			map[string]DiffKind{
+				"prop[1]": UR,
+			},
+		},
+		{
+			"UpdateLast",
+			[]interface{}{"burgundy", "ruby", "tineke"},
+			[]interface{}{"burgundy", "ruby", "robusta"},
+			map[string]DiffKind{
+				"prop[2]": UR,
+			},
+			map[string]DiffKind{
+				"prop[2]": UR,
+			},
+		},
+	}
+
+	// nolint:lll
+	runTestCase := func(t *testing.T, name string, typ schema.ValueType, inputs, state []interface{}, expected map[string]DiffKind) {
+		t.Run(name, func(t *testing.T) {
+			diffTest(t,
+				map[string]*schema.Schema{
+					"prop": {
+						Type:     typ,
+						Elem:     &schema.Schema{Type: schema.TypeString},
+						ForceNew: true,
+					},
+					"outp": {Type: schema.TypeString, Computed: true},
+				},
+				map[string]*SchemaInfo{},
+				map[string]interface{}{
+					"prop": inputs, // inputs
+				},
+				map[string]interface{}{
+					"prop": state, // state
+					"outp": "bar",
+				},
+				expected,
+			)
+		})
+	}
+
+	t.Run("Set", func(t *testing.T) {
+		for _, tc := range testCases {
+			runTestCase(t, tc.name, schema.TypeSet, tc.input, tc.state, tc.expectedDiffForSet)
+		}
+	})
+
+	t.Run("List", func(t *testing.T) {
+		for _, tc := range testCases {
+			runTestCase(t, tc.name, schema.TypeList, tc.input, tc.state, tc.expectedDiffForList)
+		}
+	})
+}