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.

Sign up for GitHub

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

Adding test coverage for changes to ignore data sources in imported state #1077

Merged
merged 3 commits into from
Oct 13, 2022
Merged

Adding test coverage for changes to ignore data sources in imported state #1077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6ed173
Adding test coverage for changes in https://github.com/hashicorp/terr…
bendbennett Oct 10, 2022
86ba558
Skips data sources from both original state and imported state
gdavison Oct 4, 2022
95a42a4
Adding changelog entry (#1066)
bendbennett Oct 13, 2022
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
3 changes: 3 additions & 0 deletions .changelog/1077.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
helper/resource: Fixed `TestStep` type `ImportStateVerify` field so that it only matches against resources following a change in behaviour in Terraform 1.3 that imports both resources and their dependent data sources
```
27 changes: 17 additions & 10 deletions helper/resource/testing_new_import_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,20 +158,27 @@ func testStepNewImportState(ctx context.Context, t testing.T, helper *plugintest
if step.ImportStateVerify {
logging.HelperResourceTrace(ctx, "Using TestStep ImportStateVerify")

newResources := importState.RootModule().Resources
oldResources := state.RootModule().Resources
// Ensure that we do not match against data sources as they
// cannot be imported and are not what we want to verify.
// Mode is not present in ResourceState so we use the
// stringified ResourceStateKey for comparison.
newResources := make(map[string]*terraform.ResourceState)
for k, v := range importState.RootModule().Resources {
if !strings.HasPrefix(k, "data.") {
newResources[k] = v
}
}
oldResources := make(map[string]*terraform.ResourceState)
for k, v := range state.RootModule().Resources {
if !strings.HasPrefix(k, "data.") {
oldResources[k] = v
}
}

for _, r := range newResources {
// Find the existing resource
var oldR *terraform.ResourceState
for r2Key, r2 := range oldResources {
// Ensure that we do not match against data sources as they
// cannot be imported and are not what we want to verify.
// Mode is not present in ResourceState so we use the
// stringified ResourceStateKey for comparison.
if strings.HasPrefix(r2Key, "data.") {
continue
}
for _, r2 := range oldResources {

if r2.Primary != nil && r2.Primary.ID == r.Primary.ID && r2.Type == r.Type && r2.Provider == r.Provider {
oldR = r2
Expand Down
249 changes: 216 additions & 33 deletions helper/resource/teststep_providers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1076,6 +1076,7 @@ func TestTest_TestStep_Taint(t *testing.T) {
}
}

//nolint:unparam
func extractResourceAttr(resourceName string, attributeName string, attributeValue *string) TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[resourceName]
Expand Down Expand Up @@ -1253,6 +1254,8 @@ func TestTest_TestStep_ProviderFactories_To_ExternalProviders(t *testing.T) {
}

func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) {
id := "none"

t.Parallel()

Test(t, TestCase{
Expand Down Expand Up @@ -1316,9 +1319,8 @@ func TestTest_TestStep_ProviderFactories_Import_Inline(t *testing.T) {
ImportStateId: "Z=:cbrJE?Ltg",
ImportStatePersist: true,
ImportStateCheck: composeImportStateCheck(
testCheckResourceAttrInstanceState("id", "none"),
testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"),
testCheckResourceAttrInstanceState("length", "12"),
testCheckResourceAttrInstanceState(&id, "result", "Z=:cbrJE?Ltg"),
testCheckResourceAttrInstanceState(&id, "length", "12"),
),
},
},
Expand Down Expand Up @@ -1391,7 +1393,7 @@ func TestTest_TestStep_ProviderFactories_Import_Inline_WithPersistMatch(t *testi
ImportStateId: "Z=:cbrJE?Ltg",
ImportStatePersist: true,
ImportStateCheck: composeImportStateCheck(
testExtractResourceAttrInstanceState("result", &result1),
testExtractResourceAttrInstanceState("none", "result", &result1),
),
},
{
Expand Down Expand Up @@ -1484,6 +1486,8 @@ func TestTest_TestStep_ProviderFactories_Import_Inline_WithoutPersist(t *testing
}

func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) {
id := "none"

t.Parallel()

Test(t, TestCase{
Expand All @@ -1500,9 +1504,8 @@ func TestTest_TestStep_ProviderFactories_Import_External(t *testing.T) {
ImportStateId: "Z=:cbrJE?Ltg",
ImportStatePersist: true,
ImportStateCheck: composeImportStateCheck(
testCheckResourceAttrInstanceState("id", "none"),
testCheckResourceAttrInstanceState("result", "Z=:cbrJE?Ltg"),
testCheckResourceAttrInstanceState("length", "12"),
testCheckResourceAttrInstanceState(&id, "result", "Z=:cbrJE?Ltg"),
testCheckResourceAttrInstanceState(&id, "length", "12"),
),
},
},
Expand All @@ -1528,7 +1531,7 @@ func TestTest_TestStep_ProviderFactories_Import_External_WithPersistMatch(t *tes
ImportStateId: "Z=:cbrJE?Ltg",
ImportStatePersist: true,
ImportStateCheck: composeImportStateCheck(
testExtractResourceAttrInstanceState("result", &result1),
testExtractResourceAttrInstanceState("none", "result", &result1),
),
},
{
Expand Down Expand Up @@ -1561,7 +1564,7 @@ func TestTest_TestStep_ProviderFactories_Import_External_WithoutPersistNonMatch(
ImportStateId: "Z=:cbrJE?Ltg",
ImportStatePersist: false,
ImportStateCheck: composeImportStateCheck(
testExtractResourceAttrInstanceState("result", &result1),
testExtractResourceAttrInstanceState("none", "result", &result1),
),
},
{
Expand Down Expand Up @@ -1714,6 +1717,188 @@ func TestTest_TestStep_ProviderFactories_RefreshWithPlanModifier_Inline(t *testi
})
}

func TestTest_TestStep_ProviderFactories_Import_Inline_With_Data_Source(t *testing.T) {
var id string

t.Parallel()

Test(t, TestCase{
ProviderFactories: map[string]func() (*schema.Provider, error){
"http": func() (*schema.Provider, error) { //nolint:unparam // required signature
return &schema.Provider{
DataSourcesMap: map[string]*schema.Resource{
"http": {
ReadContext: func(ctx context.Context, d *schema.ResourceData, i interface{}) (diags diag.Diagnostics) {
url := d.Get("url").(string)

responseHeaders := map[string]string{
"headerOne": "one",
"headerTwo": "two",
"headerThree": "three",
"headerFour": "four",
}
if err := d.Set("response_headers", responseHeaders); err != nil {
return append(diags, diag.Errorf("Error setting HTTP response headers: %s", err)...)
}

d.SetId(url)

return diags
},
Schema: map[string]*schema.Schema{
"url": {
Type: schema.TypeString,
Required: true,
},
"response_headers": {
Type: schema.TypeMap,
Computed: true,
Elem: &schema.Schema{
Type: schema.TypeString,
},
},
},
},
},
}, nil
},
"random": func() (*schema.Provider, error) { //nolint:unparam // required signature
return &schema.Provider{
ResourcesMap: map[string]*schema.Resource{
"random_string": {
CreateContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics {
d.SetId("none")
err := d.Set("length", 4)
if err != nil {
panic(err)
}
err = d.Set("result", "none")
if err != nil {
panic(err)
}
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(val)

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: `data "http" "example" {
url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"
}

resource "random_string" "example" {
length = length(data.http.example.response_headers)
}`,
Check: extractResourceAttr("random_string.example", "id", &id),
},
{
Config: `data "http" "example" {
url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"
}

resource "random_string" "example" {
length = length(data.http.example.response_headers)
}`,
ResourceName: "random_string.example",
ImportState: true,
ImportStateCheck: composeImportStateCheck(
testCheckResourceAttrInstanceState(&id, "length", "4"),
),
ImportStateVerify: true,
},
},
})
}

func TestTest_TestStep_ProviderFactories_Import_External_With_Data_Source(t *testing.T) {
var id string

t.Parallel()

Test(t, TestCase{
ExternalProviders: map[string]ExternalProvider{
"http": {
Source: "registry.terraform.io/hashicorp/http",
},
"random": {
Source: "registry.terraform.io/hashicorp/random",
},
},
Steps: []TestStep{
{
Config: `data "http" "example" {
url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"
}

resource "random_string" "example" {
length = length(data.http.example.response_headers)
}`,
Check: extractResourceAttr("random_string.example", "id", &id),
},
{
Config: `data "http" "example" {
url = "https://checkpoint-api.hashicorp.com/v1/check/terraform"
}

resource "random_string" "example" {
length = length(data.http.example.response_headers)
}`,
ResourceName: "random_string.example",
ImportState: true,
ImportStateCheck: composeImportStateCheck(
testCheckResourceAttrInstanceState(&id, "length", "12"),
),
ImportStateVerify: true,
},
},
})
}

func setTimeForTest(t time.Time) func() {
return func() {
getTimeForTest = func() time.Time {
Expand All @@ -1738,43 +1923,41 @@ func composeImportStateCheck(fs ...ImportStateCheckFunc) ImportStateCheckFunc {
}
}

func testExtractResourceAttrInstanceState(attributeName string, attributeValue *string) ImportStateCheckFunc {
func testExtractResourceAttrInstanceState(id, attributeName string, attributeValue *string) ImportStateCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Instead of fetching via the internal identifier (typically the id attribute, but not necessarily guaranteed) can we instead use the resource address string? e.g. random_string.example Reasons being:

  • This is how other testing framework "check" functions generally work
  • Resources such as this one may have duplicate resource instances with the same id
  • We're looking at making id less special in #1072

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how we use resource address when we're using a type ImportStateCheckFunc func([]*terraform.InstanceState) error as *terraform.InstanceState just has the ID and Attributes and doesn't appear to have any reference to the resource address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh, more trouble than its worth then (e.g. mapping those instance states with the resource addresses). We'll be removing these terraform bits when we take a look at the next iteration of the test framework and this isn't exported, so fine for now.

return func(is []*terraform.InstanceState) error {
if len(is) != 1 {
return fmt.Errorf("unexpected number of instance states: %d", len(is))
}
for _, v := range is {
if v.ID != id {
continue
}

s := is[0]
if attrVal, ok := v.Attributes[attributeName]; ok {
*attributeValue = attrVal

attrValue, ok := s.Attributes[attributeName]
if !ok {
return fmt.Errorf("attribute %s not found in instance state", attributeName)
return nil
}
}

*attributeValue = attrValue

return nil
return fmt.Errorf("attribute %s not found in instance state", attributeName)
}
}

func testCheckResourceAttrInstanceState(attributeName, attributeValue string) ImportStateCheckFunc {
func testCheckResourceAttrInstanceState(id *string, 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]
for _, v := range is {
if v.ID != *id {
continue
}

attrVal, ok := s.Attributes[attributeName]
if !ok {
return fmt.Errorf("attribute %s found in instance state", attributeName)
}
if attrVal, ok := v.Attributes[attributeName]; ok {
if attrVal != attributeValue {
return fmt.Errorf("expected: %s got: %s", attributeValue, attrVal)
}

if attrVal != attributeValue {
return fmt.Errorf("expected: %s got: %s", attributeValue, attrVal)
return nil
}
}

return nil
return fmt.Errorf("attribute %s not found in instance state", attributeName)
}
}

Expand Down