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

Update Resource: Add advanced filter support to azurerm_eventgrid_event_subscription #6861

Conversation

jrauschenbusch
Copy link
Contributor

@jrauschenbusch jrauschenbusch commented May 11, 2020

Adds advanced_filter support in resource azurerm_eventgrid_event_subscription.

Fixes #3452

@jrauschenbusch jrauschenbusch changed the title r/eventgrid-event-subscription: advanced filter Update Resource: Add advanced filter support to azurerm_eventgrid_event_subscription May 12, 2020
Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"bool_equals": {
Type: schema.TypeList,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jrauschenbusch commented May 25, 2020

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.

@jrauschenbusch jrauschenbusch force-pushed the r-eventgrid-event-subscription-advanced-filter branch from a49d340 to 933a608 Compare May 25, 2020 15:55
Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tombuildsstuff tombuildsstuff added this to the v2.12.0 milestone May 25, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@katbyte
Copy link
Collaborator

katbyte commented May 28, 2020

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.

@katbyte katbyte modified the milestones: v2.12.0, v2.13.0 May 28, 2020
@jrauschenbusch
Copy link
Contributor Author

@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 jrauschenbusch force-pushed the r-eventgrid-event-subscription-advanced-filter branch from 63afc0e to 4aee0c4 Compare May 29, 2020 18:30
@jrauschenbusch
Copy link
Contributor Author

@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.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jrauschenbusch! tests pass now 🙂

@katbyte katbyte merged commit 786cf99 into hashicorp:master Jun 1, 2020
katbyte added a commit that referenced this pull request Jun 1, 2020
@ghost
Copy link

ghost commented Jun 4, 2020

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
Copy link

ghost commented Jul 2, 2020

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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jul 2, 2020
@jrauschenbusch jrauschenbusch deleted the r-eventgrid-event-subscription-advanced-filter branch July 9, 2020 05:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_eventgrid_event_subscription support advanced filtering
4 participants