Skip to content
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_delivery_configuration #168

Merged
merged 20 commits into from
Jun 14, 2022
Merged

Add log_delivery_configuration #168

merged 20 commits into from
Jun 14, 2022

Conversation

nitrocode
Copy link
Member

@nitrocode nitrocode commented Jun 10, 2022

what

  • Add log_delivery_configuration

why

  • New functionality

references

Test

Test this out in your infrastructure

module "cloudwatch_logs" {
  source  = "cloudposse/cloudwatch-logs/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  context = module.this.context
}

module "redis" {
  # source = "cloudposse/elasticache-redis/aws"
  # Cloud Posse recommends pinning every module to a specific version
  # version = "x.x.x"

  source = "git::https://github.com/cloudposse/terraform-aws-elasticache-redis.git?ref=log_delivery"

  log_delivery_configuration = [
    {
      destination      = module.cloudwatch_logs.log_group_name
      destination_type = "cloudwatch-logs"
      log_format       = "json"
      log_type         = "engine-log"
    }
  ]

  context = module.this.context
}

@nitrocode
Copy link
Member Author

/test all

@nitrocode
Copy link
Member Author

/test all

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️   Due to fd9d3a9 - Move dynamics to correct block - 3 new errors were added

Change details

Error ID Change Path Resource
BC_AWS_GENERAL_9 Added /main.tf aws_elasticache_replication_group.default
BC_AWS_GENERAL_111 Added /main.tf aws_elasticache_replication_group.default
BC_AWS_GENERAL_10 Added /main.tf aws_elasticache_replication_group.default

@cloudposse cloudposse deleted a comment from bridgecrew bot Jun 10, 2022
@cloudposse cloudposse deleted a comment from bridgecrew bot Jun 10, 2022
@cloudposse cloudposse deleted a comment from bridgecrew bot Jun 10, 2022
@nitrocode
Copy link
Member Author

/test all

@nitrocode
Copy link
Member Author

/test all

@nitrocode nitrocode marked this pull request as ready for review June 10, 2022 13:30
@nitrocode nitrocode requested review from a team as code owners June 10, 2022 13:30
@nitrocode nitrocode requested review from Gowiem and dylanbannon and removed request for a team June 10, 2022 13:30
@nitrocode nitrocode changed the title Log delivery Add log_delivery_configuration Jun 10, 2022
@nitrocode nitrocode requested a review from goruha June 10, 2022 13:38
@nitrocode
Copy link
Member Author

/test all

@nitrocode nitrocode requested a review from aknysh June 11, 2022 03:41
@@ -144,6 +143,17 @@ resource "aws_elasticache_replication_group" "default" {
final_snapshot_identifier = var.final_snapshot_identifier
apply_immediately = var.apply_immediately

dynamic "log_delivery_configuration" {
for_each = var.log_delivery_configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we convert it to a map (in locals) to prevent updating the whole list if any item changes?

Copy link
Member Author

@nitrocode nitrocode Jun 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two items can be used here, one for each log_type. We could turn it into a map but i think it would be overkill.

for_each = var.log_delivery_configuration

content {
destination = lookup(log_delivery_configuration.value, "destination", null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are any of these required, or they are all optional?
(if required, we should not provide the default null value)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs do not say. I haven't looked at the golang code and the test sets every key. I figured I'd err on the side of caution and default to null in case one was optional.

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see comments

@aknysh
Copy link
Member

aknysh commented Jun 11, 2022

/test all

@nitrocode nitrocode merged commit a580353 into master Jun 14, 2022
@nitrocode nitrocode deleted the log_delivery branch June 14, 2022 12:22
brian-weis-msr pushed a commit to Measurabl/terraform-aws-elasticache-redis that referenced this pull request Apr 2, 2024
* Add log_delivery

Co-authored-by: cloudpossebot <11232728+cloudpossebot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants