azurerm_api_management_diagnostic add additional options, fix log_client_ip #10325
Conversation
Previously when log_client_ip was true on the resource and there was desire to set to false a apply loop would occur as the value wasn't being updated. This is now fixed.
ghost
added
|
New acceptance test pass (as first submitter to this repository the README is out of date with how to run the acceptance tests). The ~/go/src/github.com/terraform-providers/terraform-provider-azurerm(fix-9196-api_management_diagnostic)
$ TF_ACC=1 go test -v -timeout 120m -run '^(TestAccApiManagementDiagnostic_basic|TestAccApiManagementDiagnostic_update|TestAccApiManagementDiagnostic_requiresImport|TestAccApiManagementDiagnostic_complete)$' github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/apimanagement -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccApiManagementDiagnostic_basic
=== PAUSE TestAccApiManagementDiagnostic_basic
=== RUN TestAccApiManagementDiagnostic_update
=== PAUSE TestAccApiManagementDiagnostic_update
=== RUN TestAccApiManagementDiagnostic_requiresImport
=== PAUSE TestAccApiManagementDiagnostic_requiresImport
=== RUN TestAccApiManagementDiagnostic_complete
=== PAUSE TestAccApiManagementDiagnostic_complete
=== CONT TestAccApiManagementDiagnostic_basic
=== CONT TestAccApiManagementDiagnostic_complete
=== CONT TestAccApiManagementDiagnostic_requiresImport
=== CONT TestAccApiManagementDiagnostic_update
--- PASS: TestAccApiManagementDiagnostic_requiresImport (2276.90s)
--- PASS: TestAccApiManagementDiagnostic_complete (2370.21s)
--- PASS: TestAccApiManagementDiagnostic_basic (2385.37s)
--- PASS: TestAccApiManagementDiagnostic_update (2403.43s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/apimanagement 2403.466s |
There was a problem hiding this comment.
Thanks @slynickel - overall this looks good, i've left two small comments and once they are addressed this should be good to merge 🙂
azurerm/internal/services/apimanagement/api_management_diagnostic_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/apimanagement/api_management_diagnostic_resource.go
Outdated
Show resolved
Hide resolved
Remove switch statements for http_correlation_protocol and verbosity to instead use the azure sdk for go built in enumeration functions
|
@katbyte back to you. Thanks for the quick response time. |
| logClientIP, _ := d.GetOk("log_client_ip") | ||
| if logClientIP != nil { | ||
| parameters.LogClientIP = utils.Bool(logClientIP.(bool)) | ||
| } |
There was a problem hiding this comment.
Hi @slynickel, many thanks for the PR.
Because this attribute is optional+computed, it would be best to use GetOkExists() which tells you if the attribute is configured, but bypasses the zero value check so you can detect an explicit false value.
| logClientIP, _ := d.GetOk("log_client_ip") | |
| if logClientIP != nil { | |
| parameters.LogClientIP = utils.Bool(logClientIP.(bool)) | |
| } | |
| if logClientIP, exists := d.GetOkExists("log_client_ip"); exists { | |
| parameters.LogClientIP = utils.Bool(logClientIP.(bool)) | |
| } |
There was a problem hiding this comment.
@manicminer Isn't that function being deprecated though? ResourceData.GetOkayExist()
GetOkExists can check if TypeBool attributes that are Optional with no Default value have been set.
Deprecated: usage is discouraged due to undefined behaviors and may be removed in a future version of the SDK
My light review of the terraform-plugin-sdk is that some sections are pretty old, though the deprecation warning is within the last year.
There was a problem hiding this comment.
@slynickel I checked with the team and although it's marked as deprecated in the SDK, in a context like this (specifically optional+computed booleans) there's little alternative and it's ok to use it.
There was a problem hiding this comment.
Updated.
Make sense, thanks for context.
There was a problem hiding this comment.
@manicminer I'm assuming the failed golint check as a result of GetOkExist can be ignored?
There was a problem hiding this comment.
@slynickel we can add a nolint comment to suppress that, i've pushed that whilst I run the acceptance tests
GetOkayExists while deprecated is the best option for boolean optional computed variables
|
Test results
|
|
@slynickel Thanks again for the contribution :) |
|
This has been released in version 2.46.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.46.0"
}
# ... other configuration ... |
|
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! |
This looks larger then it is.
Fixes the remaining request in #9196 by adding additional options to
azurerm_api_management_diagnostic. The heavy lifting was done by recent PRs forazure_api_management_api_diagnosticas the Go SDK uses the same struct for both diagnostics apimangement.DiagnosticContract. This allowed the code to be completely reused.Fixes #10322 which was found while testing this PR. Update to only checking if the input variable is nil as the
okvalue fromGetOkdoes a Zero check which fails to work correctly with a Boolean variable. There could be some other ramifications of this I'm not aware of but I couldn't find a test case for this resource that was an issue.question (non-blocking): the new options to
azurerm_api_management_diagnosticreuse the functionsexpandApiManagementApiDiagnosticHTTPMessageDiagnosticandflattenApiManagementApiDiagnosticHTTPMessageDiagnosticshould I rename these? I didn't because I wasn't sure it mattered.