[PROPOSAL] Implement TypeSet resource.TestCheckFuncs #13556

appilon · 2020-05-29T21:31:48Z

This proposed PR would add 2 helper resource.TestCheckFunc that can substitute current checks that use a pre-calculated hash in the key. These functions essentially search all of the instance state for a schema.TypeSet element that matches either the simple value or a nested group of attr/value pairs.

In V2 of the SDK, the underlying test driver uses this library which naively exports TypeSets as a list so this implementation supports both "element ids" of either number or hash.

TypeSet indexes should be substituted with the sentinel value *, this works across deep nesting. TestCheckTypeSetElemNestedAttrs should be used with a granular value map to ensure an element is a full match.

Examples

Deep TypeSet simple element

test.TestCheckTypeSetElemAttr("foo.test", "cors_configuration.0.allow_headers.*", "Authorization"),

TypeSet nested object

test.TestCheckTypeSetElemNestedAttrs("foo.test", "parameter.*", map[string]string{
    "name":  "character_set_results",
    "value": "utf8",
})

TypeSet with downstream TypeList

test.TestCheckTypeSetElemNestedAttrs("foo.test", "origin.*", map[string]string{
    "custom_origin_config.0.http_port":  "80",
})

TypeSet with downstream TypeList with downstream TypeSet

test.TestCheckTypeSetElemNestedAttrs("foo.test", "parameter.*.lifecycle_rule.2.transition.*", map[string]string{
    "storage_class":  "INTELLIGENT_TIERING",
})

gdavison · 2020-06-01T19:27:36Z

This looks great, @appilon!

How does it handle collections nested inside a TypeSet? For example:

TestAccAWSCodeBuildProject_Source_Auth tests for nested values from a TypeSet inside another TypeSet
TestAccAWSMqBroker_allFieldsDefaultVpc tests for a TypeSet inside another TypeSet
TestAccAWSCloudFrontDistribution_noOptionalItemsConfig tests for a TypeList inside a TypeSet

I searched for "[a-z_]+\.[0-9]{10}\.[a-z_]+\.[0-9] (the unmatched " is expected)

ewbankkit · 2020-06-01T19:40:25Z

Definitely looking forward to this as I have to tweak another set of acceptance test values 😄.
Second the need to ideally extend this to nested collections.

appilon · 2020-06-01T21:04:49Z

Thanks @gdavison

TestAccAWSCloudFrontDistribution_noOptionalItemsConfig

This should just work, the "unflattening" part just lifts the path leading up to the Set into a map, so any state past that point should all "just work". I will look for a similar case and verify this, I would try this one but the test mentions deletion takes a while (if you have an example that you know runs relatively quickly please let me know).

TestAccAWSMqBroker_allFieldsDefaultVpc
TestAccAWSCodeBuildProject_Source_Auth

😂 I'm not sure how to handle this yet, was deliberately NOT looking for those cases, but I'll focus on this now.

appilon · 2020-06-03T00:53:42Z

@gdavison @ewbankkit The implementation has changed to support nesting sets.

gdavison · 2020-06-03T19:16:10Z

Is there a way to check the case of a TypeSet inside a TypeList where the list has multiple values? For example TestAccAWSS3Bucket_LifecycleBasic() and the tests for "lifecycle_rule.<index>.transition.<item>.<value>"

I'd like to be able to differentiate the list items, and I'm not sure how I'd do that using the depth parameter in TestCheckTypeSetElemNestedAttrs()

resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.2000431762.date", ""),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.2000431762.days", "30"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.2000431762.storage_class", "STANDARD_IA"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3601168188.date", ""),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3601168188.days", "60"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3601168188.storage_class", "INTELLIGENT_TIERING"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3854926587.date", ""),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3854926587.days", "90"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.3854926587.storage_class", "ONEZONE_IA"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.962205413.date", ""),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.962205413.days", "120"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.962205413.storage_class", "GLACIER"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.1571523406.date", ""),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.1571523406.days", "210"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.0.transition.1571523406.storage_class", "DEEP_ARCHIVE"),

resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.2.transition.460947558.days", "0"),

resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.4.transition.460947558.days", "0"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.4.transition.460947558.storage_class", "GLACIER"),

resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.5.transition.460947558.days", "0"),
resource.TestCheckResourceAttr(resourceName, "lifecycle_rule.5.transition.460947558.storage_class", "GLACIER"),

appilon · 2020-06-03T19:47:44Z

@gdavison Indeed it appears this implementation wouldn't be able to discern between those list items since the depth is the same 😢 . These are great examples though keep them coming, back to the drawing board!

appilon · 2020-06-09T02:54:55Z

@ewbankkit @gdavison oookay hopefully this covers all cases.

gdavison

Looks great! One question, because it looks like a test is changing

gdavison · 2020-06-09T21:13:22Z

aws/resource_aws_db_parameter_group_test.go

-					resource.TestCheckNoResourceAttr(resourceName, "parameter.2475805061.value"),
-					resource.TestCheckNoResourceAttr(resourceName, "parameter.1706463059.value"),
+					resource.TestCheckResourceAttr(resourceName, "parameter.#", "3"),
+					test.TestCheckTypeSetElemNestedAttrs(resourceName, "parameter.*", map[string]string{
+						"name":  "character_set_results",
+						"value": "utf8",
+					}),
+					test.TestCheckTypeSetElemNestedAttrs(resourceName, "parameter.*", map[string]string{
+						"name":  "character_set_server",
+						"value": "utf8",
+					}),
+					test.TestCheckTypeSetElemNestedAttrs(resourceName, "parameter.*", map[string]string{
+						"name":  "character_set_client",
+						"value": "utf8",
+					}),


This seems to be changing the test, since there were two items, and no values set for value, and now there are three and they have values.. Is that intentional?

Ah yes this is a bit quirky. There isn't really anyway to represent a specific set element that doesn't exist... My PR proposes helpers to guarantee a single element meets the desired criteria. So I flipped the test to "3 elements with the following property maps, and that the set only contains 3 entries (and not 5)"

I inferred that this is what was being validated by looking at the test config (goes from 5 parameters to 3)

appilon · 2020-06-12T20:02:48Z

Okay although flaky the last example you mentioned @gdavison has passed 🎉

❯ make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_LifecycleBasic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSS3Bucket_LifecycleBasic -timeout 120m
=== RUN   TestAccAWSS3Bucket_LifecycleBasic
=== PAUSE TestAccAWSS3Bucket_LifecycleBasic
=== CONT  TestAccAWSS3Bucket_LifecycleBasic
--- PASS: TestAccAWSS3Bucket_LifecycleBasic (118.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	121.065s

bflad

This should cover us for now! Two notes below, otherwise looks good to me. 🚀

bflad · 2020-06-15T11:50:58Z

aws/internal/test/testing.go

@@ -0,0 +1,114 @@
+package test


It would be super awesome if these new testing functions had unit testing so they can be updated without needing to verify with various acceptance tests. 👍 I'm also not the biggest fan of the package being named test since the function names already relay that (I'd suggest tfawsresource), but that's a personal nit since its internal to this module until it is migrated to the SDK.

no problem I can change the package name

bflad · 2020-06-15T11:51:48Z

aws/internal/test/testing.go

+)
+
+const (
+	sentinelIndex = "*"


Is it worth having the functions return an error if this sentinel value is not present or present more than once?

I can return error if the last part in the address isn't *, since it always should

Added check that the last part of a path is in fact the sentinel value

ghost · 2020-07-16T17:10:30Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

appilon requested a review from a team May 29, 2020 21:31

bflad mentioned this pull request Jun 1, 2020

Upgrade to Terraform Plugin SDK v2 #12087

Closed

18 tasks

appilon force-pushed the typeset-check branch from d67c362 to c19a2fc Compare June 1, 2020 20:57

appilon force-pushed the typeset-check branch from 697268a to fd1ed5f Compare June 9, 2020 02:43

gdavison approved these changes Jun 9, 2020

View reviewed changes

bflad mentioned this pull request Jun 12, 2020

resource/waf: remove unused params in methods #13674

Merged

appilon force-pushed the typeset-check branch from fd1ed5f to d71cbb9 Compare June 12, 2020 15:31

appilon force-pushed the typeset-check branch from d71cbb9 to 0cf3146 Compare June 12, 2020 20:04

ghost added the service/s3 Issues and PRs that pertain to the s3 service. label Jun 12, 2020

bflad approved these changes Jun 15, 2020

View reviewed changes

appilon added 2 commits June 15, 2020 12:56

Implement TypeSet resource.TestCheckFuncs

558a090

Add unit tests and rename pkg

3a56af6

Added check that the last part of a path is in fact the sentinel value

appilon force-pushed the typeset-check branch from 0cf3146 to 3a56af6 Compare June 15, 2020 18:00

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jun 15, 2020

appilon merged commit 844f6b9 into master Jun 15, 2020

appilon deleted the typeset-check branch June 15, 2020 20:04

bflad mentioned this pull request Jun 16, 2020

Db param group case sensitivity #12973

Closed

This was referenced Jun 17, 2020

TypeSetCheck: Treat unset values as matched #13807

Merged

Rewrite TypeSet checks with new helpers [Part 1] #13812

Merged

Rewrite TypeSet checks with new helpers [Part 2] #13831

Merged

appilon mentioned this pull request Jun 24, 2020

TypeSet is written with list indexes into the state and cannot be accessed hashicorp/terraform-plugin-sdk#196

Closed

ewbankkit mentioned this pull request Jul 5, 2020

r/aws_appmesh_route,r/aws_appmesh_virtual_node,r/aws_appmesh_virtual_router: Support gRPC and HTTP/2 services #11669

Merged

ghost locked and limited conversation to collaborators Jul 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROPOSAL] Implement TypeSet resource.TestCheckFuncs #13556

[PROPOSAL] Implement TypeSet resource.TestCheckFuncs #13556

appilon commented May 29, 2020 •

edited

Loading

gdavison commented Jun 1, 2020

ewbankkit commented Jun 1, 2020

appilon commented Jun 1, 2020 •

edited

Loading

appilon commented Jun 3, 2020

gdavison commented Jun 3, 2020

appilon commented Jun 3, 2020 •

edited

Loading

appilon commented Jun 9, 2020

gdavison left a comment

gdavison Jun 9, 2020

appilon Jun 9, 2020

appilon Jun 9, 2020

appilon commented Jun 12, 2020

bflad left a comment

bflad Jun 15, 2020

appilon Jun 15, 2020

bflad Jun 15, 2020

appilon Jun 15, 2020 •

edited

Loading

ghost commented Jul 16, 2020

[PROPOSAL] Implement TypeSet resource.TestCheckFuncs #13556

[PROPOSAL] Implement TypeSet resource.TestCheckFuncs #13556

Conversation

appilon commented May 29, 2020 • edited Loading

Examples

gdavison commented Jun 1, 2020

ewbankkit commented Jun 1, 2020

appilon commented Jun 1, 2020 • edited Loading

appilon commented Jun 3, 2020

gdavison commented Jun 3, 2020

appilon commented Jun 3, 2020 • edited Loading

appilon commented Jun 9, 2020

gdavison left a comment

Choose a reason for hiding this comment

gdavison Jun 9, 2020

Choose a reason for hiding this comment

appilon Jun 9, 2020

Choose a reason for hiding this comment

appilon Jun 9, 2020

Choose a reason for hiding this comment

appilon commented Jun 12, 2020

bflad left a comment

Choose a reason for hiding this comment

bflad Jun 15, 2020

Choose a reason for hiding this comment

appilon Jun 15, 2020

Choose a reason for hiding this comment

bflad Jun 15, 2020

Choose a reason for hiding this comment

appilon Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

ghost commented Jul 16, 2020

appilon commented May 29, 2020 •

edited

Loading

appilon commented Jun 1, 2020 •

edited

Loading

appilon commented Jun 3, 2020 •

edited

Loading

appilon Jun 15, 2020 •

edited

Loading