-
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
Add Log Analytics support for ACI #2763
Conversation
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 @metacpp for the contribution. The code looks good except some small issues. Once they are resolved, it would be good to go.
Co-Authored-By: metacpp <1684739+metacpp@users.noreply.github.com>
…vider-azurerm into ci_log
… while verifying import state.
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 for the update and the code now looks good to me.
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 @metacpp
Thanks for pushing those changes - apologies for the delayed re-review here!
I've taken a look through and left some comments in-line but I feel like we need to better match the SDK here (by placing the log_analytics
block within a diagnostics
block) since it appears that multiple diagnostics options may be supported in the future, by virtue of this diagnostics
wrapper existing.
Since there's a couple of out-of-scope changes which need to be made to the documentation for this resource to make this consistent - I'm going to push a commit to fix the highlighted issues and update the documentation; I hope you don't mind.
Thanks!
@@ -406,7 +461,14 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err | |||
|
|||
d.Set("restart_policy", string(props.RestartPolicy)) | |||
d.Set("os_type", string(props.OsType)) | |||
|
|||
if diag := props.Diagnostics; diag != nil { |
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.
since we're combining this into a diagnostics
block, we should move this within the flattenContainerLogAnalytics
function (since that handles returning an empty list if this is nil) such that this is set regardless
dismissing since changes have been pushed
This has been released in version 1.23.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 = "~> 1.23.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 PR is mainly to addressing the feature request in #2755, which includes:
log_analytics
field support in ACI.