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

add delivery_property to azurerm_eventgrid_system_topic_event_subscription #15559

Merged
merged 24 commits into from
Mar 14, 2022

Conversation

jackmuskopf
Copy link
Contributor

Fix for issue #15540 based on pull request #13595

@jackmuskopf jackmuskopf changed the title add delivery_property add delivery_property to azurerm_eventgrid_system_topic_event_subscription Feb 22, 2022
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this @jackmuskopf, so far this looks good. Once a test is added for this property and the docs are updated this should be good to go!

@jackmuskopf
Copy link
Contributor Author

jackmuskopf commented Mar 4, 2022

Thanks for the review! Analogous tests as in #13595 have been added:

TF_ACC=1 go test -v ./internal/services/eventgrid -run=TestAccEventGridSystemTopicEventSubscription_deliveryProperties -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesStatic
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesStatic
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesSecret
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesSecret
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesMixed
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesMixed
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesUpdate
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesUpdate
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesForEventHubs
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesForEventHubs
=== RUN   TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesHybridRelay
=== PAUSE TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesHybridRelay
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesStatic
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesUpdate
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesHybridRelay
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesMixed
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesForEventHubs
=== CONT  TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesSecret
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesForEventHubs (266.39s)
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesHybridRelay (281.47s)
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesMixed (293.69s)
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesStatic (295.20s)
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesSecret (296.42s)
--- PASS: TestAccEventGridSystemTopicEventSubscription_deliveryPropertiesUpdate (302.51s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/eventgrid     302.529s

I also added changed the eventhub_endpoint doc note to azure_function_endpoint -- eventhub_endpoint block isn't supported for Event Grid System Topic (only eventhub_endpoint_id), and I think azure_function_endpoint block would meet the requirement of having at least one endpoint configured.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @jackmuskopf, the tests are passing. A few really minor comments on the docs and the terraform schema linting needs to be fixed then this can be merged!

@jackmuskopf
Copy link
Contributor Author

Thanks @stephybun ! I've run terrafmt on the test file and accepted all the suggestions to the docs - please let me know if there is anything else!

@katbyte
Copy link
Collaborator

katbyte commented Mar 8, 2022

running tests and got an error with 3.0 beta enabled:

------- Stdout: -------
=== RUN   TestAccEventGridSystemTopicEventSubscription_serviceBusTopicID
=== PAUSE TestAccEventGridSystemTopicEventSubscription_serviceBusTopicID
=== CONT  TestAccEventGridSystemTopicEventSubscription_serviceBusTopicID
    testcase.go:110: Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Missing required argument
        
          on terraform_plugin_test.tf line 18, in resource "azurerm_servicebus_topic" "test":
          18: resource "azurerm_servicebus_topic" "test" {
        
        The argument "namespace_id" is required, but no definition was found.
        
        Error: Unsupported argument
        
          on terraform_plugin_test.tf line 20, in resource "azurerm_servicebus_topic" "test":
          20:   resource_group_name = azurerm_resource_group.test.name
        
        An argument named "resource_group_name" is not expected here.
        
        Error: Unsupported argument
        
          on terraform_plugin_test.tf line 21, in resource "azurerm_servicebus_topic" "test":
          21:   namespace_name      = azurerm_servicebus_namespace.example.name
        
        An argument named "namespace_name" is not expected here.
--- FAIL: TestAccEventGridSystemTopicEventSubscription_serviceBusTopicID (14.34s)
FAIL

@jackmuskopf
Copy link
Contributor Author

Hi @katbyte thank you for identifying this! How can I run the tests with 3.0 beta enabled?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Tests are passing with 3.0 beta enabled. Thanks for this @jackmuskopf, LGTM 🍰

@stephybun stephybun merged commit 20a701f into hashicorp:main Mar 14, 2022
@github-actions github-actions bot added this to the v3.0.0 milestone Mar 14, 2022
stephybun added a commit that referenced this pull request Mar 14, 2022
@github-actions
Copy link

This functionality has been released in v3.0.0 of the Terraform 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!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2022
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.

3 participants