-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_api_management_diagnostic add additional options, fix log_client_ip #10325
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Make sense, thanks for context.
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.
@manicminer I'm assuming the failed golint check as a result of GetOkExist can be ignored?
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.
@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
@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 👉 [email protected]. 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_diagnostic
as 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
ok
value fromGetOk
does 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_diagnostic
reuse the functionsexpandApiManagementApiDiagnosticHTTPMessageDiagnostic
andflattenApiManagementApiDiagnosticHTTPMessageDiagnostic
should I rename these? I didn't because I wasn't sure it mattered.