Update Resource: Add advanced filter support to azurerm_eventgrid_event_subscription

[jrauschenbusch]

Adds advanced_filter support in resource azurerm_eventgrid_event_subscription.

Fixes #3452

[tombuildsstuff]

hey @jrauschenbusch

Thanks for this PR :)

Taking a look through whilst this looks pretty good and works today, unfortunately reusing a single schema field for multiple times will break in a future version of Terraform - and as such we'll need to take a different approach here. Instead I'd suggest introducing a nested block for each type of variable, for example:

advanced_filter {
  integer {
    key = 3
    operator_type = "LessThan"
    values = [1, 2, 3]
  }

  # or boolean { .. }
  # or string { .. }
}

Which allows us to have more granular type information here - and will work going forwards - as such could we update this to use that approach? From a validation side we can then use the "expand" method of the filter to check that only one of boolean, integer or string blocks are specified, which should give us the same thing - WDYT?

Thanks!

[jrauschenbusch]

@tombuildsstuff Now have changed everything to my latest proposal. Tested it locally. Everything seems to work and schema validation is now a lot easier than before.

[tombuildsstuff]

hey @jrauschenbusch

Thanks for pushing those changes - taking a look through this is looking great, if we can fix up a couple of panics then this otherwise LGTM 👍

Thanks!

[tombuildsstuff]

this (along with the other nested blocks) may want changing to a Set depending on how the API behaves with multiple elements, but this is fine for now 👍

[jrauschenbusch]

It seems that duplicate values are allowed (tested by myself). But in fact a set would make more sense.

[jrauschenbusch]

Unfortunately testing is a lot more complex using sets, as one has to know the computed hash value of the properties to test their values. Do you have an idea how to easily solve this?

[jrauschenbusch]

Hey @tombuildsstuff

I have fixed all PR notes as far as they were easy to implement. If there are other points, please feel free to fix them.

Thanks!

UPDATE: I've rebased this branch, so there is no update of the parser stuff anymore.

[tombuildsstuff]

one minor point - but this otherwise LGTM, thanks @jrauschenbusch 👍

[katbyte]

@jrauschenbusch, getting a test failure when trying to run the tests:

TestAccAzureRMEventGridEventSubscription_advancedFilter] [Test Output]
=== RUN   TestAccAzureRMEventGridEventSubscription_advancedFilter
=== PAUSE TestAccAzureRMEventGridEventSubscription_advancedFilter
=== CONT  TestAccAzureRMEventGridEventSubscription_advancedFilter
--- FAIL: TestAccAzureRMEventGridEventSubscription_advancedFilter (2.12s)
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x370a350]
goroutine 2273 [running]:
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/eventgrid.flattenEventGridEventSubscriptionAdvancedFilter(0xc001d1c300, 0x4595b23, 0xe, 0x3df2480)
	/opt/teamcity-agent/work/dbea4b84960ec72a/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/eventgrid/eventgrid_event_subscription_resource.go:1061 +0x220
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/eventgrid.resourceArmEventGridEventSubscriptionRead(0xc000d996c0, 0x3f430c0, 0xc00056f400, 0x0, 0x0)

[jrauschenbusch]

@katbyte Cannot reproduce it using the latest branch version (63afc0e). But indeed, using the branch version before applying the patch of @tombuildsstuff (933a608) this error is thrown.

[jrauschenbusch]

@tombuildsstuff any update here? does the ACC test still fail or is it ready to be merged?

[katbyte]

Just checked now, same test is failing with:

TestAccAzureRMEventGridEventSubscription_advancedFilter: testing.go:640: Step 0 error: config is invalid: Unsupported argument: An argument named "resource_group_name" is not expected here.

[jrauschenbusch]

@katbyte Oh, i'm so sorry. That was a mistake from my side. I'll invest a bit more time to fix and optimize the acceptance tests. I'll ping you back as soon as i'm done.

[jrauschenbusch]

@katbyte I've now fixed/extended the Acc tests and executed it with my Azure account.

Test result:

make acctests SERVICE='eventgrid' TESTARGS='-run=TestAccAzureRMEventGridEventSubscription_advancedFilter' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/eventgrid/tests/ -run=TestAccAzureRMEventGridEventSubscription_advancedFilter -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMEventGridEventSubscription_advancedFilter
=== PAUSE TestAccAzureRMEventGridEventSubscription_advancedFilter
=== CONT  TestAccAzureRMEventGridEventSubscription_advancedFilter
--- PASS: TestAccAzureRMEventGridEventSubscription_advancedFilter (252.65s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/eventgrid/tests     (cached)

Sorry for the circumstances.

[katbyte]

Thanks @jrauschenbusch! tests pass now 🙂

[ghost]

This has been released in version 2.13.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.13.0"
}
# ... other configuration ...

[ghost]

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!