Fix unexpected crashes when using cloudflare_notification_policy with a filters attribute
#1520
Conversation
|
Oops! It looks like no changelog entry is attached to this PR. Please include a release note as described in https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/docs/changelog-process.md. Example: If you do not require a release note to be included, please add the |
0gajun
force-pushed
the
dont_use_map_for_non_primitive_values
branch
from
f4ca45d
to
9bf1bf9
Compare
|
can you please add a test covering the functionality change here? we'll also need to update the website documentation for this resource. |
Currently, TypeMap doesn't support non-primitive type values. ref : hashicorp/terraform-plugin-sdk#62 When using non-primitive values with TypeMap, terraform crashes at runtime with a following error message. ``` panic: Unknown validation type: 6 ``` This error is originated at https://github.com/hashicorp/terraform-plugin-sdk/blob/v2.11.0/helper/schema/schema.go#L2017 Only TypeBool, TypeInt, TypeFloat and TypeString are supported. So, in this change, use TypeList with new custom resource to store non-primitive values.
0gajun
force-pushed
the
dont_use_map_for_non_primitive_values
branch
from
9bf1bf9
to
7e1f0cd
Compare
|
Thanks for your comment 😄 I added a new acceptance test using a However, I have not run the acceptance test in my personal environment yet because the (I tested my code with development overrides for provider developers in my real dev environment, but I think acceptance tests should be run in an isolated environment only for the tests) Do you have any good idea to run the acceptance test by myself? |
|
i have a dedicated account for running the acceptance tests in. https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/GNUmakefile#L23 is the target (with a few other environment variables that i use) looks like these changes are breaking the existing test suite |
| @@ -155,7 +160,7 @@ func buildNotificationPolicy(d *schema.ResourceData) cloudflare.NotificationPoli | |||
| } | |||
|
|
|||
| if filters, ok := d.GetOk("filters"); ok { | |||
| notificationPolicy.Filters = filters.(map[string][]string) | |||
| notificationPolicy.Filters = filters.([]interface{})[0].(map[string][]string) | |||
There was a problem hiding this comment.
if i'm understanding this correctly, you'll need to build this map[string][]string instead of casting it. looping over filters and appending should do it.
Cause filters and its child attributes don't care about ordering of values.
|
Thanks for your kind review and running acceptance tests on your environment ! I changed filter's type from TypeList to TypeSet. Maybe this change fixes the broken existing test suite. Could you run the acceptance test again when you have time? |
| @@ -25,11 +25,26 @@ func resourceCloudflareNotificationPolicySchema() map[string]*schema.Schema { | |||
| Required: true, | |||
| }, | |||
| "filters": { | |||
| Type: schema.TypeMap, | |||
| Type: schema.TypeSet, | |||
There was a problem hiding this comment.
you still want this as a TypeList otherwise it won't be addressable as filters.0...
| Type: schema.TypeSet, | |
| Type: schema.TypeList, |
| resource.TestCheckResourceAttr(resourceName, "alert_type", "clickhouse_alert_fw_ent_anomaly"), | ||
| resource.TestCheckResourceAttr(resourceName, "account_id", accountID), | ||
| resource.TestCheckResourceAttr(resourceName, "filters.0.services.#", "1"), | ||
| resource.TestCheckResourceAttr(resourceName, "filters.0.services.0", "waf"), |
There was a problem hiding this comment.
services.0 isn't directly addressable as the index is a hash of the items; you'll need to use TestCheckTypeSetElemAttr instead.
|
superseded by #1542 |
Fixes #1189
Background
#1189
When running
terraform planforcloudflare_notification_policywith afiltersattribute, I encountered the same issue with the following stack trace.Currently, TypeMap doesn't support non-primitive type values.
ref : hashicorp/terraform-plugin-sdk#62
When using non-primitive values with TypeMap, terraform crashes at runtime with a following error message.
This error is originated at
https://github.com/hashicorp/terraform-plugin-sdk/blob/v2.11.0/helper/schema/schema.go#L2017
Only TypeBool, TypeInt, TypeFloat and TypeString are supported.
This is why terraform crashes.
So, in this change, use TypeList with new custom resource to store non-primitive values.
NOTE
I also couldn't find any documentation about a filter parameters. ( #1406 )
Therefore, currently this change doesn't support all possible filters. (now supports
zonesandservicesfilters only)Could you help me to support all possible filters, Cloudflare staffs?