-
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
Update azurerm_eventgrid_* resources #5948
Update azurerm_eventgrid_* resources #5948
Conversation
@tombuildsstuff I think this MR should now be in a reviewable state |
@mbfrahry @tombuildsstuff @tracypholmes Anyone who could review this? I know it's really a huge PR, but it was necessary to update the API client to be able to use the most up to date EventGrid features. |
@tombuildsstuff any update here? |
I for one would also like to know what's happening here too.. this would help out my scripts if it could be merged. Anything I can do to help? |
@tombuildsstuff Is there a way to get some progress here? Are there any tests missing? There is already a new EventGrid API version available... and it contains a lot of new stuff. Imho we should try to get more closer to the azure-sdk-for-go release cycles to be able to use the newest features. |
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.
the advanced filter still doesn't work in terraform 1.12 with provider version 2.0
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.
First try to get some progress here with an initial self review.
azurerm/internal/services/eventgrid/resource_arm_eventgrid_event_subscription.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/eventgrid/resource_arm_eventgrid_event_subscription.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/eventgrid/resource_arm_eventgrid_event_subscription.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/eventgrid/resource_arm_eventgrid_event_subscription.go
Show resolved
Hide resolved
@jokerbea Can you give me a bit more details on this? What was your test setup? TF 0.12 and TF AzureRM Provider 2.0**? Can you post your used TF files files? What was your expectation about the result? What went wrong from your point of view? ** Hint: For local testing you'd have to clone the forked AzureRM provider repo and build the latest state of the branch locally using the Makefile. Then you have to initialize your terraform project with the locally build terraform plugin by copying the compiled plugin to path
|
Hello Jochen
I'm trying to add the advanced filters to the subscriptions events with
terraform. The attached file shows my actual code.
As mentioned by you that parameter is not available yet.
What I'm trying to do is:
Have the advanced filters with the following parameters:
- key: data.url Operator: String contains Value: /<folder>/<source_name>/
- key: data.api Operator: String is in Value: CreateFile
Would there be any workaround for this with the present version of
terraform to enable this.
Le dim. 26 avr. 2020 à 04:45, Jochen Rauschenbusch <[email protected]>
a écrit :
… the advanced filter still doesn't work in terraform 1.12 with provider
version 2.0
Can you give me a bit more details on this?
What was your test setup? What was your expetation about the result? What
went wrong from your point of view?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://mailtrack.io/trace/link/bb230d6ab0cadaef24de933c845c897e7bdf4a7c?url=https%3A%2F%2Fgithub.com%2Fterraform-providers%2Fterraform-provider-azurerm%2Fpull%2F5948%23issuecomment-619511188&userId=2464481&signature=9081ea360ca6db56>,
or unsubscribe
<https://mailtrack.io/trace/link/c581fb0f321b1a0a47e32cc55f047ed6e45d4685?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAC6E7W54HSHUCOTROT73C43ROPYDHANCNFSM4K7JOAQQ&userId=2464481&signature=855f83eb0def2516>
.
--
*Beatriz Kanzki*
Bsc Bioinformatics
MscA Software Engineering
Specialized in Big Data analytics and cloud technologies
ᐧ
|
- advanced_filter usage simplified - advanced_filter validation rules added - service_bus_topic_endpoint added - azure_function_endpoint added
d341d87
to
74a64a1
Compare
@jokerbea As within the current AzureRM provider version only a subject filter is provided, this is currently not possible (see https://www.terraform.io/docs/providers/azurerm/r/eventgrid_event_subscription.html). Hence you have to wait until this PR is merged... |
@tombuildsstuff @tracypholmes @mbfrahry @katbyte @jackofallops Please assign a milestone label to this ticket so that it's on your roadmap. I've done a lot invested a lot of time to bring this topic forward. |
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.
Hey @jrauschenbusch, thanks for this PR! However typically we try not to combine so many changes into one PR. Could you split this out into an SDK upgrade PR and then the rest of the changes into a couple different PRs? Large PRs like this take far longer to review and merge thus are more likely to sit in the qeue for a while. Thanks!
id, err := azure.ParseAzureResourceID(d.Id()) | ||
if err != nil { | ||
return err | ||
} | ||
resourceGroup := id.ResourceGroup | ||
domainName := id.Path["domains"] | ||
name := id.Path["topics"] |
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.
Could we write a specific parse function & use it like can be seen in #5356
} | ||
|
||
// RemoveFromStringArray removes all matching values from a string array | ||
func RemoveFromStringArray(elements []string, remove string) []string { |
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.
Could we move this to the utils package?
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.
Hey @jrauschenbusch, thanks for this PR! However typically we try not to combine so many changes into one PR. Could you split this out into an SDK upgrade PR and then the rest of the changes into a couple different PRs? Large PRs like this take far longer to review and merge thus are more likely to sit in the qeue for a while. Thanks!
@katbyte Thanks for your response. I already had this feeling that the PR is ways too large. I try to split up the PR into multiple smaller ones.
My proposal:
- PR for Basic Event Grid API Update to the latest Preview version (https://github.com/Azure/azure-sdk-for-go/tree/master/services/preview/eventgrid/mgmt/2020-04-01-preview/eventgrid)
- PR for SDK util functions
- PR for new resource
azurerm_eventgrid_topic_domain
- PR for extended resource
azurerm_eventgrid_topic
- PR for extended resource
azurerm_eventgrid_event_subscription
- Dedicated PR for
advanced_filters
in resourceazurerm_eventgrid_event_subscription
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.
You can attach the util functions into the first PR requiring them but otherwise sounds good!
Upgrade EventGrid SDK to 2020-04-01-preview from 2018-09-15-preview to address @katbyte comments in PR hashicorp#5948.
Thanks! closing in favour of them |
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! |
Solves #3452, #4097, #4308 and #4857
List of changes:
Fixes #3452
Fixes #4097
Fixes #4308
Fixes #4857