Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502

Merged
merged 4 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions internal/testprovider/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -880,3 +880,21 @@ func AssertProvider(f func(data *schemav2.ResourceData)) *schemav2.Provider {
},
}
}

func CustomizedDiffProvider(f func(data *schemav2.ResourceData)) *schemav2.Provider {
return &schemav2.Provider{
Schema: map[string]*schemav2.Schema{},
ResourcesMap: map[string]*schemav2.Resource{
"test_resource": {
Schema: map[string]*schemav2.Schema{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably delete a lot here the test will only exercise CustomizeDiff

"labels": {Type: schemav2.TypeString, Optional: true, Computed: true},
},
SchemaVersion: 1,
CustomizeDiff: func(ctx context.Context, diff *schemav2.ResourceDiff, i interface{}) error {
err := diff.SetNew("labels", "1")
return err
},
},
},
}
}
5 changes: 5 additions & 0 deletions pkg/tfbridge/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ type ProviderInfo 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
}

// Send logs or status logs to the user.
Expand Down
11 changes: 11 additions & 0 deletions pkg/tfbridge/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,17 @@ func (p *Provider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*pulum
doIgnoreChanges(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, req.GetIgnoreChanges(), diff)
detailedDiff, changes := makeDetailedDiff(ctx, res.TF.Schema(), res.Schema.Fields, olds, news, diff)

// There are some providers/situations which `makeDetailedDiff` distorts the expected changes, leading
// to changes being dropped by Pulumi.
// Until we fix `makeDetailedDiff`, it is safer to refer to the Terraform Diff attribute length for setting
// the DiffResponse.
// 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 {
changes = pulumirpc.DiffResponse_DIFF_SOME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that it doesn't do the inverse, that is if len(diff.Attributes()) == 0 but makeDetailedDiff returned CHANGES, it does not override it back to NO_CHANGES. Is that perhaps a degenerate/ non-existent situation? Reading the code seems to be the case, so we're probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not verified such a situation as of today to the best of my knowledge.

}

// If there were changes in this diff, check to see if we have a replacement.
var replaces []string
var replaced map[string]bool
Expand Down
38 changes: 38 additions & 0 deletions pkg/tfbridge/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,44 @@ func TestTransformOutputs(t *testing.T) {
})
}

func TestSkipDetailedDiff(t *testing.T) {
provider := func(t *testing.T) *Provider {
p := testprovider.CustomizedDiffProvider(func(data *schema.ResourceData) {})
return &Provider{
tf: shimv2.NewProvider(p),
config: shimv2.NewSchemaMap(p.Schema),
resources: map[tokens.Type]Resource{
"TestResource": {
TF: shimv2.NewResource(p.ResourcesMap["test_resource"]),
TFName: "test_resource",
Schema: &ResourceInfo{
Tok: "TestResource",
},
},
},
info: ProviderInfo{
XSkipDetailedDiffForChanges: true,
},
}
}
t.Run("Diff", func(t *testing.T) {
testutils.Replay(t, provider(t), `
{
"method": "/pulumirpc.ResourceProvider/Diff",
"request": {
"id": "0",
"urn": "urn:pulumi:dev::teststack::TestResource::exres",
"olds": {},
"news": {}
},
"response": {
"changes": "DIFF_SOME",
"hasDetailedDiff": true
}
}`)
})
}

func TestTransformFromState(t *testing.T) {
provider := func(t *testing.T) *Provider {
p := testprovider.AssertProvider(func(data *schema.ResourceData) {
Expand Down