cloudposse / terraform-aws-elasticache-redis

Terraform module to provision an ElastiCache Redis Cluster
https://cloudposse.com/accelerate
Apache License 2.0
141 stars 244 forks source link

feat: use security-group module instead of resource #119

Closed SweetOps closed 3 years ago

SweetOps commented 3 years ago

what

why

references

SweetOps commented 3 years ago

/test all

SweetOps commented 3 years ago

/test all

z0rc commented 3 years ago

Please explain why options existing_security_groups and use_existing_security_groups were removed.

nitrocode commented 3 years ago

@z0rc you should be able to provide existing security groups using var.security_groups and disable the security group creation by setting var.security_group_enabled = false. Does that work for you ?

Edit: It appears it worked for @z0rc (see https://github.com/cloudposse/terraform-aws-elasticache-redis/issues/123#issuecomment-866230241)

Nuru commented 3 years ago

@SweetOps @nitrocode Can we provide backward compatibility by doing something like

locals {
  security_groups = compact(concat(var.existing_security_groups, var.security_groups))
  security_group_enabled = var.use_existing_security_groups == null ? var.security_group_enabled : ! var.use_existing_security_groups
}

variable "use_existing_security_groups" {
  type        = bool
  description = <<-EOT
    DEPRECATED: Use `var.security_group_enabled` instead.
    HISTORICAL USAGE: Flag to enable/disable creation of Security Group in the module. 
    Set to `true` to disable Security Group creation and provide a list of existing security 
    Group IDs in `existing_security_groups` to place the cluster into"
    EOT
  default     = null
}

variable "existing_security_groups" {
  type        = list(string)
  default     = []
  description = <<_EOT
    DEPRECATED: Use `var.security_groups` and set `var.security_group_enabled = false` instead.
    HISTORICAL USAGE: List of existing Security Group IDs to place the cluster into. 
    Set `use_existing_security_groups` to `true` to enable using `existing_security_groups` as Security Groups for the cluster"
    EOT
}
nitrocode commented 3 years ago

Breaking changes have been added in the release notes https://github.com/cloudposse/terraform-aws-elasticache-redis/releases/tag/0.40.0

@Nuru , we could do this in a follow up PR, perhaps also update the README a bit more.

What do you think @SweetOps and @osterman ?