From 629a5ff3544665821a49127753511deece30fe8e Mon Sep 17 00:00:00 2001 From: Venelin Date: Thu, 17 Oct 2024 16:26:46 +0100 Subject: [PATCH 01/12] refactor detailed diff v2 to short circuit on nulls and unknowns --- pkg/tfbridge/detailed_diff.go | 227 +++++++++++++++++------------ pkg/tfbridge/detailed_diff_test.go | 22 +-- 2 files changed, 140 insertions(+), 109 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index f7d6a00a0..8896bd62d 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -41,6 +41,89 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { return keysSlice } +func lookupSchemas( + path propertyPath, tfs shim.SchemaMap, ps map[string]*info.Schema, +) (shim.Schema, *info.Schema, error) { + schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), tfs, ps) + return LookupSchemas(schemaPath, tfs, ps) +} + +// walkPropertyValue walks a property value and calls the visitor function for each path in the property value. +func walkPropertyValue( + val resource.PropertyValue, path propertyPath, visitor func(propertyPath, resource.PropertyValue) bool, +) bool { + if !visitor(path, val) { + return false + } + + switch { + case val.IsArray(): + for i, elVal := range val.ArrayValue() { + if !walkPropertyValue(elVal, path.Index(i), visitor) { + return false + } + } + + case val.IsObject(): + for k, elVal := range val.ObjectValue() { + if !walkPropertyValue(elVal, path.Subpath(string(k)), visitor) { + return false + } + } + } + return true +} + +func willTriggerReplacement( + path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema, +) bool { + // A change on a property might trigger a replacement if: + // - The property itself is marked as ForceNew + // - The direct parent property is a collection (list, set, map) and is marked as ForceNew + // See pkg/cross-tests/diff_cross_test.go + // TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew + // for a full case study of replacements in TF + tfs, ps, err := lookupSchemas(path, rootTFSchema, rootPulumiSchema) + if err != nil { + return false + } + if isForceNew(tfs, ps) { + return true + } + + if len(path) == 1 { + return false + } + + parent := path[:len(path)-1] + tfs, ps, err = lookupSchemas(parent, rootTFSchema, rootPulumiSchema) + if err != nil { + return false + } + // Note this is mimicking the TF behaviour, so the effective type is not considered here. + if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap { + return false + } + return isForceNew(tfs, ps) +} + +func willTriggerReplacementRecursive( + path propertyPath, value resource.PropertyValue, tfs shim.SchemaMap, ps map[string]*info.Schema, +) bool { + replacement := false + visitor := func(subpath propertyPath, val resource.PropertyValue) bool { + if willTriggerReplacement(subpath, tfs, ps) { + replacement = true + return false + } + return true + } + + walkPropertyValue(value, path, visitor) + + return replacement +} + func promoteToReplace(diff *pulumirpc.PropertyDiff) *pulumirpc.PropertyDiff { if diff == nil { return nil @@ -143,17 +226,6 @@ func (k propertyPath) IsReservedKey() bool { return leaf == "__meta" || leaf == "__defaults" } -func mapHasReplacements(m map[detailedDiffKey]*pulumirpc.PropertyDiff) bool { - for _, diff := range m { - if diff.GetKind() == pulumirpc.PropertyDiff_ADD_REPLACE || - diff.GetKind() == pulumirpc.PropertyDiff_DELETE_REPLACE || - diff.GetKind() == pulumirpc.PropertyDiff_UPDATE_REPLACE { - return true - } - } - return false -} - type detailedDiffer struct { tfs shim.SchemaMap ps map[string]*SchemaInfo @@ -182,71 +254,13 @@ func (differ detailedDiffer) getEffectiveType(path walk.SchemaPath) shim.ValueTy return tfs.Type() } -func (differ detailedDiffer) lookupSchemas(path propertyPath) (shim.Schema, *info.Schema, error) { - schemaPath := PropertyPathToSchemaPath(resource.PropertyPath(path), differ.tfs, differ.ps) - return LookupSchemas(schemaPath, differ.tfs, differ.ps) -} - -func (differ detailedDiffer) isForceNew(pair propertyPath) bool { - // A change on a property might trigger a replacement if: - // - The property itself is marked as ForceNew - // - The direct parent property is a collection (list, set, map) and is marked as ForceNew - // See pkg/cross-tests/diff_cross_test.go - // TestAttributeCollectionForceNew, TestBlockCollectionForceNew, TestBlockCollectionElementForceNew - // for a full case study of replacements in TF - tfs, ps, err := differ.lookupSchemas(pair) - if err != nil { - return false - } - if isForceNew(tfs, ps) { - return true - } - - if len(pair) == 1 { - return false - } - - parent := pair[:len(pair)-1] - tfs, ps, err = differ.lookupSchemas(parent) - if err != nil { - return false - } - // Note this is mimicking the TF behaviour, so the effective type is not considered here. - if tfs.Type() != shim.TypeList && tfs.Type() != shim.TypeSet && tfs.Type() != shim.TypeMap { - return false - } - return isForceNew(tfs, ps) -} - -// We do not short-circuit detailed diffs when comparing non-nil properties against nil ones. The reason for that is -// that a replace might be triggered by a ForceNew inside a nested property of a non-ForceNew property. We instead -// always walk the full tree even when comparing against a nil property. We then later do a simplification step for -// the detailed diff in simplifyDiff in order to reduce the diff to what the user expects to see. -// See [pulumi/pulumi-terraform-bridge#2405] for more details. -func (differ detailedDiffer) simplifyDiff( - diff map[detailedDiffKey]*pulumirpc.PropertyDiff, path propertyPath, old, new resource.PropertyValue, -) (map[detailedDiffKey]*pulumirpc.PropertyDiff, bool) { - baseDiff := makeBaseDiff(old, new) - if baseDiff == undecidedDiff { - return nil, false - } - propDiff := baseDiff.ToPropertyDiff() - if propDiff == nil { - return nil, true - } - if differ.isForceNew(path) || mapHasReplacements(diff) { - propDiff = promoteToReplace(propDiff) - } - return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff}, true -} - // makePlainPropDiff is used for plain properties and ones with an unknown schema. // It does not access the TF schema, so it does not know about the type of the property. func (differ detailedDiffer) makePlainPropDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { baseDiff := makeBaseDiff(old, new) - isForceNew := differ.isForceNew(path) + isReplacement := willTriggerReplacement(path, differ.tfs, differ.ps) var propDiff *pulumirpc.PropertyDiff if baseDiff != undecidedDiff { propDiff = baseDiff.ToPropertyDiff() @@ -254,7 +268,7 @@ func (differ detailedDiffer) makePlainPropDiff( propDiff = &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_UPDATE} } - if isForceNew { + if isReplacement { propDiff = promoteToReplace(propDiff) } @@ -264,6 +278,32 @@ func (differ detailedDiffer) makePlainPropDiff( return nil } +// makeShortCircuitDiff is used for properties that are nil or computed in either the old or new state. +// It makes sure to check recursively if the property will trigger a replacement. +func (differ detailedDiffer) makeShortCircuitDiff( + path propertyPath, old, new resource.PropertyValue, +) map[detailedDiffKey]*pulumirpc.PropertyDiff { + contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed(), + "short-circuit diff should only be used for nil properties") + if old.IsNull() && new.IsNull() { + return nil + } + + baseDiff := makeBaseDiff(old, new) + contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind") + + propDiff := baseDiff.ToPropertyDiff() + if new.IsComputed() && willTriggerReplacement(path, differ.tfs, differ.ps) { + propDiff = promoteToReplace(propDiff) + } else if !new.IsNull() && !new.IsComputed() && willTriggerReplacementRecursive(path, new, differ.tfs, differ.ps) { + propDiff = promoteToReplace(propDiff) + } else if !old.IsNull() && willTriggerReplacementRecursive(path, old, differ.tfs, differ.ps) { + propDiff = promoteToReplace(propDiff) + } + + return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff} +} + func (differ detailedDiffer) makePropDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { @@ -289,15 +329,19 @@ func (differ detailedDiffer) makePropDiff( func (differ detailedDiffer) makeListDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) - oldList := []resource.PropertyValue{} - newList := []resource.PropertyValue{} - if isPresent(old) && old.IsArray() { - oldList = old.ArrayValue() + if !isPresent(old) || !old.IsArray() { + old = resource.NewNullProperty() } - if isPresent(new) && new.IsArray() { - newList = new.ArrayValue() + if (!isPresent(new) || !new.IsArray()) && !new.IsComputed() { + new = resource.NewNullProperty() } + if old.IsNull() || new.IsNull() || new.IsComputed() { + return differ.makeShortCircuitDiff(path, old, new) + } + + diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) + oldList := old.ArrayValue() + newList := new.ArrayValue() // naive diffing of lists // TODO[pulumi/pulumi-terraform-bridge#2295]: implement a more sophisticated diffing algorithm @@ -318,27 +362,25 @@ func (differ detailedDiffer) makeListDiff( } } - simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new) - if isSimplified { - return simplerDiff - } - return diff } func (differ detailedDiffer) makeMapDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) - oldMap := resource.PropertyMap{} - newMap := resource.PropertyMap{} - if isPresent(old) && old.IsObject() { - oldMap = old.ObjectValue() + if !isPresent(old) || !old.IsObject() { + old = resource.NewNullProperty() + } + if !isPresent(new) || !new.IsObject() && !new.IsComputed() { + new = resource.NewNullProperty() } - if isPresent(new) && new.IsObject() { - newMap = new.ObjectValue() + if old.IsNull() || new.IsNull() || new.IsComputed() { + return differ.makeShortCircuitDiff(path, old, new) } + oldMap := old.ObjectValue() + newMap := new.ObjectValue() + diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) for _, k := range sortedMergedKeys(oldMap, newMap) { subindex := path.Subpath(string(k)) oldVal := oldMap[k] @@ -351,11 +393,6 @@ func (differ detailedDiffer) makeMapDiff( } } - simplerDiff, isSimplified := differ.simplifyDiff(diff, path, old, new) - if isSimplified { - return simplerDiff - } - return diff } diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index e75216d97..2ab8306fe 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -42,11 +42,9 @@ func TestSchemaLookupMaxItemsOnePlain(t *testing.T) { }, } - differ := detailedDiffer{ - tfs: shimv2.NewSchemaMap(sdkv2Schema), - } + tfs := shimv2.NewSchemaMap(sdkv2Schema) - sch, _, err := differ.lookupSchemas(newPropertyPath("string_prop")) + sch, _, err := lookupSchemas(newPropertyPath("string_prop"), tfs, nil) require.NoError(t, err) require.NotNil(t, sch) require.Equal(t, sch.Type(), shim.TypeList) @@ -71,16 +69,14 @@ func TestSchemaLookupMaxItemsOne(t *testing.T) { }, } - differ := detailedDiffer{ - tfs: shimv2.NewSchemaMap(res.Schema), - } + tfs := shimv2.NewSchemaMap(res.Schema) - sch, _, err := differ.lookupSchemas(newPropertyPath("foo")) + sch, _, err := lookupSchemas(newPropertyPath("foo"), tfs, nil) require.NoError(t, err) require.NotNil(t, sch) require.Equal(t, sch.Type(), shim.TypeList) - sch, _, err = differ.lookupSchemas(newPropertyPath("foo").Subpath("bar")) + sch, _, err = lookupSchemas(newPropertyPath("foo").Subpath("bar"), tfs, nil) require.NoError(t, err) require.NotNil(t, sch) require.Equal(t, sch.Type(), shim.TypeString) @@ -100,16 +96,14 @@ func TestSchemaLookupMap(t *testing.T) { }, } - differ := detailedDiffer{ - tfs: shimv2.NewSchemaMap(res.Schema), - } + tfs := shimv2.NewSchemaMap(res.Schema) - sch, _, err := differ.lookupSchemas(newPropertyPath("foo")) + sch, _, err := lookupSchemas(newPropertyPath("foo"), tfs, nil) require.NoError(t, err) require.NotNil(t, sch) require.Equal(t, sch.Type(), shim.TypeMap) - sch, _, err = differ.lookupSchemas(propertyPath{"foo", "bar"}) + sch, _, err = lookupSchemas(newPropertyPath("foo").Subpath("bar"), tfs, nil) require.NoError(t, err) require.NotNil(t, sch) require.Equal(t, sch.Type(), shim.TypeString) From 49ec2bee1422d910d50b119fd7f219392a64a122 Mon Sep 17 00:00:00 2001 From: Venelin Date: Fri, 18 Oct 2024 12:00:21 +0100 Subject: [PATCH 02/12] move type mismatch check and short-circuit logic --- pkg/tfbridge/detailed_diff.go | 45 +++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 8896bd62d..12da5ea39 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -304,6 +304,22 @@ func (differ detailedDiffer) makeShortCircuitDiff( return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff} } +func isTypeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { + if !isPresent(val) { + return false + } + switch propType { + case shim.TypeList: + return !val.IsArray() + case shim.TypeSet: + return !val.IsArray() + case shim.TypeMap: + return !val.IsObject() + default: + return false + } +} + func (differ detailedDiffer) makePropDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { @@ -311,6 +327,15 @@ func (differ detailedDiffer) makePropDiff( return nil } propType := differ.getEffectiveType(differ.propertyPathToSchemaPath(path)) + if !isPresent(old) || isTypeMismatched(old, propType) { + old = resource.NewNullProperty() + } + if !isPresent(new) || isTypeMismatched(new, propType) && !new.IsComputed() { + new = resource.NewNullProperty() + } + if old.IsNull() || new.IsNull() || new.IsComputed() { + return differ.makeShortCircuitDiff(path, old, new) + } switch propType { case shim.TypeList: @@ -329,16 +354,6 @@ func (differ detailedDiffer) makePropDiff( func (differ detailedDiffer) makeListDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - if !isPresent(old) || !old.IsArray() { - old = resource.NewNullProperty() - } - if (!isPresent(new) || !new.IsArray()) && !new.IsComputed() { - new = resource.NewNullProperty() - } - if old.IsNull() || new.IsNull() || new.IsComputed() { - return differ.makeShortCircuitDiff(path, old, new) - } - diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) oldList := old.ArrayValue() newList := new.ArrayValue() @@ -368,16 +383,6 @@ func (differ detailedDiffer) makeListDiff( func (differ detailedDiffer) makeMapDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - if !isPresent(old) || !old.IsObject() { - old = resource.NewNullProperty() - } - if !isPresent(new) || !new.IsObject() && !new.IsComputed() { - new = resource.NewNullProperty() - } - if old.IsNull() || new.IsNull() || new.IsComputed() { - return differ.makeShortCircuitDiff(path, old, new) - } - oldMap := old.ObjectValue() newMap := new.ObjectValue() diff := make(map[detailedDiffKey]*pulumirpc.PropertyDiff) From e13ae475836e429861cb701abe168e908469dbd1 Mon Sep 17 00:00:00 2001 From: Venelin Date: Fri, 18 Oct 2024 12:01:38 +0100 Subject: [PATCH 03/12] rename --- pkg/tfbridge/detailed_diff.go | 36 +++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 12da5ea39..8455a0378 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -41,6 +41,22 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { return keysSlice } +func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { + if !isPresent(val) { + return false + } + switch propType { + case shim.TypeList: + return !val.IsArray() + case shim.TypeSet: + return !val.IsArray() + case shim.TypeMap: + return !val.IsObject() + default: + return false + } +} + func lookupSchemas( path propertyPath, tfs shim.SchemaMap, ps map[string]*info.Schema, ) (shim.Schema, *info.Schema, error) { @@ -304,22 +320,6 @@ func (differ detailedDiffer) makeShortCircuitDiff( return map[detailedDiffKey]*pulumirpc.PropertyDiff{path.Key(): propDiff} } -func isTypeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { - if !isPresent(val) { - return false - } - switch propType { - case shim.TypeList: - return !val.IsArray() - case shim.TypeSet: - return !val.IsArray() - case shim.TypeMap: - return !val.IsObject() - default: - return false - } -} - func (differ detailedDiffer) makePropDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { @@ -327,10 +327,10 @@ func (differ detailedDiffer) makePropDiff( return nil } propType := differ.getEffectiveType(differ.propertyPathToSchemaPath(path)) - if !isPresent(old) || isTypeMismatched(old, propType) { + if !isPresent(old) || isTypeShapeMismatched(old, propType) { old = resource.NewNullProperty() } - if !isPresent(new) || isTypeMismatched(new, propType) && !new.IsComputed() { + if !isPresent(new) || isTypeShapeMismatched(new, propType) && !new.IsComputed() { new = resource.NewNullProperty() } if old.IsNull() || new.IsNull() || new.IsComputed() { From e6837ce0216533cad012cf9c6176716556838273 Mon Sep 17 00:00:00 2001 From: Venelin Date: Thu, 24 Oct 2024 20:13:46 -0700 Subject: [PATCH 04/12] use propertyvalue.TransformPropertyValue instead of the new visitor function --- pkg/tfbridge/detailed_diff.go | 41 +++++++++-------------------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 8455a0378..6fe392b4d 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -12,6 +12,7 @@ import ( "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info" shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/walk" + "github.com/pulumi/pulumi-terraform-bridge/v3/unstable/propertyvalue" ) func isPresent(val resource.PropertyValue) bool { @@ -64,32 +65,6 @@ func lookupSchemas( return LookupSchemas(schemaPath, tfs, ps) } -// walkPropertyValue walks a property value and calls the visitor function for each path in the property value. -func walkPropertyValue( - val resource.PropertyValue, path propertyPath, visitor func(propertyPath, resource.PropertyValue) bool, -) bool { - if !visitor(path, val) { - return false - } - - switch { - case val.IsArray(): - for i, elVal := range val.ArrayValue() { - if !walkPropertyValue(elVal, path.Index(i), visitor) { - return false - } - } - - case val.IsObject(): - for k, elVal := range val.ObjectValue() { - if !walkPropertyValue(elVal, path.Subpath(string(k)), visitor) { - return false - } - } - } - return true -} - func willTriggerReplacement( path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema, ) bool { @@ -127,15 +102,19 @@ func willTriggerReplacementRecursive( path propertyPath, value resource.PropertyValue, tfs shim.SchemaMap, ps map[string]*info.Schema, ) bool { replacement := false - visitor := func(subpath propertyPath, val resource.PropertyValue) bool { - if willTriggerReplacement(subpath, tfs, ps) { + visitor := func(subpath resource.PropertyPath, val resource.PropertyValue) (resource.PropertyValue, error) { + if willTriggerReplacement(propertyPath(subpath), tfs, ps) { replacement = true - return false } - return true + return val, nil } - walkPropertyValue(value, path, visitor) + _, err := propertyvalue.TransformPropertyValue( + resource.PropertyPath(path), + visitor, + value, + ) + contract.AssertNoErrorf(err, "TransformPropertyValue should not return an error") return replacement } From 05a625c0c6add5a5232e6744236f47a4b7278053 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 28 Oct 2024 15:40:43 +0000 Subject: [PATCH 05/12] Tests for the existing behaviour with collection ForceNew --- dynamic/go.sum | 4 +- go.mod | 2 +- go.sum | 4 +- pf/go.sum | 4 +- pkg/tests/schema_pulumi_test.go | 424 ++++++++++++++++++++++++++++++++ x/muxer/go.mod | 2 +- x/muxer/go.sum | 4 +- 7 files changed, 434 insertions(+), 10 deletions(-) diff --git a/dynamic/go.sum b/dynamic/go.sum index 522e87ed7..404bd7896 100644 --- a/dynamic/go.sum +++ b/dynamic/go.sum @@ -752,8 +752,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE= github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c= github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA= github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY= -github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng= -github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= +github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg= +github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns= github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM= github.com/pulumi/pulumi-yaml v1.11.1 h1:ULUL9fpb2Bwgf3jJHx0FamKYm0ld0KxBQr/uSAslRLk= diff --git a/go.mod b/go.mod index fef0a9828..baf2d86e9 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,7 @@ require ( github.com/mitchellh/reflectwalk v1.0.2 github.com/pkg/errors v0.9.1 github.com/pulumi/inflector v0.1.1 - github.com/pulumi/providertest v0.1.2 + github.com/pulumi/providertest v0.1.3 github.com/pulumi/pulumi-java/pkg v0.16.1 github.com/pulumi/pulumi-yaml v1.11.1 github.com/pulumi/schema-tools v0.1.2 diff --git a/go.sum b/go.sum index e75d1d7be..c16eb0719 100644 --- a/go.sum +++ b/go.sum @@ -1932,8 +1932,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE= github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c= github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA= github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY= -github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng= -github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= +github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg= +github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns= github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM= github.com/pulumi/pulumi-yaml v1.11.1 h1:ULUL9fpb2Bwgf3jJHx0FamKYm0ld0KxBQr/uSAslRLk= diff --git a/pf/go.sum b/pf/go.sum index d02b9d021..b8a7b904c 100644 --- a/pf/go.sum +++ b/pf/go.sum @@ -712,8 +712,8 @@ github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE= github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c= github.com/pulumi/inflector v0.1.1 h1:dvlxlWtXwOJTUUtcYDvwnl6Mpg33prhK+7mzeF+SobA= github.com/pulumi/inflector v0.1.1/go.mod h1:HUFCjcPTz96YtTuUlwG3i3EZG4WlniBvR9bd+iJxCUY= -github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng= -github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= +github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg= +github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= github.com/pulumi/pulumi-java/pkg v0.16.1 h1:orHnDWFbpOERwaBLry9f+6nqPX7x0MsrIkaa5QDGAns= github.com/pulumi/pulumi-java/pkg v0.16.1/go.mod h1:QH0DihZkWYle9XFc+LJ76m4hUo+fA3RdyaM90pqOaSM= github.com/pulumi/pulumi-yaml v1.11.1 h1:ULUL9fpb2Bwgf3jJHx0FamKYm0ld0KxBQr/uSAslRLk= diff --git a/pkg/tests/schema_pulumi_test.go b/pkg/tests/schema_pulumi_test.go index f37eb33e4..20a994cd8 100644 --- a/pkg/tests/schema_pulumi_test.go +++ b/pkg/tests/schema_pulumi_test.go @@ -17,6 +17,7 @@ import ( "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/pulcheck" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tests/tfcheck" "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info" shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2" "github.com/pulumi/pulumi/sdk/v3/go/auto/optpreview" "github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh" @@ -4159,3 +4160,426 @@ func TestMakeTerraformResultNilVsEmptyMap(t *testing.T) { assert.True(t, props["test"].DeepEquals(emptyMap)) }) } + +func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { + collectionForceNewResource := func(typ schema.ValueType) *schema.Resource { + return &schema.Resource{ + Schema: map[string]*schema.Schema{ + "test": { + Type: typ, + Optional: true, + ForceNew: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeString, + Optional: true, + }, + }, + }, + }, + }, + } + } + + propertyForceNewResource := func(typ schema.ValueType) *schema.Resource { + return &schema.Resource{ + Schema: map[string]*schema.Schema{ + "test": { + Type: typ, + Optional: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeString, + Optional: true, + ForceNew: true, + }, + }, + }, + }, + }, + } + } + + auxResource := func(typ schema.ValueType) *schema.Resource { + return &schema.Resource{ + Schema: map[string]*schema.Schema{ + "aux": { + Type: typ, + Computed: true, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "prop": { + Type: schema.TypeString, + Computed: true, + }, + }, + }, + }, + }, + CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { + d.SetId("aux") + err := d.Set("aux", []map[string]interface{}{{"prop": "aux"}}) + require.NoError(t, err) + return nil + }, + } + } + + initialProgram := ` + name: test + runtime: yaml + resources: + mainRes: + type: prov:index:Test + properties: + tests: [{prop: 'value'}] +` + + program := ` + name: test + runtime: yaml + resources: + auxRes: + type: prov:index:Aux + mainRes: + type: prov:index:Test + properties: + tests: %s +` + + runTest := func(t *testing.T, program2 string, bridgedProvider info.Provider, expectedOutput autogold.Value) { + pt := pulcheck.PulCheck(t, bridgedProvider, initialProgram) + pt.Up(t) + pt.WritePulumiYaml(t, program2) + + res := pt.Preview(t, optpreview.Diff()) + + expectedOutput.Equal(t, res.StdOut) + } + + t.Run("list force new", func(t *testing.T) { + resMap := map[string]*schema.Resource{ + "prov_test": collectionForceNewResource(schema.TypeList), + "prov_aux": auxResource(schema.TypeList), + } + + tfp := &schema.Provider{ResourcesMap: resMap} + bridgedProvider := pulcheck.BridgedProvider(t, "prov", tfp) + runTest := func(t *testing.T, program2 string, expectedOutput autogold.Value) { + runTest(t, program2, bridgedProvider, expectedOutput) + } + + t.Run("unknown plain property", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[{prop: \"${auxRes.auxes[0].prop}\"}]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + ~ [0]: { + ~ prop: "value" => output + } + ] +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown object", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[\"${auxRes.auxes[0]}\"]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + - [0]: { + - prop: "value" + } + + [0]: output + ] +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown collection", func(t *testing.T) { + program2 := fmt.Sprintf(program, "\"${auxRes.auxes}\"") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + - tests: [ + - [0]: { + - prop: "value" + } + ] + + tests: output +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + }) + + t.Run("list property force new", func(t *testing.T) { + resMap := map[string]*schema.Resource{ + "prov_test": propertyForceNewResource(schema.TypeList), + "prov_aux": auxResource(schema.TypeList), + } + + tfp := &schema.Provider{ResourcesMap: resMap} + bridgedProvider := pulcheck.BridgedProvider(t, "prov", tfp) + runTest := func(t *testing.T, program2 string, expectedOutput autogold.Value) { + runTest(t, program2, bridgedProvider, expectedOutput) + } + + t.Run("unknown plain property", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[{prop: \"${auxRes.auxes[0].prop}\"}]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + ~ [0]: { + ~ prop: "value" => output + } + ] +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown object", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[\"${auxRes.auxes[0]}\"]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + - [0]: { + - prop: "value" + } + + [0]: output + ] +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown collection", func(t *testing.T) { + program2 := fmt.Sprintf(program, "\"${auxRes.auxes}\"") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + - tests: [ + - [0]: { + - prop: "value" + } + ] + + tests: output +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + }) + + t.Run("set force new", func(t *testing.T) { + resMap := map[string]*schema.Resource{ + "prov_test": collectionForceNewResource(schema.TypeSet), + "prov_aux": auxResource(schema.TypeSet), + } + + tfp := &schema.Provider{ResourcesMap: resMap} + bridgedProvider := pulcheck.BridgedProvider(t, "prov", tfp) + runTest := func(t *testing.T, program2 string, expectedOutput autogold.Value) { + runTest(t, program2, bridgedProvider, expectedOutput) + } + + t.Run("unknown plain property", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[{prop: \"${auxRes.auxes[0].prop}\"}]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + ~ [0]: { + ~ prop: "value" => output + } + ] +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown object", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[\"${auxRes.auxes[0]}\"]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + - [0]: { + - prop: "value" + } + + [0]: output + ] +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown collection", func(t *testing.T) { + program2 := fmt.Sprintf(program, "\"${auxRes.auxes}\"") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + - tests: [ + - [0]: { + - prop: "value" + } + ] + + tests: output +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + }) + + t.Run("set property force new", func(t *testing.T) { + resMap := map[string]*schema.Resource{ + "prov_test": propertyForceNewResource(schema.TypeSet), + "prov_aux": auxResource(schema.TypeSet), + } + + tfp := &schema.Provider{ResourcesMap: resMap} + bridgedProvider := pulcheck.BridgedProvider(t, "prov", tfp) + runTest := func(t *testing.T, program2 string, expectedOutput autogold.Value) { + runTest(t, program2, bridgedProvider, expectedOutput) + } + + t.Run("unknown plain property", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[{prop: \"${auxRes.auxes[0].prop}\"}]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + +-prov:index/test:Test: (replace) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + ~ [0]: { + ~ prop: "value" => output + } + ] +Resources: + + 1 to create + +-1 to replace + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown object", func(t *testing.T) { + program2 := fmt.Sprintf(program, "[\"${auxRes.auxes[0]}\"]") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + ~ tests: [ + - [0]: { + - prop: "value" + } + + [0]: output + ] +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + + t.Run("unknown collection", func(t *testing.T) { + program2 := fmt.Sprintf(program, "\"${auxRes.auxes}\"") + runTest(t, program2, autogold.Expect(`Previewing update (test): + pulumi:pulumi:Stack: (same) + [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + + prov:index/aux:Aux: (create) + [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] + ~ prov:index/test:Test: (update) + [id=newid] + [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] + - tests: [ + - [0]: { + - prop: "value" + } + ] + + tests: output +Resources: + + 1 to create + ~ 1 to update + 2 changes. 1 unchanged +`)) + }) + }) +} diff --git a/x/muxer/go.mod b/x/muxer/go.mod index 04324e372..eecb23ec5 100644 --- a/x/muxer/go.mod +++ b/x/muxer/go.mod @@ -7,7 +7,7 @@ toolchain go1.23.1 replace github.com/pulumi/pulumi-terraform-bridge/v3 => ../../ require ( - github.com/pulumi/providertest v0.1.2 + github.com/pulumi/providertest v0.1.3 github.com/pulumi/pulumi-terraform-bridge/v3 v3.0.0-00010101000000-000000000000 github.com/stretchr/testify v1.9.0 google.golang.org/protobuf v1.34.2 diff --git a/x/muxer/go.sum b/x/muxer/go.sum index 9c575a1e3..599fd9fff 100644 --- a/x/muxer/go.sum +++ b/x/muxer/go.sum @@ -154,8 +154,8 @@ github.com/pulumi/appdash v0.0.0-20231130102222-75f619a67231 h1:vkHw5I/plNdTr435 github.com/pulumi/appdash v0.0.0-20231130102222-75f619a67231/go.mod h1:murToZ2N9hNJzewjHBgfFdXhZKjY3z5cYC1VXk+lbFE= github.com/pulumi/esc v0.10.0 h1:jzBKzkLVW0mePeanDRfqSQoCJ5yrkux0jIwAkUxpRKE= github.com/pulumi/esc v0.10.0/go.mod h1:2Bfa+FWj/xl8CKqRTWbWgDX0SOD4opdQgvYSURTGK2c= -github.com/pulumi/providertest v0.1.2 h1:9pJS9MeNkMyGwyNeHmvh8QqLgJy39Nk2/ym5u7r13ng= -github.com/pulumi/providertest v0.1.2/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= +github.com/pulumi/providertest v0.1.3 h1:GpNKRy/haNjRHiUA9bi4diU4Op2zf3axYXbga5AepHg= +github.com/pulumi/providertest v0.1.3/go.mod h1:GcsqEGgSngwaNOD+kICJPIUQlnA911fGBU8HDlJvVL0= github.com/pulumi/pulumi/pkg/v3 v3.137.0 h1:/KPFQQaB5W0/GhVxSTGnEzv3ZW5uieGN5Q2q+Lsr+Zw= github.com/pulumi/pulumi/pkg/v3 v3.137.0/go.mod h1:ZQXJUTysDwq/mtilutRBKguH6DI+3b2WgNcOrs0whJ0= github.com/pulumi/pulumi/sdk/v3 v3.137.0 h1:bxhYpOY7Z4xt+VmezEpHuhjpOekkaMqOjzxFg/1OhCw= From cf8bebc4e6a11b64214b602e1ed63a00a8323539 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 28 Oct 2024 15:46:43 +0000 Subject: [PATCH 06/12] re-record tests under detailed diff v2 --- pkg/tests/schema_pulumi_test.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/pkg/tests/schema_pulumi_test.go b/pkg/tests/schema_pulumi_test.go index 20a994cd8..1666a5fa3 100644 --- a/pkg/tests/schema_pulumi_test.go +++ b/pkg/tests/schema_pulumi_test.go @@ -4162,6 +4162,9 @@ func TestMakeTerraformResultNilVsEmptyMap(t *testing.T) { } func TestUnknownCollectionForceNewDetailedDiff(t *testing.T) { + // TODO: Remove this once accurate bridge previews are rolled out + t.Setenv("PULUMI_TF_BRIDGE_ACCURATE_BRIDGE_PREVIEW", "true") + collectionForceNewResource := func(typ schema.ValueType) *schema.Resource { return &schema.Resource{ Schema: map[string]*schema.Schema{ @@ -4381,7 +4384,7 @@ Resources: [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - ~ prov:index/test:Test: (update) + +-prov:index/test:Test: (replace) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] ~ tests: [ @@ -4392,7 +4395,7 @@ Resources: ] Resources: + 1 to create - ~ 1 to update + +-1 to replace 2 changes. 1 unchanged `)) }) @@ -4404,7 +4407,7 @@ Resources: [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - ~ prov:index/test:Test: (update) + +-prov:index/test:Test: (replace) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] - tests: [ @@ -4415,7 +4418,7 @@ Resources: + tests: output Resources: + 1 to create - ~ 1 to update + +-1 to replace 2 changes. 1 unchanged `)) }) @@ -4543,7 +4546,7 @@ Resources: [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - ~ prov:index/test:Test: (update) + +-prov:index/test:Test: (replace) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] ~ tests: [ @@ -4554,7 +4557,7 @@ Resources: ] Resources: + 1 to create - ~ 1 to update + +-1 to replace 2 changes. 1 unchanged `)) }) @@ -4566,7 +4569,7 @@ Resources: [urn=urn:pulumi:test::test::pulumi:pulumi:Stack::test-test] + prov:index/aux:Aux: (create) [urn=urn:pulumi:test::test::prov:index/aux:Aux::auxRes] - ~ prov:index/test:Test: (update) + +-prov:index/test:Test: (replace) [id=newid] [urn=urn:pulumi:test::test::prov:index/test:Test::mainRes] - tests: [ @@ -4577,7 +4580,7 @@ Resources: + tests: output Resources: + 1 to create - ~ 1 to update + +-1 to replace 2 changes. 1 unchanged `)) }) From 7a26700b947b10daff47e45b7d649b181d45e724 Mon Sep 17 00:00:00 2001 From: Venelin Date: Mon, 28 Oct 2024 16:24:30 +0000 Subject: [PATCH 07/12] address review --- pkg/tfbridge/detailed_diff.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 6fe392b4d..faee52b50 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -43,6 +43,7 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { } func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { + contract.Assertf(!val.IsComputed() && !val.IsSecret(), "val should not be computed or secret") if !isPresent(val) { return false } @@ -65,7 +66,7 @@ func lookupSchemas( return LookupSchemas(schemaPath, tfs, ps) } -func willTriggerReplacement( +func propertyPathTriggersReplacement( path propertyPath, rootTFSchema shim.SchemaMap, rootPulumiSchema map[string]*info.Schema, ) bool { // A change on a property might trigger a replacement if: @@ -98,12 +99,12 @@ func willTriggerReplacement( return isForceNew(tfs, ps) } -func willTriggerReplacementRecursive( +func propertyValueTriggersReplacement( path propertyPath, value resource.PropertyValue, tfs shim.SchemaMap, ps map[string]*info.Schema, ) bool { replacement := false visitor := func(subpath resource.PropertyPath, val resource.PropertyValue) (resource.PropertyValue, error) { - if willTriggerReplacement(propertyPath(subpath), tfs, ps) { + if propertyPathTriggersReplacement(propertyPath(subpath), tfs, ps) { replacement = true } return val, nil @@ -255,7 +256,7 @@ func (differ detailedDiffer) makePlainPropDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { baseDiff := makeBaseDiff(old, new) - isReplacement := willTriggerReplacement(path, differ.tfs, differ.ps) + isReplacement := propertyPathTriggersReplacement(path, differ.tfs, differ.ps) var propDiff *pulumirpc.PropertyDiff if baseDiff != undecidedDiff { propDiff = baseDiff.ToPropertyDiff() @@ -288,11 +289,11 @@ func (differ detailedDiffer) makeShortCircuitDiff( contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind") propDiff := baseDiff.ToPropertyDiff() - if new.IsComputed() && willTriggerReplacement(path, differ.tfs, differ.ps) { + if new.IsComputed() && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) - } else if !new.IsNull() && !new.IsComputed() && willTriggerReplacementRecursive(path, new, differ.tfs, differ.ps) { + } else if !new.IsNull() && !new.IsComputed() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) - } else if !old.IsNull() && willTriggerReplacementRecursive(path, old, differ.tfs, differ.ps) { + } else if !old.IsNull() && propertyValueTriggersReplacement(path, old, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) } @@ -309,7 +310,7 @@ func (differ detailedDiffer) makePropDiff( if !isPresent(old) || isTypeShapeMismatched(old, propType) { old = resource.NewNullProperty() } - if !isPresent(new) || isTypeShapeMismatched(new, propType) && !new.IsComputed() { + if !new.IsComputed() && (!isPresent(new) || isTypeShapeMismatched(new, propType)) { new = resource.NewNullProperty() } if old.IsNull() || new.IsNull() || new.IsComputed() { From 563858713e8d00cff46e88752c30fe8488f25c75 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 29 Oct 2024 14:19:31 +0000 Subject: [PATCH 08/12] handle type mismatches better and test it --- pkg/tfbridge/detailed_diff.go | 9 ++- pkg/tfbridge/detailed_diff_test.go | 113 +++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index faee52b50..98cab88d6 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -307,10 +307,15 @@ func (differ detailedDiffer) makePropDiff( return nil } propType := differ.getEffectiveType(differ.propertyPathToSchemaPath(path)) - if !isPresent(old) || isTypeShapeMismatched(old, propType) { + + if isTypeShapeMismatched(old, propType) || isTypeShapeMismatched(new, propType) { + return differ.makePlainPropDiff(path, old, new) + } + + if !isPresent(old) { old = resource.NewNullProperty() } - if !new.IsComputed() && (!isPresent(new) || isTypeShapeMismatched(new, propType)) { + if !new.IsComputed() && !isPresent(new) { new = resource.NewNullProperty() } if old.IsNull() || new.IsNull() || new.IsComputed() { diff --git a/pkg/tfbridge/detailed_diff_test.go b/pkg/tfbridge/detailed_diff_test.go index 2ab8306fe..2dbda14ef 100644 --- a/pkg/tfbridge/detailed_diff_test.go +++ b/pkg/tfbridge/detailed_diff_test.go @@ -1747,3 +1747,116 @@ func TestDetailedDiffPulumiSchemaOverride(t *testing.T) { }) }) } + +func TestDetailedDiffMismatchedSchemas(t *testing.T) { + stringSchema := map[string]*schema.Schema{ + "foo": { + Type: schema.TypeString, + Optional: true, + }, + } + + listSchema := map[string]*schema.Schema{ + "foo": { + Type: schema.TypeList, + Optional: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + } + + setSchema := map[string]*schema.Schema{ + "foo": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + } + + mapSchema := map[string]*schema.Schema{ + "foo": { + Type: schema.TypeMap, + Optional: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + } + + stringValue := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": "string-value", + }, + ) + + listValue := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": []interface{}{"list-value"}, + }, + ) + + mapValue := resource.NewPropertyMapFromMap( + map[string]interface{}{ + "foo": map[string]interface{}{"bar": "map-value"}, + }, + ) + + t.Run("list schema with string value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(listSchema) + runDetailedDiffTest(t, stringValue, listValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("list schema with map value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(listSchema) + runDetailedDiffTest(t, mapValue, listValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("set schema with string value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(setSchema) + runDetailedDiffTest(t, stringValue, listValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("set schema with map value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(setSchema) + runDetailedDiffTest(t, mapValue, listValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("string schema with list value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(stringSchema) + runDetailedDiffTest(t, listValue, stringValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("string schema with map value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(stringSchema) + runDetailedDiffTest(t, mapValue, stringValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("map schema with string value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(mapSchema) + runDetailedDiffTest(t, stringValue, mapValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) + + t.Run("map schema with list value", func(t *testing.T) { + tfs := shimv2.NewSchemaMap(mapSchema) + runDetailedDiffTest(t, listValue, mapValue, tfs, nil, map[string]*pulumirpc.PropertyDiff{ + "foo": {Kind: pulumirpc.PropertyDiff_UPDATE}, + }) + }) +} From 8ea066090e7c2dff03929ef5e851ef29ad79847b Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 29 Oct 2024 14:30:00 +0000 Subject: [PATCH 09/12] handle property value outputs --- pkg/tfbridge/detailed_diff.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 98cab88d6..c2d70f1e4 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -43,7 +43,7 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { } func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { - contract.Assertf(!val.IsComputed() && !val.IsSecret(), "val should not be computed or secret") + contract.Assertf(!val.IsComputed() && !val.IsOutput() && !val.IsSecret(), "val should not be computed or secret") if !isPresent(val) { return false } @@ -181,7 +181,7 @@ func makeBaseDiff(old, new resource.PropertyValue) baseDiff { return deleteDiff } - if new.IsComputed() { + if new.IsComputed() || new.IsOutput() { return updateDiff } @@ -279,8 +279,8 @@ func (differ detailedDiffer) makePlainPropDiff( func (differ detailedDiffer) makeShortCircuitDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed(), - "short-circuit diff should only be used for nil properties") + contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed() || new.IsOutput(), + "short-circuit diff should only be used for nil, computed, or output properties") if old.IsNull() && new.IsNull() { return nil } @@ -289,9 +289,9 @@ func (differ detailedDiffer) makeShortCircuitDiff( contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind") propDiff := baseDiff.ToPropertyDiff() - if new.IsComputed() && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) { + if (new.IsComputed() || new.IsOutput()) && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) - } else if !new.IsNull() && !new.IsComputed() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) { + } else if !new.IsNull() && !new.IsComputed() && !new.IsOutput() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) } else if !old.IsNull() && propertyValueTriggersReplacement(path, old, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) @@ -315,10 +315,10 @@ func (differ detailedDiffer) makePropDiff( if !isPresent(old) { old = resource.NewNullProperty() } - if !new.IsComputed() && !isPresent(new) { + if !new.IsComputed() && !new.IsOutput() && !isPresent(new) { new = resource.NewNullProperty() } - if old.IsNull() || new.IsNull() || new.IsComputed() { + if old.IsNull() || new.IsNull() || new.IsComputed() || new.IsOutput() { return differ.makeShortCircuitDiff(path, old, new) } From 2ca5b2c7de18736a444ccac6bc141f0fef953ebc Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 29 Oct 2024 14:33:36 +0000 Subject: [PATCH 10/12] fix unknowns in typeShapemismatched --- pkg/tfbridge/detailed_diff.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index c2d70f1e4..22d9f2619 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -43,7 +43,10 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { } func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { - contract.Assertf(!val.IsComputed() && !val.IsOutput() && !val.IsSecret(), "val should not be computed or secret") + contract.Assertf(!val.IsSecret(), "val should not be secret") + if val.IsComputed() || val.IsOutput() { + return false + } if !isPresent(val) { return false } From 83dc15114cec196e7bfd266c1b8bf3fc785d5df7 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 29 Oct 2024 14:43:40 +0000 Subject: [PATCH 11/12] Revert "handle property value outputs" This reverts commit 8ea066090e7c2dff03929ef5e851ef29ad79847b. --- pkg/tfbridge/detailed_diff.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index 22d9f2619..d995c72c0 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -43,8 +43,8 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { } func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { - contract.Assertf(!val.IsSecret(), "val should not be secret") - if val.IsComputed() || val.IsOutput() { + contract.Assertf(!val.IsSecret()|| val.IsOutput(), "secrets and outputs are not handled") + if val.IsComputed() { return false } if !isPresent(val) { @@ -184,7 +184,7 @@ func makeBaseDiff(old, new resource.PropertyValue) baseDiff { return deleteDiff } - if new.IsComputed() || new.IsOutput() { + if new.IsComputed() { return updateDiff } @@ -282,8 +282,8 @@ func (differ detailedDiffer) makePlainPropDiff( func (differ detailedDiffer) makeShortCircuitDiff( path propertyPath, old, new resource.PropertyValue, ) map[detailedDiffKey]*pulumirpc.PropertyDiff { - contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed() || new.IsOutput(), - "short-circuit diff should only be used for nil, computed, or output properties") + contract.Assertf(old.IsNull() || new.IsNull() || new.IsComputed(), + "short-circuit diff should only be used for nil properties") if old.IsNull() && new.IsNull() { return nil } @@ -292,9 +292,9 @@ func (differ detailedDiffer) makeShortCircuitDiff( contract.Assertf(baseDiff != undecidedDiff, "short-circuit diff could not determine diff kind") propDiff := baseDiff.ToPropertyDiff() - if (new.IsComputed() || new.IsOutput()) && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) { + if new.IsComputed() && propertyPathTriggersReplacement(path, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) - } else if !new.IsNull() && !new.IsComputed() && !new.IsOutput() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) { + } else if !new.IsNull() && !new.IsComputed() && propertyValueTriggersReplacement(path, new, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) } else if !old.IsNull() && propertyValueTriggersReplacement(path, old, differ.tfs, differ.ps) { propDiff = promoteToReplace(propDiff) @@ -318,10 +318,10 @@ func (differ detailedDiffer) makePropDiff( if !isPresent(old) { old = resource.NewNullProperty() } - if !new.IsComputed() && !new.IsOutput() && !isPresent(new) { + if !new.IsComputed() && !isPresent(new) { new = resource.NewNullProperty() } - if old.IsNull() || new.IsNull() || new.IsComputed() || new.IsOutput() { + if old.IsNull() || new.IsNull() || new.IsComputed() { return differ.makeShortCircuitDiff(path, old, new) } From 749dfbb225c782b216315ac7a5318d9d190c5d69 Mon Sep 17 00:00:00 2001 From: Venelin Date: Tue, 29 Oct 2024 14:43:50 +0000 Subject: [PATCH 12/12] lint --- pkg/tfbridge/detailed_diff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/tfbridge/detailed_diff.go b/pkg/tfbridge/detailed_diff.go index d995c72c0..5e87e39f8 100644 --- a/pkg/tfbridge/detailed_diff.go +++ b/pkg/tfbridge/detailed_diff.go @@ -43,7 +43,7 @@ func sortedMergedKeys[K cmp.Ordered, V any, M ~map[K]V](a, b M) []K { } func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool { - contract.Assertf(!val.IsSecret()|| val.IsOutput(), "secrets and outputs are not handled") + contract.Assertf(!val.IsSecret() || val.IsOutput(), "secrets and outputs are not handled") if val.IsComputed() { return false }