Skip to content

Commit

Permalink
Remove experimental flag for SkipDetailedDiff (#1893)
Browse files Browse the repository at this point in the history
Removes the flag option `XSkipDetailedDiffForChanges` and rolls out
behavior to all providers.

This change will break the next bridge release for pulumi-gcp and
pulumi-aws. When updating those providers to these changes, the
`XSkipDetailedDiffForChanges` provider info option must be removed.

Note that the maxItemsOne migration tests needed to be updated to show
that a change of `[]` to `""` reflects a diff on the field in question
now. This makes sense to me, since a nil slice is different from an
empty string, but this may actually not be desired behavior, so I'd
appreciate scrutiny here. Either way, the functionality under test there
requires a diff to happen on MaxItemsOne migrations and that does not
change.

Fixes #1501.
  • Loading branch information
guineveresaenger authored Apr 23, 2024
2 parents e71c1a6 + 682a19a commit c57799f
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 21 deletions.
19 changes: 8 additions & 11 deletions pkg/tfbridge/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2071,14 +2071,13 @@ func TestListNestedAddMaxItemsOne(t *testing.T) {
}

type diffTestCase struct {
resourceSchema map[string]*schema.Schema
resourceFields map[string]*SchemaInfo
state resource.PropertyMap
inputs resource.PropertyMap
expected map[string]*pulumirpc.PropertyDiff
expectedDiffChanges pulumirpc.DiffResponse_DiffChanges
ignoreChanges []string
XSkipDetailedDiffForChanges bool
resourceSchema map[string]*schema.Schema
resourceFields map[string]*SchemaInfo
state resource.PropertyMap
inputs resource.PropertyMap
expected map[string]*pulumirpc.PropertyDiff
expectedDiffChanges pulumirpc.DiffResponse_DiffChanges
ignoreChanges []string
}

func diffTest2(t *testing.T, tc diffTestCase) {
Expand All @@ -2100,7 +2099,6 @@ func diffTest2(t *testing.T, tc diffTestCase) {
p := Provider{
tf: provider,
info: ProviderInfo{
XSkipDetailedDiffForChanges: tc.XSkipDetailedDiffForChanges,
Resources: map[string]*ResourceInfo{
"p_resource": {
Tok: "pkg:index:PResource",
Expand Down Expand Up @@ -2151,8 +2149,7 @@ func TestChangingMaxItems1FilterProperty(t *testing.T) {
},
}
diffTest2(t, diffTestCase{
XSkipDetailedDiffForChanges: true,
resourceSchema: schema,
resourceSchema: schema,
state: resource.PropertyMap{
"rules": resource.NewArrayProperty(
[]resource.PropertyValue{
Expand Down
5 changes: 0 additions & 5 deletions pkg/tfbridge/info/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,6 @@ type Provider struct {
// See also: pulumi/pulumi-terraform-bridge#1448
SkipValidateProviderConfigForPluginFramework bool

// Disables using detailed diff to determine diff changes and falls back on the length of TF Diff Attributes.
//
// See https://github.com/pulumi/pulumi-terraform-bridge/issues/1501
XSkipDetailedDiffForChanges bool

// Enables generation of a trimmed, runtime-only metadata file
// to help reduce resource plugin start time
//
Expand Down
2 changes: 1 addition & 1 deletion pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
// We will still use `detailedDiff` for diff display purposes.

// See also https://github.com/pulumi/pulumi-terraform-bridge/issues/1501.
if p.info.XSkipDetailedDiffForChanges && len(diff.Attributes()) > 0 {
if len(diff.Attributes()) > 0 {
changes = pulumirpc.DiffResponse_DIFF_SOME
// Perhaps collectionDiffs can shed some light and locate the changes to the end-user.
for path, diff := range dd.collectionDiffs {
Expand Down
31 changes: 27 additions & 4 deletions pkg/tfbridge/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2156,9 +2156,6 @@ func TestSkipDetailedDiff(t *testing.T) {
Schema: &ResourceInfo{Tok: "Replace"},
},
},
info: ProviderInfo{
XSkipDetailedDiffForChanges: skipDetailedDiffForChanges,
},
}
}
t.Run("Diff", func(t *testing.T) {
Expand Down Expand Up @@ -2432,7 +2429,7 @@ func TestMaxItemOneWrongStateDiff(t *testing.T) {
"urn": "urn:pulumi:dev::teststack::NestedStrRes::exres",
"id": "0",
"olds": {
"nested_str": []
"nested_str": [""]
},
"news": {
"nested_str": ""
Expand All @@ -2444,6 +2441,32 @@ func TestMaxItemOneWrongStateDiff(t *testing.T) {
}
}`)
})
t.Run("DiffNilListAndVal", func(t *testing.T) {
testutils.Replay(t, provider, `
{
"method": "/pulumirpc.ResourceProvider/Diff",
"request": {
"urn": "urn:pulumi:dev::teststack::NestedStrRes::exres",
"id": "0",
"olds": {
"nested_str": []
},
"news": {
"nested_str": ""
}
},
"response": {
"changes": "DIFF_SOME",
"hasDetailedDiff": true,
"detailedDiff": {
"nested_str": {
"kind": "UPDATE"
}
},
"diffs": ["nested_str"]
}
}`)
})
t.Run("DiffListAndValNonEmpty", func(t *testing.T) {
testutils.Replay(t, provider, `
{
Expand Down

0 comments on commit c57799f

Please sign in to comment.