-
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
New Resource : azurerm_monitor_alert_prometheus_rule_group
#21751
New Resource : azurerm_monitor_alert_prometheus_rule_group
#21751
Conversation
|
||
* `record` - (Optional) Specifies the recorded metrics name. | ||
|
||
* `resolve_configuration` - (Optional) A `resolve_configuration` block as defined below. |
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.
this might make more sense as\ ?
* `resolve_configuration` - (Optional) A `resolve_configuration` block as defined below. | |
* `alert_resolution` - (Optional) A `alert_resolution` block as defined below. |
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 your feedback. I have updated the code. Could you please take another look?
scopes = [azurerm_monitor_workspace.example.id] | ||
rule { | ||
enabled = false | ||
expression = "histogram_quantile(0.99, sum(rate(jobs_duration_seconds_bucket{service=\" billing-processing \"}[5m])) by (job_type))" |
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.
it might be better to use heredoc/multiple line expression here rather then escape "?
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 your feedback. I have updated the code. Could you please take another look?
|
||
output.Alert = pointer.From(input.Alert) | ||
|
||
output.Annotations = pointer.From(input.Annotations) | ||
|
||
output.Enabled = pointer.From(input.Enabled) | ||
|
||
output.For = pointer.From(input.For) | ||
|
||
output.Labels = pointer.From(input.Labels) | ||
|
||
output.Record = pointer.From(input.Record) | ||
|
||
resolveConfigurationValue := flattenPrometheusRuleResolveConfigurationModel(input.ResolveConfiguration) | ||
|
||
output.ResolveConfiguration = resolveConfigurationValue | ||
|
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.
we don't need these blank lines?
output.Alert = pointer.From(input.Alert) | |
output.Annotations = pointer.From(input.Annotations) | |
output.Enabled = pointer.From(input.Enabled) | |
output.For = pointer.From(input.For) | |
output.Labels = pointer.From(input.Labels) | |
output.Record = pointer.From(input.Record) | |
resolveConfigurationValue := flattenPrometheusRuleResolveConfigurationModel(input.ResolveConfiguration) | |
output.ResolveConfiguration = resolveConfigurationValue | |
output.Alert = pointer.From(input.Alert) | |
output.Annotations = pointer.From(input.Annotations) | |
output.Enabled = pointer.From(input.Enabled) | |
output.For = pointer.From(input.For) | |
output.Labels = pointer.From(input.Labels) | |
output.Record = pointer.From(input.Record) | |
resolveConfigurationValue := flattenPrometheusRuleResolveConfigurationModel(input.ResolveConfiguration) | |
output.ResolveConfiguration = resolveConfigurationValue |
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 your feedback. I have updated the code. Could you please take another look?
location = azurerm_resource_group.test.location | ||
resource_group_name = azurerm_resource_group.test.name | ||
dns_prefix = "acctestaks%[2]d" | ||
kubernetes_version = "1.25.5" |
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.
Just passing by as I was researching azurerm_monitor_workspace
and AKS Prometheus metrics, but should this kubernetes_version
maybe be based on currentKubernetesVersion
?
terraform-provider-azurerm/internal/services/containers/kubernetes_cluster_resource_test.go
Line 22 in 35b106b
currentKubernetesVersion = "1.25.5" |
Thanks a lot anyway for creating this resource 👍🏽
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.
Just passing by as I was researching
azurerm_monitor_workspace
and AKS Prometheus metrics, but should thiskubernetes_version
maybe be based oncurrentKubernetesVersion
?terraform-provider-azurerm/internal/services/containers/kubernetes_cluster_resource_test.go
Line 22 in 35b106b
currentKubernetesVersion = "1.25.5" Thanks a lot anyway for creating this resource 👍🏽
Yes, kubernetes_version
is value of currentKubernetesVersion
. Thanks for your reminder. Per the description of kubernetes_version
, If not specified, the latest recommended version will be used at provisioning time
, so in all tests for resource azurerm_monitor_alert_prometheus_rule_group
I removed the specified kubernetes_version
to reduce dependencies. Please let me know if you have other suggestions.
6e60162
to
296aa02
Compare
296aa02
to
8b7cdab
Compare
@katbyte After updating the code, all tests have passed in TeamCity as shown below. Just to let you know. |
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.
LGTM 🌳
This functionality has been released in v3.58.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Swagger: https://github.com/Azure/azure-rest-api-specs/blob/1f449b5a17448f05ce1cd914f8ed75a0b568d130/specification/alertsmanagement/resource-manager/Microsoft.AlertsManagement/stable/2023-03-01/PrometheusRuleGroups.json#L157
Test results: