From 35f4e6f4ee9596a1ccde9b3308598bff57b3f662 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 5 Mar 2025 11:31:01 +0100 Subject: [PATCH 01/11] add test file for import block with id --- .../testing_new_import_block_id_test.go | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 helper/resource/testing_new_import_block_id_test.go diff --git a/helper/resource/testing_new_import_block_id_test.go b/helper/resource/testing_new_import_block_id_test.go new file mode 100644 index 000000000..377882cff --- /dev/null +++ b/helper/resource/testing_new_import_block_id_test.go @@ -0,0 +1,104 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package resource + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-go/tfprotov6" + "github.com/hashicorp/terraform-plugin-go/tftypes" + + "github.com/hashicorp/terraform-plugin-testing/internal/testing/testprovider" + "github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/providerserver" + "github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/resource" + "github.com/hashicorp/terraform-plugin-testing/tfversion" +) + +func TestTest_TestStep_ImportBlockId(t *testing.T) { + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_5_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "examplecloud": providerserver.NewProviderServer(testprovider.Provider{ + Resources: map[string]testprovider.Resource{ + "examplecloud_container": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + ImportStateResponse: &resource.ImportStateResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + { + Name: "location", + Type: tftypes.String, + Required: true, + }, + { + Name: "name", + Type: tftypes.String, + Required: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: ` + resource "examplecloud_container" "test" { + location = "westeurope" + name = "somevalue" + }`, + }, + { + ResourceName: "examplecloud_container.test", + ImportState: true, + ImportStateKind: ImportBlockWithId, + ImportStateVerify: true, + }, + }, + }) +} From 2761fa9f8cabc211a4e046acac5c4b87f0f5d7a6 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 5 Mar 2025 11:32:09 +0100 Subject: [PATCH 02/11] update testing of new import states to support generating import blocks with an id string and calling apply --- helper/resource/testing.go | 10 ++++ helper/resource/testing_new.go | 16 +----- helper/resource/testing_new_import_state.go | 55 ++++++++++++++++++--- 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 9e1961a46..e0cd642d8 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -454,6 +454,14 @@ type ExternalProvider struct { Source string // the provider source } +type ImportStateKind byte + +const ( + ImportCommandWithId ImportStateKind = iota + ImportBlockWithId + ImportBlockWithResourceIdentity +) + // TestStep is a single apply sequence of a test, done within the // context of a state. // @@ -633,6 +641,8 @@ type TestStep struct { // ID of that resource. ImportState bool + ImportStateKind ImportStateKind + // ImportStateId is the ID to perform an ImportState operation with. // This is optional. If it isn't set, then the resource ID is automatically // determined by inspecting the state for ResourceName's ID. diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index 0a7c7e7f7..8adeeb18c 100644 --- a/helper/resource/testing_new.go +++ b/helper/resource/testing_new.go @@ -133,7 +133,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest // use this to track last step successfully applied // acts as default for import tests - var appliedCfg teststep.Config + var appliedCfg string var stepNumber int for stepIndex, step := range c.Steps { @@ -426,7 +426,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest } } - mergedConfig, err := step.mergedConfig(ctx, c, hasTerraformBlock, hasProviderBlock, helper.TerraformVersion()) + appliedCfg, err = step.mergedConfig(ctx, c, hasTerraformBlock, hasProviderBlock, helper.TerraformVersion()) if err != nil { logging.HelperResourceError(ctx, @@ -436,18 +436,6 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest t.Fatalf("Error generating merged configuration: %s", err) } - confRequest := teststep.PrepareConfigurationRequest{ - Directory: step.ConfigDirectory, - File: step.ConfigFile, - Raw: mergedConfig, - TestStepConfigRequest: config.TestStepConfigRequest{ - StepNumber: stepIndex + 1, - TestName: t.Name(), - }, - }.Exec() - - appliedCfg = teststep.Configuration(confRequest) - logging.HelperResourceDebug(ctx, "Finished TestStep") continue diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 7dbc0b800..232e2cf46 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -6,6 +6,7 @@ package resource import ( "context" "fmt" + "github.com/hashicorp/terraform-exec/tfexec" "reflect" "strings" @@ -20,7 +21,7 @@ import ( "github.com/hashicorp/terraform-plugin-testing/internal/plugintest" ) -func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfg teststep.Config, providers *providerFactories, stepIndex int) error { +func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfgRaw string, providers *providerFactories, stepIndex int) error { t.Helper() configRequest := teststep.PrepareConfigurationRequest{ @@ -93,11 +94,40 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceTrace(ctx, fmt.Sprintf("Using import identifier: %s", importId)) + if testStepConfig == nil { + logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") + } + // Create working directory for import tests if testStepConfig == nil { logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") - testStepConfig = cfg + switch step.ImportStateKind { + case ImportBlockWithResourceIdentity: + t.Fatalf("TODO implement me") + case ImportBlockWithId: + cfgRaw += fmt.Sprintf(` + import { + to = %s + id = %q + } + `, step.ResourceName, importId) + default: + // Not an import block test so nothing to do here + } + + confRequest := teststep.PrepareConfigurationRequest{ + Directory: step.ConfigDirectory, + File: step.ConfigFile, + Raw: cfgRaw, + TestStepConfigRequest: config.TestStepConfigRequest{ + StepNumber: stepIndex + 1, + TestName: t.Name(), + }, + }.Exec() + + testStepConfig = teststep.Configuration(confRequest) + //testStepConfig = cfg if testStepConfig == nil { t.Fatal("Cannot import state with no specified config") } @@ -129,11 +159,22 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest } } - err = runProviderCommand(ctx, t, func() error { - return importWd.Import(ctx, step.ResourceName, importId) - }, importWd, providers) - if err != nil { - return err + if step.ImportStateKind == ImportBlockWithResourceIdentity || step.ImportStateKind == ImportBlockWithId { + var opts []tfexec.ApplyOption + + err = runProviderCommand(ctx, t, func() error { + return importWd.Apply(ctx, opts...) + }, importWd, providers) + if err != nil { + return err + } + } else { + err = runProviderCommand(ctx, t, func() error { + return importWd.Import(ctx, step.ResourceName, importId) + }, importWd, providers) + if err != nil { + return err + } } var importState *terraform.State From 8a9f46f8fcbaed7000b8fa2db69c4a2c27c3ef56 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 5 Mar 2025 11:49:59 +0100 Subject: [PATCH 03/11] import ordering --- helper/resource/testing_new_import_state.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 232e2cf46..0d5ae795f 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -6,19 +6,18 @@ package resource import ( "context" "fmt" - "github.com/hashicorp/terraform-exec/tfexec" "reflect" "strings" "github.com/google/go-cmp/cmp" "github.com/mitchellh/go-testing-interface" + "github.com/hashicorp/terraform-exec/tfexec" "github.com/hashicorp/terraform-plugin-testing/config" - "github.com/hashicorp/terraform-plugin-testing/internal/teststep" - "github.com/hashicorp/terraform-plugin-testing/terraform" - "github.com/hashicorp/terraform-plugin-testing/internal/logging" "github.com/hashicorp/terraform-plugin-testing/internal/plugintest" + "github.com/hashicorp/terraform-plugin-testing/internal/teststep" + "github.com/hashicorp/terraform-plugin-testing/terraform" ) func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfgRaw string, providers *providerFactories, stepIndex int) error { From 4f3dc6c3d0d29bd53f10d4def843cc575c6d3126 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 5 Mar 2025 11:52:20 +0100 Subject: [PATCH 04/11] remove duplicate nil check on testStepConfig --- helper/resource/testing_new_import_state.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 0d5ae795f..09cb40af1 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -93,10 +93,6 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceTrace(ctx, fmt.Sprintf("Using import identifier: %s", importId)) - if testStepConfig == nil { - logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") - } - // Create working directory for import tests if testStepConfig == nil { logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") From 7df59d797049133ba812a8e3c1a0f04d9e875e43 Mon Sep 17 00:00:00 2001 From: Steph Date: Wed, 5 Mar 2025 11:55:00 +0100 Subject: [PATCH 05/11] update comment --- helper/resource/testing_new_import_state.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 09cb40af1..0c83e6432 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -93,7 +93,7 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceTrace(ctx, fmt.Sprintf("Using import identifier: %s", importId)) - // Create working directory for import tests + // Prepare the test config dependent on the kind of import test being performed if testStepConfig == nil { logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") @@ -122,7 +122,6 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest }.Exec() testStepConfig = teststep.Configuration(confRequest) - //testStepConfig = cfg if testStepConfig == nil { t.Fatal("Cannot import state with no specified config") } From e3f2ff3302d28f138231903b8219e8feda5e54c6 Mon Sep 17 00:00:00 2001 From: Steph Date: Thu, 6 Mar 2025 08:36:20 +0100 Subject: [PATCH 06/11] replace instances of stepIndex + 1 with stepNumber --- helper/resource/testing_new.go | 4 ++-- helper/resource/testing_new_import_state.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/helper/resource/testing_new.go b/helper/resource/testing_new.go index 8adeeb18c..8e0bc2167 100644 --- a/helper/resource/testing_new.go +++ b/helper/resource/testing_new.go @@ -249,7 +249,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest File: step.ConfigFile, Raw: rawCfg, TestStepConfigRequest: config.TestStepConfigRequest{ - StepNumber: stepIndex + 1, + StepNumber: stepNumber, TestName: t.Name(), }, }.Exec() @@ -289,7 +289,7 @@ func runNewTest(ctx context.Context, t testing.T, c TestCase, helper *plugintest if step.ImportState { logging.HelperResourceTrace(ctx, "TestStep is ImportState mode") - err := testStepNewImportState(ctx, t, helper, wd, step, appliedCfg, providers, stepIndex) + err := testStepNewImportState(ctx, t, helper, wd, step, appliedCfg, providers, stepNumber) if step.ExpectError != nil { logging.HelperResourceDebug(ctx, "Checking TestStep ExpectError") if err == nil { diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 0c83e6432..94b2d492a 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -20,7 +20,7 @@ import ( "github.com/hashicorp/terraform-plugin-testing/terraform" ) -func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfgRaw string, providers *providerFactories, stepIndex int) error { +func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfgRaw string, providers *providerFactories, stepNumber int) error { t.Helper() configRequest := teststep.PrepareConfigurationRequest{ @@ -28,7 +28,7 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest File: step.ConfigFile, Raw: step.Config, TestStepConfigRequest: config.TestStepConfigRequest{ - StepNumber: stepIndex + 1, + StepNumber: stepNumber, TestName: t.Name(), }, }.Exec() @@ -116,7 +116,7 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest File: step.ConfigFile, Raw: cfgRaw, TestStepConfigRequest: config.TestStepConfigRequest{ - StepNumber: stepIndex + 1, + StepNumber: stepNumber, TestName: t.Name(), }, }.Exec() From df54519e8a55437b8ae48f9f508474498a0903a9 Mon Sep 17 00:00:00 2001 From: Steph Date: Mon, 10 Mar 2025 11:12:56 +0100 Subject: [PATCH 07/11] extend test cases for import block tests --- .../testing_new_import_block_id_test.go | 355 ++++++++++++++++++ 1 file changed, 355 insertions(+) diff --git a/helper/resource/testing_new_import_block_id_test.go b/helper/resource/testing_new_import_block_id_test.go index 377882cff..820fa1145 100644 --- a/helper/resource/testing_new_import_block_id_test.go +++ b/helper/resource/testing_new_import_block_id_test.go @@ -4,6 +4,10 @@ package resource import ( + "context" + "fmt" + "github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/datasource" + "github.com/hashicorp/terraform-plugin-testing/terraform" "testing" "github.com/hashicorp/terraform-plugin-go/tfprotov6" @@ -102,3 +106,354 @@ func TestTest_TestStep_ImportBlockId(t *testing.T) { }, }) } + +func TestTest_TestStep_ImportBlockId_SkipDataSourceState(t *testing.T) { + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_5_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "examplecloud": providerserver.NewProviderServer(testprovider.Provider{ + DataSources: map[string]testprovider.DataSource{ + "examplecloud_thing": { + ReadResponse: &datasource.ReadResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "datasource-test"), + }, + ), + }, + SchemaResponse: &datasource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + }, + }, + }, + }, + }, + }, + Resources: map[string]testprovider.Resource{ + "examplecloud_thing": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "resource-test"), + }, + ), + }, + ImportStateResponse: &resource.ImportStateResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "resource-test"), + }, + ), + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: ` + data "examplecloud_thing" "test" {} + resource "examplecloud_thing" "test" {} + `, + }, + { + ResourceName: "examplecloud_thing.test", + ImportState: true, + ImportStateKind: ImportBlockWithId, + ImportStateCheck: func(is []*terraform.InstanceState) error { + if len(is) > 1 { + return fmt.Errorf("expected 1 state, got: %d", len(is)) + } + + return nil + }, + }, + }, + }) +} + +func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *testing.T) { + /* + This test tries to imitate a real world example of behaviour we often see in the AzureRM provider which requires + the use of `ImportStateVerifyIgnore` when testing the import of a resource. + + A sensitive field e.g. a password can be supplied on create but isn't returned in the API response on a subsequent + read, resulting in a different value for password in the two states. + + In the AzureRM provider this is usually handled one of two ways, both requiring `ImportStateVerifyIgnore` to make + the test pass: + + 1. Property doesn't get set in the read + * in pluginSDK at create the config gets written to state because that's what we're expecting + * the subsequent read updates the values to create a post-apply diff and update computed values + * since we don't do anything to the property in the read the imported resource's state has the password missing + compared to the created resource's state + + 2. We retrieve the value from config and set that into state + * the config isn't available at import time using only the import command (I think?) so there is nothing to + retrieve and set into state when importing + + For this test to pass I needed to add a `PlanChangeFunc` to the resource to set the id to a known value in the plan - see comment in the `PlanChangeFunc` + + I also need to omit the `password` in the import config, otherwise the value in the config is used when importing the resource and the test + ends up passing regardless of whether `ImportStateVerifyIgnore` has been specified or not + + Ultimately it looks like: + * Terraform is saying there's a bug in the provider? (see comment in `PlanChangeFunc`) + * The import behaviour using a block vs. the command appears to differ + */ + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_5_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "examplecloud": providerserver.NewProviderServer(testprovider.Provider{ + Resources: map[string]testprovider.Resource{ + "examplecloud_container": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "name": tftypes.String, + "password": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "sometestid"), + "name": tftypes.NewValue(tftypes.String, "somename"), + "password": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + ImportStateResponse: &resource.ImportStateResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "name": tftypes.String, + "password": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "sometestid"), + "name": tftypes.NewValue(tftypes.String, "somename"), + "password": tftypes.NewValue(tftypes.String, nil), // this simulates an absent property when importing + }, + ), + }, + PlanChangeFunc: func(ctx context.Context, request resource.PlanChangeRequest, response *resource.PlanChangeResponse) { + /* + Returning a nil for another attribute to simulate a situation where `ImportStateVerifyIgnore` + should be used results in the error below from Terraform + + Error: Provider returned invalid result object after apply + + After the apply operation, the provider still indicated an unknown value for + examplecloud_container.test.id. All values must be known after apply, so this + is always a bug in the provider and should be reported in the provider's own + repository. Terraform will still save the other known object values in the + state. + + Modifying the plan to set the id to a known value appears to be the only way to + circumvent this behaviour, the cause of which I don't fully understand + + This doesn't seem great, because this gets applied to all Plans that happen in this + test - so we're modifying plans in steps that we might not want to. + */ + + objVal := map[string]tftypes.Value{} + + if !response.PlannedState.IsNull() { + _ = response.PlannedState.As(&objVal) + objVal["id"] = tftypes.NewValue(tftypes.String, "sometestid") + } + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + { + Name: "name", + Type: tftypes.String, + Required: true, + }, + { + Name: "password", + Type: tftypes.String, + Optional: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: ` + resource "examplecloud_container" "test" { + name = "somename" + password = "somevalue" + }`, + }, + { + Config: ` + terraform { + required_providers { + examplecloud = { + source = "registry.terraform.io/hashicorp/examplecloud" + } + } + } + + resource "examplecloud_container" "test" { + name = "somename" + } + + import { + to = examplecloud_container.test + id = "sometestid" + }`, + ResourceName: "examplecloud_container.test", + ImportState: true, + ImportStateKind: ImportBlockWithId, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"password"}, + }, + }, + }) +} + +func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore(t *testing.T) { + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_5_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "examplecloud": providerserver.NewProviderServer(testprovider.Provider{ + Resources: map[string]testprovider.Resource{ + "examplecloud_container": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "name": tftypes.String, + "password": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "sometestid"), + "name": tftypes.NewValue(tftypes.String, "somename"), + "password": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + ImportStateResponse: &resource.ImportStateResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "name": tftypes.String, + "password": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "sometestid"), + "name": tftypes.NewValue(tftypes.String, "somename"), + "password": tftypes.NewValue(tftypes.String, nil), // this simulates an absent property when importing + }, + ), + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + { + Name: "name", + Type: tftypes.String, + Computed: true, + }, + { + Name: "password", + Type: tftypes.String, + Computed: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: `resource "examplecloud_container" "test" {}`, + }, + { + ResourceName: "examplecloud_container.test", + ImportState: true, + ImportStateKind: ImportBlockWithId, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"password"}, + }, + }, + }) +} From 0c9036619c1377d525d86339bc3ad357fc9a7439 Mon Sep 17 00:00:00 2001 From: Steph Date: Tue, 11 Mar 2025 10:45:23 +0100 Subject: [PATCH 08/11] add comments to import kind enums --- helper/resource/testing.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index e0cd642d8..c7bdcd75e 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -457,8 +457,11 @@ type ExternalProvider struct { type ImportStateKind byte const ( + // ImportCommandWithId imports the state using the import command ImportCommandWithId ImportStateKind = iota + // ImportBlockWithId imports the state using an import block with an ID ImportBlockWithId + // ImportBlockWithResourceIdentity imports the state using an import block with a resource identity ImportBlockWithResourceIdentity ) From 6cb9373587f227df07a7c7aa07d8118238022ed2 Mon Sep 17 00:00:00 2001 From: Steph Date: Tue, 11 Mar 2025 11:05:29 +0100 Subject: [PATCH 09/11] add validation to prevent new import kinds to be used with config files or directories --- .../testing_new_import_block_id_test.go | 13 ++++------- helper/resource/testing_new_import_state.go | 22 ++++++++++++++----- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/helper/resource/testing_new_import_block_id_test.go b/helper/resource/testing_new_import_block_id_test.go index 820fa1145..669b36360 100644 --- a/helper/resource/testing_new_import_block_id_test.go +++ b/helper/resource/testing_new_import_block_id_test.go @@ -214,7 +214,7 @@ func TestTest_TestStep_ImportBlockId_SkipDataSourceState(t *testing.T) { func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *testing.T) { /* This test tries to imitate a real world example of behaviour we often see in the AzureRM provider which requires - the use of `ImportStateVerifyIgnore` when testing the import of a resource. + the use of `ImportStateVerifyIgnore` when testing the import of a resource using the import command. A sensitive field e.g. a password can be supplied on create but isn't returned in the API response on a subsequent read, resulting in a different value for password in the two states. @@ -297,7 +297,7 @@ func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *tes state. Modifying the plan to set the id to a known value appears to be the only way to - circumvent this behaviour, the cause of which I don't fully understand + circumvent this behaviour, the cause of which I don't fully understand. This doesn't seem great, because this gets applied to all Plans that happen in this test - so we're modifying plans in steps that we might not want to. @@ -351,18 +351,13 @@ func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *tes required_providers { examplecloud = { source = "registry.terraform.io/hashicorp/examplecloud" - } + } } } resource "examplecloud_container" "test" { name = "somename" - } - - import { - to = examplecloud_container.test - id = "sometestid" - }`, + }`, ResourceName: "examplecloud_container.test", ImportState: true, ImportStateKind: ImportBlockWithId, diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index 94b2d492a..c6d4f4816 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -23,6 +23,14 @@ import ( func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest.Helper, wd *plugintest.WorkingDir, step TestStep, cfgRaw string, providers *providerFactories, stepNumber int) error { t.Helper() + // Currently import modes `ImportBlockWithId` and `ImportBlockWithResourceIdentity` cannot support config file or directory + // since these modes append the import block to the configuration automatically + if step.ImportStateKind != ImportCommandWithId { + if step.ConfigFile != nil || step.ConfigDirectory != nil { + t.Fatalf("ImportStateKind %q is not supported for config file or directory", step.ImportStateKind) + } + } + configRequest := teststep.PrepareConfigurationRequest{ Directory: step.ConfigDirectory, File: step.ConfigFile, @@ -93,15 +101,19 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest logging.HelperResourceTrace(ctx, fmt.Sprintf("Using import identifier: %s", importId)) - // Prepare the test config dependent on the kind of import test being performed - if testStepConfig == nil { - logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") + if testStepConfig == nil || step.Config != "" { + importConfig := step.Config + if importConfig == "" { + logging.HelperResourceTrace(ctx, "Using prior TestStep Config for import") + importConfig = cfgRaw + } + // Update the test config dependent on the kind of import test being performed switch step.ImportStateKind { case ImportBlockWithResourceIdentity: t.Fatalf("TODO implement me") case ImportBlockWithId: - cfgRaw += fmt.Sprintf(` + importConfig += fmt.Sprintf(` import { to = %s id = %q @@ -114,7 +126,7 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest confRequest := teststep.PrepareConfigurationRequest{ Directory: step.ConfigDirectory, File: step.ConfigFile, - Raw: cfgRaw, + Raw: importConfig, TestStepConfigRequest: config.TestStepConfigRequest{ StepNumber: stepNumber, TestName: t.Name(), From 805da5ac7f2ec4553a24e2a8b7b8f6adff98faec Mon Sep 17 00:00:00 2001 From: Steph Date: Thu, 13 Mar 2025 09:10:15 +0100 Subject: [PATCH 10/11] run plan and check for no-op when importing via a block --- .../testing_new_import_block_id_test.go | 157 ++++++++++++++---- helper/resource/testing_new_import_state.go | 44 ++++- 2 files changed, 171 insertions(+), 30 deletions(-) diff --git a/helper/resource/testing_new_import_block_id_test.go b/helper/resource/testing_new_import_block_id_test.go index 669b36360..83cf63cd8 100644 --- a/helper/resource/testing_new_import_block_id_test.go +++ b/helper/resource/testing_new_import_block_id_test.go @@ -4,10 +4,10 @@ package resource import ( - "context" "fmt" "github.com/hashicorp/terraform-plugin-testing/internal/testing/testsdk/datasource" "github.com/hashicorp/terraform-plugin-testing/terraform" + "regexp" "testing" "github.com/hashicorp/terraform-plugin-go/tfprotov6" @@ -46,6 +46,126 @@ func TestTest_TestStep_ImportBlockId(t *testing.T) { }, ), }, + ReadResponse: &resource.ReadResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + ImportStateResponse: &resource.ImportStateResponse{ + State: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + SchemaResponse: &resource.SchemaResponse{ + Schema: &tfprotov6.Schema{ + Block: &tfprotov6.SchemaBlock{ + Attributes: []*tfprotov6.SchemaAttribute{ + { + Name: "id", + Type: tftypes.String, + Computed: true, + }, + { + Name: "location", + Type: tftypes.String, + Required: true, + }, + { + Name: "name", + Type: tftypes.String, + Required: true, + }, + }, + }, + }, + }, + }, + }, + }), + }, + Steps: []TestStep{ + { + Config: ` + resource "examplecloud_container" "test" { + location = "westeurope" + name = "somevalue" + }`, + }, + { + ResourceName: "examplecloud_container.test", + ImportState: true, + ImportStateKind: ImportBlockWithId, + ImportStateVerify: true, + }, + }, + }) +} + +func TestTest_TestStep_ImportBlockId_ExpectError(t *testing.T) { + t.Parallel() + + UnitTest(t, TestCase{ + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_5_0), // ProtoV6ProviderFactories + }, + ProtoV6ProviderFactories: map[string]func() (tfprotov6.ProviderServer, error){ + "examplecloud": providerserver.NewProviderServer(testprovider.Provider{ + Resources: map[string]testprovider.Resource{ + "examplecloud_container": { + CreateResponse: &resource.CreateResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, + ReadResponse: &resource.ReadResponse{ + NewState: tftypes.NewValue( + tftypes.Object{ + AttributeTypes: map[string]tftypes.Type{ + "id": tftypes.String, + "location": tftypes.String, + "name": tftypes.String, + }, + }, + map[string]tftypes.Value{ + "id": tftypes.NewValue(tftypes.String, "westeurope/somevalue"), + "location": tftypes.NewValue(tftypes.String, "westeurope"), + "name": tftypes.NewValue(tftypes.String, "somevalue"), + }, + ), + }, ImportStateResponse: &resource.ImportStateResponse{ State: tftypes.NewValue( tftypes.Object{ @@ -98,10 +218,16 @@ func TestTest_TestStep_ImportBlockId(t *testing.T) { }`, }, { + Config: ` + resource "examplecloud_container" "test" { + location = "eastus" + name = "somevalue" + }`, ResourceName: "examplecloud_container.test", ImportState: true, ImportStateKind: ImportBlockWithId, ImportStateVerify: true, + ExpectError: regexp.MustCompile(`importing resource examplecloud_container.test should be a no-op, but got action update with plan(.?)`), }, }, }) @@ -211,6 +337,8 @@ func TestTest_TestStep_ImportBlockId_SkipDataSourceState(t *testing.T) { }) } +// These tests currently pass but only because the `getState` function which is used on the imported resource +// to do the state comparison doesn't return an error if there is no state in the working directory func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *testing.T) { /* This test tries to imitate a real world example of behaviour we often see in the AzureRM provider which requires @@ -283,33 +411,6 @@ func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *tes }, ), }, - PlanChangeFunc: func(ctx context.Context, request resource.PlanChangeRequest, response *resource.PlanChangeResponse) { - /* - Returning a nil for another attribute to simulate a situation where `ImportStateVerifyIgnore` - should be used results in the error below from Terraform - - Error: Provider returned invalid result object after apply - - After the apply operation, the provider still indicated an unknown value for - examplecloud_container.test.id. All values must be known after apply, so this - is always a bug in the provider and should be reported in the provider's own - repository. Terraform will still save the other known object values in the - state. - - Modifying the plan to set the id to a known value appears to be the only way to - circumvent this behaviour, the cause of which I don't fully understand. - - This doesn't seem great, because this gets applied to all Plans that happen in this - test - so we're modifying plans in steps that we might not want to. - */ - - objVal := map[string]tftypes.Value{} - - if !response.PlannedState.IsNull() { - _ = response.PlannedState.As(&objVal) - objVal["id"] = tftypes.NewValue(tftypes.String, "sometestid") - } - }, SchemaResponse: &resource.SchemaResponse{ Schema: &tfprotov6.Schema{ Block: &tfprotov6.SchemaBlock{ diff --git a/helper/resource/testing_new_import_state.go b/helper/resource/testing_new_import_state.go index c6d4f4816..96be26543 100644 --- a/helper/resource/testing_new_import_state.go +++ b/helper/resource/testing_new_import_state.go @@ -6,6 +6,7 @@ package resource import ( "context" "fmt" + tfjson "github.com/hashicorp/terraform-json" "reflect" "strings" @@ -166,14 +167,53 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest } if step.ImportStateKind == ImportBlockWithResourceIdentity || step.ImportStateKind == ImportBlockWithId { - var opts []tfexec.ApplyOption + var opts []tfexec.PlanOption err = runProviderCommand(ctx, t, func() error { - return importWd.Apply(ctx, opts...) + return importWd.CreatePlan(ctx, opts...) }, importWd, providers) if err != nil { return err } + + var plan *tfjson.Plan + err = runProviderCommand(ctx, t, func() error { + var err error + plan, err = importWd.SavedPlan(ctx) + return err + }, importWd, providers) + if err != nil { + return err + } + + if plan.ResourceChanges != nil { + for _, rc := range plan.ResourceChanges { + if rc.Address != step.ResourceName { + // we're only interested in the changes for the resource being imported + continue + } + if rc.Change != nil && rc.Change.Actions != nil { + // should this be length checked and used as a condition, if it's a no-op then there shouldn't be any other changes here + for _, action := range rc.Change.Actions { + if action != "no-op" { + var stdout string + err = runProviderCommand(ctx, t, func() error { + var err error + stdout, err = importWd.SavedPlanRawStdout(ctx) + return err + }, importWd, providers) + if err != nil { + return fmt.Errorf("retrieving formatted plan output: %w", err) + } + + return fmt.Errorf("importing resource %s should be a no-op, but got action %s with plan \\nstdout:\\n\\n%s", rc.Address, action, stdout) + } + } + } + } + } + + // TODO compare plan to state from previous step } else { err = runProviderCommand(ctx, t, func() error { return importWd.Import(ctx, step.ResourceName, importId) From 1b4767178c4323eeadfaa4a7785f9fc8343dddd7 Mon Sep 17 00:00:00 2001 From: Steph Date: Thu, 13 Mar 2025 14:35:43 +0100 Subject: [PATCH 11/11] update comment on real example test to reflect current understanding --- .../testing_new_import_block_id_test.go | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/helper/resource/testing_new_import_block_id_test.go b/helper/resource/testing_new_import_block_id_test.go index 83cf63cd8..51219b77b 100644 --- a/helper/resource/testing_new_import_block_id_test.go +++ b/helper/resource/testing_new_import_block_id_test.go @@ -341,33 +341,27 @@ func TestTest_TestStep_ImportBlockId_SkipDataSourceState(t *testing.T) { // to do the state comparison doesn't return an error if there is no state in the working directory func TestTest_TestStep_ImportBlockId_ImportStateVerifyIgnore_Real_Example(t *testing.T) { /* - This test tries to imitate a real world example of behaviour we often see in the AzureRM provider which requires - the use of `ImportStateVerifyIgnore` when testing the import of a resource using the import command. + This test tries to imitate a real world example of behaviour we often see in the AzureRM provider which requires + the use of `ImportStateVerifyIgnore` when testing the import of a resource using the import command. - A sensitive field e.g. a password can be supplied on create but isn't returned in the API response on a subsequent - read, resulting in a different value for password in the two states. + A sensitive field e.g. a password can be supplied on create but isn't returned in the API response on a subsequent + read, resulting in a different value for password in the two states. - In the AzureRM provider this is usually handled one of two ways, both requiring `ImportStateVerifyIgnore` to make - the test pass: + In the AzureRM provider this is usually handled one of two ways, both requiring `ImportStateVerifyIgnore` to make + the test pass: - 1. Property doesn't get set in the read - * in pluginSDK at create the config gets written to state because that's what we're expecting - * the subsequent read updates the values to create a post-apply diff and update computed values - * since we don't do anything to the property in the read the imported resource's state has the password missing - compared to the created resource's state + 1. Property doesn't get set in the read + * in pluginSDK at create the config gets written to state because that's what we're expecting + * the subsequent read updates the values to create a post-apply diff and update computed values + * since we don't do anything to the property in the read the imported resource's state has the password missing + compared to the created resource's state - 2. We retrieve the value from config and set that into state - * the config isn't available at import time using only the import command (I think?) so there is nothing to - retrieve and set into state when importing + 2. We retrieve the value from config and set that into state + * the config isn't available at import time using only the import command (I think?) so there is nothing to + retrieve and set into state when importing - For this test to pass I needed to add a `PlanChangeFunc` to the resource to set the id to a known value in the plan - see comment in the `PlanChangeFunc` - - I also need to omit the `password` in the import config, otherwise the value in the config is used when importing the resource and the test - ends up passing regardless of whether `ImportStateVerifyIgnore` has been specified or not - - Ultimately it looks like: - * Terraform is saying there's a bug in the provider? (see comment in `PlanChangeFunc`) - * The import behaviour using a block vs. the command appears to differ + I also need to omit the `password` in the import config, otherwise the value in the config is used when importing the + with an import block and the test ends up passing regardless of whether `ImportStateVerifyIgnore` has been specified or not */ t.Parallel()