From 3fc4e1808916efa214491dfa7959027c6ba613cf Mon Sep 17 00:00:00 2001 From: The Magician Date: Tue, 6 Feb 2024 13:55:25 -0800 Subject: [PATCH] fix(tgc): Fixing the if block for existingConverterAsset and add test case (#9914) (#1927) [upstream:25dd36888f9654efdd8440e661bc9961758f97fd] Signed-off-by: Modular Magician --- tfplan2cai/converters/google/convert.go | 15 +- tfplan2cai/converters/google/convert_test.go | 151 +++++++++++++++++-- 2 files changed, 148 insertions(+), 18 deletions(-) diff --git a/tfplan2cai/converters/google/convert.go b/tfplan2cai/converters/google/convert.go index 2d4c9d08b..97afbf578 100644 --- a/tfplan2cai/converters/google/convert.go +++ b/tfplan2cai/converters/google/convert.go @@ -172,7 +172,6 @@ func (c *Converter) addDelete(rc *tfjson.ResourceChange) error { } for _, converted := range convertedItems { - key := converted.Type + converted.Name var existingConverterAsset *resources.Asset if existing, exists := c.assets[key]; exists { @@ -190,14 +189,14 @@ func (c *Converter) addDelete(rc *tfjson.ResourceChange) error { } else { existingConverterAsset = &asset } - if existingConverterAsset != nil { - converted = converter.MergeDelete(*existingConverterAsset, converted) - augmented, err := c.augmentAsset(rd, c.cfg, converted) - if err != nil { - return err - } - c.assets[key] = augmented + } + if existingConverterAsset != nil { + converted = converter.MergeDelete(*existingConverterAsset, converted) + augmented, err := c.augmentAsset(rd, c.cfg, converted) + if err != nil { + return err } + c.assets[key] = augmented } } } diff --git a/tfplan2cai/converters/google/convert_test.go b/tfplan2cai/converters/google/convert_test.go index 5854e823c..c024896dd 100644 --- a/tfplan2cai/converters/google/convert_test.go +++ b/tfplan2cai/converters/google/convert_test.go @@ -12,7 +12,10 @@ import ( "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/caiasset" "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/tfplan2cai/ancestrymanager" resources "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/tfplan2cai/converters/google/resources" + "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/tfplan2cai/converters/google/resources/cai" + "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/tfplan2cai/converters/google/resources/services/resourcemanager" "github.com/GoogleCloudPlatform/terraform-google-conversion/v5/tfplan2cai/tfdata" + "github.com/google/go-cmp/cmp" tfjson "github.com/hashicorp/terraform-json" provider "github.com/hashicorp/terraform-provider-google-beta/google-beta/provider" "github.com/hashicorp/terraform-provider-google-beta/google-beta/tpgresource" @@ -635,18 +638,146 @@ func TestConvertWrapper(t *testing.T) { }, } for _, test := range tests { - _, err := convertWrapper(test.converter, test.rd, test.cfg) - if !test.wantErr { + t.Run(test.name, func(t *testing.T) { + _, err := convertWrapper(test.converter, test.rd, test.cfg) + if !test.wantErr { + if err != nil { + t.Errorf("convertWrapper() = %q, want = nil", err) + } + } else { + if err == nil { + t.Errorf("convertWrapper() = nil, want = %v", test.wantErrMsg) + } else if !strings.Contains(err.Error(), test.wantErrMsg) { + t.Errorf("convertWrapper() = %q, want containing %q", err, test.wantErrMsg) + } + } + }) + } +} + +func TestConvertMergingWithExistAsset(t *testing.T) { + tests := []struct { + name string + changes []*tfjson.ResourceChange + want []caiasset.Asset + }{ + { + name: "CreateOrUpdateOrNoop", + changes: []*tfjson.ResourceChange{ + { + Address: "google_project_iam_binding.test", + Mode: "managed", + Type: "google_project_iam_binding", + Name: "test", + ProviderName: "registry.terraform.io/hashicorp/google-beta", + Change: &tfjson.Change{ + Actions: tfjson.Actions{"create"}, + Before: nil, + After: map[string]interface{}{ + "members": []interface{}{ + "user:jane@example.com", + }, + "project": "example-project", + "role": "roles/editor", + }, + }, + }, + }, + want: []caiasset.Asset{ + { + Name: "//cloudresourcemanager.googleapis.com/projects/example-project", + Type: "cloudresourcemanager.googleapis.com/Project", + IAMPolicy: &caiasset.IAMPolicy{ + Bindings: []caiasset.IAMBinding{ + { + Role: "roles/editor", + Members: []string{"user:jane@example.com"}, + }, + }, + }, + }, + }, + }, + { + name: "Delete", + changes: []*tfjson.ResourceChange{ + { + Address: "google_project_iam_binding.test", + Mode: "managed", + Type: "google_project_iam_binding", + Name: "test", + ProviderName: "registry.terraform.io/hashicorp/google-beta", + Change: &tfjson.Change{ + Actions: tfjson.Actions{"delete"}, + After: nil, + Before: map[string]interface{}{ + "members": []interface{}{ + "user:jane@example.com", + }, + "project": "example-project", + "role": "roles/editor", + }, + }, + }, + }, + want: []caiasset.Asset{ + { + Name: "//cloudresourcemanager.googleapis.com/projects/example-project", + Type: "cloudresourcemanager.googleapis.com/Project", + IAMPolicy: &caiasset.IAMPolicy{}, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + converter, _, err := newTestConverter(false) + assert.Nil(t, err) + + fetchFuncCalled := 0 + fetchFunc := func(d tpgresource.TerraformResourceData, config *transport_tpg.Config) (cai.Asset, error) { + fetchFuncCalled++ + return resourcemanager.FetchProjectIamPolicy(d, config) + } + + converter.converters["google_project_iam_binding"] = []cai.ResourceConverter{ + { + AssetType: "cloudresourcemanager.googleapis.com/Project", + Convert: resourcemanager.GetProjectIamBindingCaiObject, + FetchFullResource: fetchFunc, + MergeCreateUpdate: resourcemanager.MergeProjectIamBinding, + MergeDelete: resourcemanager.MergeProjectIamBindingDelete, + }, + } + + // pre-populate the converter's asset cache + asset, err := converter.augmentAsset(nil, converter.cfg, cai.Asset{ + Name: "//cloudresourcemanager.googleapis.com/projects/example-project", + Type: "cloudresourcemanager.googleapis.com/Project", + IAMPolicy: &cai.IAMPolicy{ + Bindings: []cai.IAMBinding{ + { + Role: "roles/editor", + Members: []string{"user:abc@example.com"}, + }, + }, + }, + }) if err != nil { - t.Errorf("convertWrapper() = %q, want = nil", err) + t.Fatal(err) } - } else { - if err == nil { - t.Errorf("convertWrapper() = nil, want = %v", test.wantErrMsg) - } else if !strings.Contains(err.Error(), test.wantErrMsg) { - t.Errorf("convertWrapper() = %q, want containing %q", err, test.wantErrMsg) + converter.assets = map[string]Asset{ + "cloudresourcemanager.googleapis.com/Project//cloudresourcemanager.googleapis.com/projects/example-project": asset, } - } - } + err = converter.AddResourceChanges(test.changes) + if err != nil { + t.Fatalf("AddResourceChanges() = %s, want = nil", err) + } + if diff := cmp.Diff(test.want, converter.Assets()); diff != "" { + t.Errorf("AddResourceChanges() returned unexpected diff (-want +got):\n%s", diff) + } + assert.Equal(t, 0, fetchFuncCalled) + }) + } }