query: update ExpectLength behaviour to support multiple list blocks#597
query: update ExpectLength behaviour to support multiple list blocks#597
ExpectLength behaviour to support multiple list blocks#597Conversation
… list blocks that are dynamically created
| // specifying a trailing '*' to indicate that we should be looking for multiple summaries | ||
|
|
||
| if !strings.HasSuffix(e.resourceAddress, "*") { | ||
| if strings.EqualFold(strings.TrimPrefix(summary.Address, "list."), e.resourceAddress) { |
There was a problem hiding this comment.
This change might affect some users.
Although the documentation examples for this check specify the resource address as some_resource.test users can specify list.some_resource.test without affecting the outcome of the result (although arguably the check was behaving incorrectly to begin with). Now users need to ensure that they're not specifying the list. prefix in the resource address.
There was a problem hiding this comment.
Very interesting 🤔 , the reverse (referencing list.<name-of-list-resource>.<alias>) actually makes more sense to me initially. Since you're asserting the list results itself, not the outputted resource type / alias (which technically, wouldn't have an alias until config has been generated)
| } | ||
| `, | ||
| QueryResultChecks: []querycheck.QueryResultCheck{ | ||
| querycheck.ExpectLength("examplecloud_containerette.test*", 12), |
There was a problem hiding this comment.
I have not read the entirety of this test suite, but why 12? Is that correct? Or is it wrong on purpose?
There was a problem hiding this comment.
I believe it's because examplecloud_containerette.test* is being used to match:
list.examplecloud_containerette.test== 6 instanceslist.examplecloud_containerette.test2== 6 instances
The confusing bit is that the resource config above doesn't really impact the list results, since they are just hardcoded in examplecloudListResource():
terraform-plugin-testing/helper/resource/query/examplecloud_list_test.go
Lines 33 to 230 in 08dd5f0
There was a problem hiding this comment.
Right, this test is trying to demonstrate that using the pattern some_list_resource.test* will correctly compare against a sum of multiple list blocks as opposed to just resources found by the last list block in the config, which is how the check currently behaves.
| total := 0 | ||
| for _, summary := range req.QuerySummaries { | ||
| // To support query tests where for_each is used to construct the list blocks dynamically (e.g. with child resources) we allow | ||
| // specifying a trailing '*' to indicate that we should be looking for multiple summaries |
There was a problem hiding this comment.
Would it make sense to use [*] instead of *? Indicating more clearly (imo) that we're looking for multiple keys for a specific list declaration
| } | ||
| } else { | ||
| // when using for_each summary.Address returns as `list.{resource_name}.{resource_label}[{each.key}]` | ||
| if strings.HasPrefix(strings.TrimPrefix(summary.Address, "list."), strings.TrimSuffix(e.resourceAddress, "*")) { |
There was a problem hiding this comment.
This could be a case of "just don't do that", but hypothetically if someone specifies a query config like the below, this check would return an unexpected count
Configuration
list "azurerm_virtual_network" "test" {
provider = azurerm
include_resource = true
config {
resource_group_name = "acctestRG-%d"
}
}
list "azurerm_subnet" "test" {
for_each = toset([for vnet in list.azurerm_virtual_network.test.data : vnet.state.id])
provider = azurerm
config {
virtual_network_id = each.key
}
}
list "azurerm_subnet" "test-single" {
provider = azurerm
config {
virtual_network_id = list.azurerm_virtual_network.test.data[0].state.id
}
}
Test Step
{
Query: true,
Config: r.multipleParentsQuery(data),
QueryResultChecks: []querycheck.QueryResultCheck{
querycheck.ExpectLength("azurerm_virtual_network.test", 2),
// this will return 8 due to the `test-single` summaries being included
querycheck.ExpectLength("azurerm_subnet.test*", 5),
querycheck.ExpectLength("azurerm_subnet.test-single", 3),
},
},
This could be solved by using [*] like mentioned above, and then this check becomes strings.HasPrefix(strings.TrimPrefix(summary.Address, "list."), strings.TrimSuffix(e.resourceAddress, "*]"))
(example in azurerm: resource test file & modified ExpectLength check)
|
Thanks for all the feedback and discussion. Two things need to happen
I'm going to open separate PRs for these two pieces of work then close this WIP. |
4 participants
Related Issue
Resource Test Config
List Test Config
Failing ExpectLength Checks
Example derived from https://github.com/hashicorp/terraform-provider-azurerm/blob/527c291f1dd717cab9bc7c36bfc8a4602b3f7708/internal/services/network/subnet_resource_list_test.go
Description
This PR proposes how we could update the behaviour of the
ExpectLengthcheck without causing disruption through a breaking change or through the replacement of this check.The config examples above highlight several shortcomings in the current design of the
ExpectLengthquerycheckfor_eachcannot be reliably checkedSeveral changes have been made to support both use cases, the second case in particular will likely be a highly common and popular way for users to query child resources.
I've chosen to deprecate the existing
QuerySummaryproperty in theCheckQueryRequestand introduced aQuerySummariesthat will collect all availabletjson.ListCompleteMessagesfrom the query command.The
ExpectLengthcheck has been updated to find the correct summary based on the provided resource address for validating the count.Furthermore we would need to provide a means for providers to indicate that we're computing the sum of multiple summaries, thus the suggestion to support the addition of a trailing
*on the resource address specified in the query check e.g.querycheck.ExpectLength("child.test*", 5)Whilst it isn't the most robust solution, it's a non-breaking option.
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.