-
Notifications
You must be signed in to change notification settings - Fork 1
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
added cloudwatch module #184
Conversation
terraform/modules/cloudwatch/main.tf
Outdated
} | ||
} | ||
|
||
resource "aws_sns_topic" "slack_notifications" { |
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.
SNS should be split out into a separate module. the topic can be created there and passed to the events module
terraform/modules/cloudwatch/main.tf
Outdated
endpoint = var.slack_notification_endpoint | ||
} | ||
|
||
resource "aws_iam_role" "cloudwatch_events_role" { |
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.
can the IAM resources be split out into an "iam.tf" file - this makes it easier to review these if we are asked for this by cloudone
terraform/modules/cloudwatch/main.tf
Outdated
schedule_expression = var.cron_expression | ||
} | ||
|
||
resource "aws_cloudwatch_event_target" "ecs_event_target" { |
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.
for the event target we should be setting this as a conditional step, right now we only have ecs as the target but in the future this could change. I think there should be some variable "target_type" that will control whether this block gets run. if the target type is ecs you would run this, something like:
for_each = var.target_type == "ecs" ? 1 : 0
@@ -0,0 +1,30 @@ | |||
variable "resource_prefix" { |
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.
from previous comment we should have something to determine the target type like this:
variable "target_type" {
type = string
description = "Select a target type for the cloudwatch event"
sensitive = false
validation {
condition = contains(["ecs"], var.target_type)
error_message = "The variable target_type must be one of: ecs"
}
}
type = string | ||
} | ||
|
||
variable "ecs_cluster_name" { |
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.
these should have default null values and can be validated in locals like:
locals {
validate_ecs_cluster = (var.target_type == "ecs" && var.ecs_cluster_name == null) ? tobool("Please pass in the ecs cluster name if you are using ecs as the target.") : true
}
this way there will be an error if the target is set to ecs and these vars don't have values passed in
No description provided.