refactor(module/application_insights)!: module refactor and adjusted examples #346
Conversation
…hts module Update examples to use the refactored module.
…hts module Re-order variables
alperenkose
changed the title
| provider "azurerm" { | ||
| features { | ||
| resource_group { | ||
| prevent_deletion_if_contains_resources = false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
try to avoid indented code blocks in favour of explicit ones, with langugae notation
| provider "azurerm" { | |
| features { | |
| resource_group { | |
| prevent_deletion_if_contains_resources = false | |
| } | |
| } | |
| } | |
| ```hcl | |
| provider "azurerm" { | |
| features { | |
| resource_group { | |
| prevent_deletion_if_contains_resources = false | |
| } | |
| } | |
| } | |
| ``` |
|
|
||
| In both situations the instrumentation key for the Application Insights has to be provided in the firewall's configuration. For more information please refer to [documentation](https://docs.paloaltonetworks.com/vm-series/10-2/vm-series-deployment/set-up-the-vm-series-firewall-on-azure/enable-azure-application-insights-on-the-vm-series-firewall). | ||
|
|
||
| **NOTICE** |
There was a problem hiding this comment.
Maybe we could use GH admonitions? to make it look better?
|
|
||
| The following snippet deploys Application Insights and Log Analytics Workspace, setting the retention to 1 year. | ||
|
|
||
| ```hcl |
There was a problem hiding this comment.
the example is obsolete, for example the worskspace name is required
| @@ -1,23 +1,31 @@ | |||
| variable "name" { | |||
| description = "Name of the Application Insights instance." | |||
| type = string | |||
| nullable = false | |||
There was a problem hiding this comment.
nullable makes sense only if you have a default value
| type = string | ||
| nullable = false | ||
| } | ||
|
|
||
| variable "workspace_sku" { |
There was a problem hiding this comment.
two things here:
- multiline description, an
EOFlike notation would probably look better - check if the default value is still
PerGB2018. Maybe we could dropdefaulthere andnullthis value instead, to relay on Azure defaults. If it's not possible, addnullableto force thedefaultvalue.
| @@ -31,19 +39,3 @@ variable "metrics_retention_in_days" { | |||
| default = null | |||
There was a problem hiding this comment.
Multiline description, add EOF notation
And maybe block-quote the possible values, like: 0, 30, 60, etc
|
|
||
| Properties supported (for details on each property see [modules documentation](../../modules/application_insights/README.md)): | ||
| For detailed documentation on each property refer to [module documentation](../../modules/application_insights/README.md) |
There was a problem hiding this comment.
there are only 4 properties here, I would avoid pointing the user to module's documentation if it's not really necensary, like when you omit some nested props to avoid duplicating documentation.
| type = map(object({ | ||
| name = string | ||
| workspace_name = string | ||
| workspace_sku = optional(string, "PerGB2018") |
There was a problem hiding this comment.
see comment around workspace_sku variable, if we need to keep the default value in the module's variable definition, add nullable to the definition, skip the default value here. This way you control the default value only in one place and you do not overwrite it in the example code.
| - `metrics_retention_in_days` : (optional, number) defaults to current Azure default value, see module documentation for details | ||
| - `name` - (`string`, required) name of the Application Insights instance. | ||
| - `workspace_name` - (`string`, required) name of the Log Analytics Workspace to be created together with the AI instance. | ||
| - `workspace_sku` - (`string`, optional, defaults "PerGB2018") SKU used by WAL, for details see [Application Insights module documentation](../../modules/application_insights/README.md#workspace_sku). |
There was a problem hiding this comment.
if we need to keep the default value in the module, set the parentised text to soemthing like optional, default to module defaults
|
this will be refactored with the #335 |
Description
Closes #326
This branch is based on #307.
Types of changes
Checklist