azurerm_application_gateway - support WAF Policies

[sirlatrom]

Fixes #4667.

[tombuildsstuff]

hey @sirlatrom

Thanks for this PR :)

I've taken a look through and left a few comments inline but this is mostly looking good to me, if we can fix those up then this should otherwise be good 👍

Thanks!

[sirlatrom]

@tombuildsstuff I applied your fixes, thanks for the review!

[katbyte]

Hey @sirlatrom,

thanks for the PR. Iv'e left one question inline about simplifying the firewall_policy to firewall_policy_id. Its a SubResource type so i think it makes sense.

[sirlatrom]

Also see #6126 to be able to create said policies.

[katbyte]

Hey @sirlatrom, thanks for that change. I've left some more comments that need attention - the docs need updating also could we set this property in the complete/another test? thanks!

[katbyte]

I think this can/should be

Suggested change

	if res, ok := d.GetOk("firewall_policy_id"); ok {
	vs := res.([]interface{})
	id := vs[0].(string)
	gateway.ApplicationGatewayPropertiesFormat.FirewallPolicy = &network.SubResource{
	ID: &id,
	}
	}
	if v, ok := d.GetOk("firewall_policy_id"); ok {
	gateway.ApplicationGatewayPropertiesFormat.FirewallPolicy = &network.SubResource{
	ID: &utils.String(v.(string)),
	}
	}

?

[katbyte]

given this is no longer a block or complex set we can:

Suggested change

	if setErr := d.Set("firewall_policy_id", flattenApplicationGatewayFirewallPolicy(props.FirewallPolicy)); setErr != nil {
	return fmt.Errorf("Error setting `firewall_policy_id`: %+v", setErr)
	}
	if props.FirewallPolicy !=nil {
	d.Set("firewall_policy_id", props.FirewallPolicy.ID)
	}

[katbyte]

and we'll need to update the docs here

[sirlatrom]

I believe I've addressed the changes in 896fcb8.

[katbyte]

Thanks @sirlatrom, one last thing is could we set that property in a test?

[sirlatrom]

@katbyte I've tried writing a test that sets the property, but when running the test it does not seem to matter what assertions I make, so maybe there's some documentation somewhere on what I can do to run the test properly?

[sirlatrom]

Rebased on master to resolve conflicts. No manual merge work was needed.

[sirlatrom]

@katbyte I see this has been marked as included in v2.9.0, but it hasn't been merged as such. I suppose now the release has been cut, it would have to be moved to v2.10.0?

[katbyte]

Thanks @sirlatrom! LGTM 🙂

[ghost]

This has been released in version 2.10.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.10.0"
}
# ... other configuration ...

[ghost]

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!