BCSS-23624 - feat(cloudwatch): add cloudwatch module#61
Conversation
nhs-oliverslater
changed the title
|
I worry that wrapping all the submodules into one single larger module will make things rather bloated and unwieldy - can we discuss this in SU to review and possible submodule these? |
…tric-filter, cloudwatch-logs and cloudwatch-metric-alarm
refactor(readme): remove cloudwatch entry from module list
| check "log_group_must_exist" { | ||
| assert { | ||
| condition = var.log_group_name != "" | ||
| error_message = "log_group_name is required and must not be empty." | ||
| } | ||
| } |
There was a problem hiding this comment.
Doesn't this duplicate the validation on var.log_group_name?
| variable "pattern" { | ||
| type = string | ||
| default = "" | ||
| description = "Log pattern to filter on (e.g., 'ERROR', '[ERROR]', etc). Empty string matches all events." | ||
| } |
There was a problem hiding this comment.
What syntax does this pattern use? Is it a regex (which flavour?), a substring to find, or something else?
Update the description to clarify?
| variable "create_log_group" { | ||
| type = bool | ||
| default = true | ||
| description = "Whether to create the CloudWatch log group." | ||
| } | ||
|
|
||
| variable "create_log_stream" { | ||
| type = bool | ||
| default = false | ||
| description = "Whether to create a CloudWatch log stream. Requires create_log_group = true." | ||
| } |
There was a problem hiding this comment.
| variable "create_log_group" { | |
| type = bool | |
| default = true | |
| description = "Whether to create the CloudWatch log group." | |
| } | |
| variable "create_log_stream" { | |
| type = bool | |
| default = false | |
| description = "Whether to create a CloudWatch log stream. Requires create_log_group = true." | |
| } | |
| variable "create" { | |
| type = string | |
| default = "LOG_GROUP_ONLY" | |
| description = "TODO" | |
| validation { | |
| condition = contains(["NOTHING", "LOG_GROUP_ONLY", "LOG_GROUP_AND_LOG_STREAM"], var.create) | |
| error_message = "TODO" | |
| } | |
| } |
Would this be easier for callers than two separate booleans?
We could recover create_log_group and create_log_stream as locals if we need them.
| variable "kms_key_id" { | ||
| type = string | ||
| default = null | ||
| description = "ARN of KMS key for log group encryption. When null, uses AWS-managed encryption." | ||
| } |
There was a problem hiding this comment.
Do we want to allow AWS-managed encryption here?
I genuinely don't understand what our stance is on requiring our own KMS keys vs using the default.
| variable "metric_alarm" { | ||
| type = object({ | ||
| metric_name = string | ||
| namespace = string | ||
| comparison_operator = string | ||
| evaluation_periods = number | ||
| threshold = number | ||
| statistic = optional(string, "Sum") | ||
| period = optional(number, 60) | ||
| actions_enabled = optional(bool, true) | ||
| }) | ||
| default = null | ||
| description = "Configuration for a single metric alarm. Set to null to skip creation." | ||
| } |
There was a problem hiding this comment.
Presumably there's a limited number of valid values for the statistic field… is it worth adding some validation for it?
4 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.