From 6dc3fa393fe8e8e34790aa5343469be8cefb5f16 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 11:31:52 +0100 Subject: [PATCH 1/8] Use ImportStatePersist to preserve state generated by import operation (#717) --- helper/resource/testing.go | 6 ++++++ helper/resource/testing_new_import_state.go | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 53c3746d84d..e509586e350 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -564,6 +564,12 @@ type TestStep struct { ImportStateVerify bool ImportStateVerifyIgnore []string + // ImportStatePersist, if true, will update the persisted state with the + // state generated by the import operation (i.e., terraform import). When + // false (default) the state generated by the import operation is discarded + // at the end of the test step that is verifying import behavior. + ImportStatePersist bool + // ProviderFactories can be specified for the providers that are valid for // this TestStep. When providers are specified at the TestStep level, all // TestStep within a TestCase must declare providers. diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index ec61b055f3a..9b67439a06f 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -7,7 +7,7 @@ import ( "strings" "github.com/davecgh/go-spew/spew" - testing "github.com/mitchellh/go-testing-interface" + "github.com/mitchellh/go-testing-interface" "github.com/hashicorp/terraform-plugin-sdk/v2/internal/logging" "github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugintest" @@ -86,8 +86,17 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest t.Fatal("Cannot import state with no specified config") } } - importWd := helper.RequireNewWorkingDir(ctx, t) - defer importWd.Close() + + var importWd *plugintest.WorkingDir + + // Use the same working directory to persist the state from import + if step.ImportStatePersist { + importWd = wd + } else { + importWd = helper.RequireNewWorkingDir(ctx, t) + defer importWd.Close() + } + err = importWd.SetConfig(ctx, step.Config) if err != nil { t.Fatalf("Error setting test config: %s", err) From c8b634c8a3465dd3047c5237fd13685556cedec8 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 13:49:37 +0100 Subject: [PATCH 2/8] Removing additional call to Init as this has already been called in runNewTest and when called at this point will overwrite the contents of the .terraform directory with the current version of the provider (#717) --- helper/resource/testing_new_import_state.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 9b67439a06f..b86fc863133 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -104,13 +104,6 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceDebug(ctx, "Running Terraform CLI init and import") - err = runProviderCommand(ctx, t, func() error { - return importWd.Init(ctx) - }, importWd, providers) - if err != nil { - t.Fatalf("Error running init: %s", err) - } - err = runProviderCommand(ctx, t, func() error { return importWd.Import(ctx, step.ResourceName, importId) }, importWd, providers) From bf90ede6eca29c5184c772bfaff14ffff447137d Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 15:05:42 +0100 Subject: [PATCH 3/8] Adding CHANGELOG entry (#717) --- .changelog/1052.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/1052.txt diff --git a/.changelog/1052.txt b/.changelog/1052.txt new file mode 100644 index 00000000000..a54e1e9b514 --- /dev/null +++ b/.changelog/1052.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +helper/resource: Add ImportStatePersist to optionally persist state generated during import +``` From f286a7a3f911f0d1dc24d692a016f70e696ec6a0 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 16:48:47 +0100 Subject: [PATCH 4/8] Adding test coverage (#717) --- helper/resource/teststep_providers_test.go | 305 +++++++++++++++++++++ 1 file changed, 305 insertions(+) diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index f12b4dc323e..fac94bbac5e 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -569,3 +569,308 @@ func TestTest_TestStep_ProviderFactories_To_ExternalProviders(t *testing.T) { }, }) } + +func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { + t.Parallel() + + Test(t, TestCase{ + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ProviderFactories: map[string]func() (*schema.Provider, error){ + "random": func() (*schema.Provider, error) { //nolint:unparam // required signature + return &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "random_password": { + DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + ReadContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + Schema: map[string]*schema.Schema{ + "length": { + Required: true, + ForceNew: true, + Type: schema.TypeInt, + }, + "result": { + Type: schema.TypeString, + Computed: true, + Sensitive: true, + }, + + "id": { + Computed: true, + Type: schema.TypeString, + }, + }, + Importer: &schema.ResourceImporter{ + StateContext: func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + val := d.Id() + + d.SetId("none") + d.Set("result", val) + + return []*schema.ResourceData{d}, nil + }, + }, + }, + }, + }, nil + }, + }, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: true, + ImportStateCheck: composeImportStateCheck( + testCheckResourceAttrInstanceState("id", "none"), + testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"), + testCheckNoResourceAttrInstanceState("length"), + ), + }, + }, + }) +} + +func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { + var result1, result2 string + + t.Parallel() + + Test(t, TestCase{ + ProviderFactories: map[string]func() (*schema.Provider, error){ + "random": func() (*schema.Provider, error) { //nolint:unparam // required signature + return &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "random_password": { + CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { + d.SetId("none") + return nil + }, + DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + ReadContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + Schema: map[string]*schema.Schema{ + "length": { + Required: true, + ForceNew: true, + Type: schema.TypeInt, + }, + "result": { + Type: schema.TypeString, + Computed: true, + Sensitive: true, + }, + + "id": { + Computed: true, + Type: schema.TypeString, + }, + }, + Importer: &schema.ResourceImporter{ + StateContext: func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + val := d.Id() + + d.SetId("none") + d.Set("result", val) + + return []*schema.ResourceData{d}, nil + }, + }, + }, + }, + }, nil + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: true, + ImportStateCheck: composeImportStateCheck( + testExtractResourceAttrInstanceState("result", &result1), + ), + }, + { + Config: `resource "random_password" "test" { length = 12 }`, + Check: ComposeTestCheckFunc( + testExtractResourceAttr("random_password.test", "result", &result2), + testCheckAttributeValuesEqual(&result1, &result2), + ), + }, + }, + }) +} + +func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) { + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: true, + ImportStateCheck: composeImportStateCheck( + testCheckResourceAttrInstanceState("id", "none"), + testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"), + testCheckResourceAttrInstanceState("length", "12"), + ), + }, + }, + }) +} + +func TestTest_TestStep_ProviderFactories_Import_ExternalWithTest(t *testing.T) { + var result1, result2 string + + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: true, + ImportStateCheck: composeImportStateCheck( + testExtractResourceAttrInstanceState("result", &result1), + ), + }, + { + Config: `resource "random_password" "test" { length = 12 }`, + Check: ComposeTestCheckFunc( + testExtractResourceAttr("random_password.test", "result", &result2), + testCheckAttributeValuesEqual(&result1, &result2), + ), + }, + }, + }) +} + +func composeImportStateCheck(fs ...ImportStateCheckFunc) ImportStateCheckFunc { + return func(s []*terraform.InstanceState) error { + for i, f := range fs { + if err := f(s); err != nil { + return fmt.Errorf("check %d/%d error: %s", i+1, len(fs), err) + } + } + + return nil + } +} + +func testExtractResourceAttrInstanceState(attributeName string, attributeValue *string) ImportStateCheckFunc { + return func(is []*terraform.InstanceState) error { + if len(is) != 1 { + return fmt.Errorf("unexpected number of instance states: %d", len(is)) + } + + s := is[0] + + attrValue, ok := s.Attributes[attributeName] + if !ok { + return fmt.Errorf("attribute %s not found in instance state", attributeName) + } + + *attributeValue = attrValue + + return nil + } +} + +func testCheckNoResourceAttrInstanceState(attributeName string) ImportStateCheckFunc { + return func(is []*terraform.InstanceState) error { + if len(is) != 1 { + return fmt.Errorf("unexpected number of instance states: %d", len(is)) + } + + s := is[0] + + _, ok := s.Attributes[attributeName] + if ok { + return fmt.Errorf("attribute %s found in instance state", attributeName) + } + + return nil + } +} + +func testCheckResourceAttrInstanceState(attributeName, attributeValue string) ImportStateCheckFunc { + return func(is []*terraform.InstanceState) error { + if len(is) != 1 { + return fmt.Errorf("unexpected number of instance states: %d", len(is)) + } + + s := is[0] + + attrVal, ok := s.Attributes[attributeName] + if !ok { + return fmt.Errorf("attribute %s found in instance state", attributeName) + } + + if attrVal != attributeValue { + return fmt.Errorf("expected: %s got: %s", attributeValue, attrVal) + } + + return nil + } +} + +func testExtractResourceAttr(resourceName string, attributeName string, attributeValue *string) TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[resourceName] + + if !ok { + return fmt.Errorf("resource name %s not found in state", resourceName) + } + + attrValue, ok := rs.Primary.Attributes[attributeName] + + if !ok { + return fmt.Errorf("attribute %s not found in resource %s state", attributeName, resourceName) + } + + *attributeValue = attrValue + + return nil + } +} + +func testCheckAttributeValuesEqual(i *string, j *string) TestCheckFunc { + return func(s *terraform.State) error { + if testStringValue(i) != testStringValue(j) { + return fmt.Errorf("attribute values are different, got %s and %s", testStringValue(i), testStringValue(j)) + } + + return nil + } +} + +func testStringValue(sPtr *string) string { + if sPtr == nil { + return "" + } + + return *sPtr +} From d7ffdfcf39b30412a32d4159214a695e316ba960 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 16:55:01 +0100 Subject: [PATCH 5/8] Linting (#717) --- helper/resource/teststep_providers_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index fac94bbac5e..19814f982b2 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -610,7 +610,10 @@ func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { val := d.Id() d.SetId("none") - d.Set("result", val) + err := d.Set("result", val) + if err != nil { + panic(err) + } return []*schema.ResourceData{d}, nil }, @@ -677,7 +680,10 @@ func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { val := d.Id() d.SetId("none") - d.Set("result", val) + err := d.Set("result", val) + if err != nil { + panic(err) + } return []*schema.ResourceData{d}, nil }, From f833dec7fcfc3e940e7b9500b8b0898702beb8bc Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 17:37:52 +0100 Subject: [PATCH 6/8] Appears that init needs to be run within testStepNewImportState if we're not persisting the state generated by the import (#717) --- helper/resource/testing_new_import_state.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index b86fc863133..fc4ebc9cb0b 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -104,6 +104,15 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceDebug(ctx, "Running Terraform CLI init and import") + if !step.ImportStatePersist { + err = runProviderCommand(ctx, t, func() error { + return importWd.Init(ctx) + }, importWd, providers) + if err != nil { + t.Fatalf("Error running init: %s", err) + } + } + err = runProviderCommand(ctx, t, func() error { return importWd.Import(ctx, step.ResourceName, importId) }, importWd, providers) From c2594f0305eeec240546a7ee112de62c613dfd02 Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 17:43:04 +0100 Subject: [PATCH 7/8] Fixing test and adding a couple of additional tests to verify that import test works when ImportStatePersist is false (#717) --- helper/resource/teststep_providers_test.go | 143 ++++++++++++++++++++- 1 file changed, 136 insertions(+), 7 deletions(-) diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index 19814f982b2..a170246475c 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -610,11 +610,17 @@ func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { val := d.Id() d.SetId("none") + err := d.Set("result", val) if err != nil { panic(err) } + err = d.Set("length", len(val)) + if err != nil { + panic(err) + } + return []*schema.ResourceData{d}, nil }, }, @@ -630,14 +636,14 @@ func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) { ImportStateCheck: composeImportStateCheck( testCheckResourceAttrInstanceState("id", "none"), testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"), - testCheckNoResourceAttrInstanceState("length"), + testCheckResourceAttrInstanceState("length", "12"), ), }, }, }) } -func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { +func TestTest_TestStep_ProviderFactories_Import_Inline_WithPersistMatch(t *testing.T) { var result1, result2 string t.Parallel() @@ -648,10 +654,6 @@ func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { return &schema.Provider{ ResourcesMap: map[string]*schema.Resource{ "random_password": { - CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { - d.SetId("none") - return nil - }, DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { return nil }, @@ -680,11 +682,17 @@ func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { val := d.Id() d.SetId("none") + err := d.Set("result", val) if err != nil { panic(err) } + err = d.Set("length", len(val)) + if err != nil { + panic(err) + } + return []*schema.ResourceData{d}, nil }, }, @@ -715,6 +723,84 @@ func TestTest_TestStep_ProviderFactories_Import_InlineWithTest(t *testing.T) { }) } +func TestTest_TestStep_ProviderFactories_Import_Inline_WithoutPersist(t *testing.T) { + t.Parallel() + + Test(t, TestCase{ + ProviderFactories: map[string]func() (*schema.Provider, error){ + "random": func() (*schema.Provider, error) { //nolint:unparam // required signature + return &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "random_password": { + CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { + d.SetId("none") + return nil + }, + DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + ReadContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics { + return nil + }, + Schema: map[string]*schema.Schema{ + "length": { + Required: true, + ForceNew: true, + Type: schema.TypeInt, + }, + "result": { + Type: schema.TypeString, + Computed: true, + Sensitive: true, + }, + + "id": { + Computed: true, + Type: schema.TypeString, + }, + }, + Importer: &schema.ResourceImporter{ + StateContext: func(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + val := d.Id() + + d.SetId("none") + + err := d.Set("result", val) + if err != nil { + panic(err) + } + + err = d.Set("length", len(val)) + if err != nil { + panic(err) + } + + return []*schema.ResourceData{d}, nil + }, + }, + }, + }, + }, nil + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: false, + }, + { + Config: `resource "random_password" "test" { length = 12 }`, + Check: ComposeTestCheckFunc( + TestCheckNoResourceAttr("random_password.test", "result"), + ), + }, + }, + }) +} + func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) { t.Parallel() @@ -741,7 +827,7 @@ func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) { }) } -func TestTest_TestStep_ProviderFactories_Import_ExternalWithTest(t *testing.T) { +func TestTest_TestStep_ProviderFactories_Import_External_WithPersistMatch(t *testing.T) { var result1, result2 string t.Parallel() @@ -774,6 +860,39 @@ func TestTest_TestStep_ProviderFactories_Import_ExternalWithTest(t *testing.T) { }) } +func TestTest_TestStep_ProviderFactories_Import_External_WithoutPersistNonMatch(t *testing.T) { + var result1, result2 string + + t.Parallel() + + Test(t, TestCase{ + ExternalProviders: map[string]ExternalProvider{ + "random": { + Source: "registry.terraform.io/hashicorp/random", + }, + }, + Steps: []TestStep{ + { + Config: `resource "random_password" "test" { length = 12 }`, + ResourceName: "random_password.test", + ImportState: true, + ImportStateId: "Z=:cbrJE?Ltg", + ImportStatePersist: false, + ImportStateCheck: composeImportStateCheck( + testExtractResourceAttrInstanceState("result", &result1), + ), + }, + { + Config: `resource "random_password" "test" { length = 12 }`, + Check: ComposeTestCheckFunc( + testExtractResourceAttr("random_password.test", "result", &result2), + testCheckAttributeValuesDiffer(&result1, &result2), + ), + }, + }, + }) +} + func composeImportStateCheck(fs ...ImportStateCheckFunc) ImportStateCheckFunc { return func(s []*terraform.InstanceState) error { for i, f := range fs { @@ -873,6 +992,16 @@ func testCheckAttributeValuesEqual(i *string, j *string) TestCheckFunc { } } +func testCheckAttributeValuesDiffer(i *string, j *string) TestCheckFunc { + return func(s *terraform.State) error { + if testStringValue(i) == testStringValue(j) { + return fmt.Errorf("attribute values are the same") + } + + return nil + } +} + func testStringValue(sPtr *string) string { if sPtr == nil { return "" From 6f4efc463a9c1bde75fcde0d7bb47f2f1f5d97cf Mon Sep 17 00:00:00 2001 From: Benjamin Bennett Date: Wed, 7 Sep 2022 17:45:34 +0100 Subject: [PATCH 8/8] Linting (#717) --- helper/resource/teststep_providers_test.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/helper/resource/teststep_providers_test.go b/helper/resource/teststep_providers_test.go index a170246475c..6f4b01e49f0 100644 --- a/helper/resource/teststep_providers_test.go +++ b/helper/resource/teststep_providers_test.go @@ -924,23 +924,6 @@ func testExtractResourceAttrInstanceState(attributeName string, attributeValue * } } -func testCheckNoResourceAttrInstanceState(attributeName string) ImportStateCheckFunc { - return func(is []*terraform.InstanceState) error { - if len(is) != 1 { - return fmt.Errorf("unexpected number of instance states: %d", len(is)) - } - - s := is[0] - - _, ok := s.Attributes[attributeName] - if ok { - return fmt.Errorf("attribute %s found in instance state", attributeName) - } - - return nil - } -} - func testCheckResourceAttrInstanceState(attributeName, attributeValue string) ImportStateCheckFunc { return func(is []*terraform.InstanceState) error { if len(is) != 1 {