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.

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

Use list of maps for notification filters to allow better control and validation. #1542

Merged
merged 7 commits into from
Apr 4, 2022

Conversation

broswen
Copy link
Contributor

@broswen broswen commented Apr 1, 2022

Fixes #1189

This PR takes the changes made by 0gajun on #1520 and makes some final changes to the tests and code refactoring.

Note: I have commented out and removed the conditions argument due to lack of external documentation.

@jacobbednarz
Copy link
Member

thanks for the effort you've put in here however, i'm really not comfortable relying on TypeMaps. we had alot of internal issues with Terraform when using maps and we finally migrated away from them with the 3.x upgrade (see 3.x upgrade guide) and there hasn't been any changes in Terraform core that improved schema support, validation or helper methods so i'm not keen to re-introduce them for nested properties at this stage. fwiw, the nested TypeList or TypeSet actually map correctly to the API response here, TypeMap is a bit of munge to get it to work.

looks like there is an acceptance test failure too on the service side as well which we'll need to dig into.

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareNotificationPolicy_WithFiltersAttribute" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareNotificationPolicy_WithFiltersAttribute
    resource_cloudflare_notification_policy_test.go:88: Step 2/2 error: Error running apply: exit status 1

        Error: error updating notification policy 084abc3c3c4e435ca7434258d9dc2952: internal service error

          with cloudflare_notification_policy.ylezvrpogw,
          on terraform_plugin_test.tf line 2, in resource "cloudflare_notification_policy" "ylezvrpogw":
           2:   resource "cloudflare_notification_policy" "ylezvrpogw" {

--- FAIL: TestAccCloudflareNotificationPolicy_WithFiltersAttribute (24.76s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	25.335s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]
FAIL
make: *** [testacc] Error 1

@jacobbednarz
Copy link
Member

looks like we fixed the new test but the old one is broken with what appears to be a config issue

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareNotificationPolicy_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareNotificationPolicy_Basic
    resource_cloudflare_notification_policy_test.go:20: Step 2/2 error: Error running apply: exit status 1

        Error: error updating notification policy 568c216d01384658b625282a093b4f96: Invalid policy condition for alert type. (17007)

          with cloudflare_notification_policy.njfvpufpld,
          on terraform_plugin_test.tf line 2, in resource "cloudflare_notification_policy" "njfvpufpld":
           2:   resource "cloudflare_notification_policy" "njfvpufpld" {

--- FAIL: TestAccCloudflareNotificationPolicy_Basic (12.81s)
=== RUN   TestAccCloudflareNotificationPolicy_WithFiltersAttribute
--- PASS: TestAccCloudflareNotificationPolicy_WithFiltersAttribute (15.69s)
FAIL
FAIL	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	29.072s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]
FAIL
make: *** [testacc] Error 1

request

{
  "id": "568c216d01384658b625282a093b4f96",
  "name": "updated test SSL policy from terraform provider",
  "description": "updated description",
  "enabled": true,
  "alert_type": "universal_ssl_event_type",
  "mechanisms": {
    "email": [
      {
        "id": "[email protected]"
      }
    ]
  }
}

response

{
  "result": null,
  "success": false,
  "errors": [
    {
      "code": 17007,
      "message": "Invalid policy condition for alert type."
    }
  ],
  "messages": []
}

are we also able to update the filters documentation at https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/website/docs/r/notification_policy.html.markdown to clarify the available fields?

@broswen
Copy link
Contributor Author

broswen commented Apr 4, 2022

@jacobbednarz That's odd, those two acceptance tests work on my personal account. It looks like the conditions property needs to be set (to empty) even though it's not used for that alert type. The buildNotificationPolicy() function always sets that member to an empty map before sending. Not sure why it is not appearing in the JSON payload.

I added more documentation as requested and pulled a bunch of alert types and filters from the dashboard.

@jacobbednarz
Copy link
Member

we're good on the acceptance tests front, i may have been using an older cloudflare-go version

TF_ACC=1 go test $(go list ./...) -v -run "^TestAccCloudflareNotificationPolicy_" -count 1 -parallel 1 -timeout 120m -parallel 1
?   	github.com/cloudflare/terraform-provider-cloudflare	[no test files]
=== RUN   TestAccCloudflareNotificationPolicy_Basic
--- PASS: TestAccCloudflareNotificationPolicy_Basic (16.69s)
=== RUN   TestAccCloudflareNotificationPolicy_WithFiltersAttribute
--- PASS: TestAccCloudflareNotificationPolicy_WithFiltersAttribute (17.63s)
PASS
ok  	github.com/cloudflare/terraform-provider-cloudflare/cloudflare	34.827s
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/changelog-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/maintainer-only-file-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/tools/cmd/tf-log-check	[no test files]
?   	github.com/cloudflare/terraform-provider-cloudflare/version	[no test files]

@jacobbednarz
Copy link
Member

thanks for this @broswen 👏 let's get this merged and ready for the next release.

@jacobbednarz jacobbednarz merged commit 7a6bf78 into cloudflare:master Apr 4, 2022
@jacobbednarz jacobbednarz added this to the v3.12.0 milestone Apr 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

This functionality has been released in v3.12.0 of the Terraform Cloudflare Provider.

Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes on cloudflare_notification_policy
2 participants