-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support for azurerm_kusto_iothub_data_connection missing table_name, mapping_rule_name, data_format #12293
Support for azurerm_kusto_iothub_data_connection missing table_name, mapping_rule_name, data_format #12293
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @EliiseS, thanks for this PR, this is off to a great start!
I have a couple of comments inline below. We will also need to add some test coverage for these new properties. Looking at the existing test, I would suggest adding a TestAccKustoIotHubDataConnection_complete()
test function, and moving all the optional fields (including the existing event_system_properties
and the newly added ones here) to the "complete" config. That enables us to simultaneously test the simplest and most complex configurations.
Once the tests are added, I'll be happy to re-review and prepare this for merging. Thanks!
azurerm/internal/services/kusto/kusto_iothub_data_connection_resource.go
Show resolved
Hide resolved
azurerm/internal/services/kusto/kusto_iothub_data_connection_resource.go
Outdated
Show resolved
Hide resolved
Great, thanks for your feedback! I've added the test, let me know if you'd like anything else :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EliiseS Thanks for the updates, this is looking great. Apologies, in my last review I missed that we'll need to add the new properties to the documentation (see website/docs/r/kusto_iothub_data_connection.html.markdown
). There's also a linting error which looks like it's just alignment in your test config (see inline below). If you could update the docs, and the tests pass, this should be good to merge 👍
azurerm/internal/services/kusto/kusto_iothub_data_connection_resource_test.go
Outdated
Show resolved
Hide resolved
I've updated the docs and I've ever so slightly reordered the eventhub variables in the doc too, to match the Iot Hub |
The PR check passed @manicminer. Could you run the other actions if you're happy with everything else? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @EliiseS! This looks good to merge.
This functionality has been released in v2.65.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! |
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. |
Issue link: closes #12292
First draft, perhaps missing tests?