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 azurerm_eventgrid_* resources #5948

Closed

Conversation

jrauschenbusch
Copy link
Contributor

@jrauschenbusch jrauschenbusch commented Mar 1, 2020

Solves #3452, #4097, #4308 and #4857

List of changes:

  • Updates clients to EventGrid API version 2020-01-01-preview
  • Adds resource azurerm_eventgrid_topic_domain
  • Additions to resource azurerm_eventgrid_topic:
    • input_schema
    • input_mapping_fields
    • input_mapping_default_values
  • Additions to resource azurerm_eventgrid_event_subscription:
    • service_bus_queue_endpoint
    • azure_function_endpoint
    • expiration_time_utc
    • advanced_filters

Fixes #3452
Fixes #4097
Fixes #4308
Fixes #4857

@jrauschenbusch
Copy link
Contributor Author

@tombuildsstuff I think this MR should now be in a reviewable state

@jrauschenbusch
Copy link
Contributor Author

jrauschenbusch commented Mar 16, 2020

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

@jrauschenbusch
Copy link
Contributor Author

@tombuildsstuff any update here?

@CraftyFella
Copy link

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?

#5948 (comment)

@jrauschenbusch
Copy link
Contributor Author

jrauschenbusch commented Apr 22, 2020

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

Copy link

@bkanzki bkanzki left a 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

Copy link
Contributor Author

@jrauschenbusch jrauschenbusch left a 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.

@jrauschenbusch
Copy link
Contributor Author

jrauschenbusch commented Apr 26, 2020

the advanced filter still doesn't work in terraform 1.12 with provider version 2.0

@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 ~/.terraform.d/plugins and use terraform init inside your project path. (Precondition would be an pre-installed and configured Go SDK)

mkdir -p ~/go/src/github.com/terraform-providers  && cd $_

git clone git@github.com:jrauschenbusch/terraform-provider-azurerm.git
cd terraform-provider-azurerm
make build

cp ~/go/bin/terraform-provider-azurerm ~/.terraform.d/plugins/terraform-provider-azurerm_v2.0.0

cd /path/to/your/tf/project
terraform init

@bkanzki
Copy link

bkanzki commented Apr 27, 2020 via email

- advanced_filter usage simplified
- advanced_filter validation rules added
- service_bus_topic_endpoint added
- azure_function_endpoint added
@jrauschenbusch
Copy link
Contributor Author

jrauschenbusch commented May 6, 2020

What I'm trying to do is:

Have the advanced filters with the following parameters:

  • key: data.url Operator: String contains Value: //<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.

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

@jrauschenbusch
Copy link
Contributor Author

jrauschenbusch commented May 6, 2020

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

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.

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!

Comment on lines +101 to +107
id, err := azure.ParseAzureResourceID(d.Id())
if err != nil {
return err
}
resourceGroup := id.ResourceGroup
domainName := id.Path["domains"]
name := id.Path["topics"]
Copy link
Collaborator

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

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?

Copy link
Contributor Author

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:

Copy link
Collaborator

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!

katbyte pushed a commit that referenced this pull request May 11, 2020
Upgrade EventGrid SDK to 2020-04-01-preview from 2018-09-15-preview to address @katbyte comments in PR #5948.
jrauschenbusch added a commit to jrauschenbusch/terraform-provider-azurerm that referenced this pull request May 13, 2020
Upgrade EventGrid SDK to 2020-04-01-preview from 2018-09-15-preview to address @katbyte comments in PR hashicorp#5948.
@katbyte
Copy link
Collaborator

katbyte commented May 14, 2020

Thanks! closing in favour of them

@katbyte katbyte closed this May 14, 2020
@ghost
Copy link

ghost commented Jun 13, 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 Jun 13, 2020
@jrauschenbusch jrauschenbusch deleted the eventgrid-resources branch July 9, 2020 05:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.