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

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

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Conversation

SweetOps
Copy link
Contributor

what

  • use security-group module instead of resource
  • update tests

why

  • more flexible than current implementation
  • bring configuration of security group/rules to one standard

references

  • CPCO-409

@SweetOps SweetOps requested review from a team as code owners May 21, 2021 15:37
@SweetOps SweetOps requested review from Makeshift and joe-niland and removed request for a team May 21, 2021 15:37
@SweetOps SweetOps marked this pull request as draft May 21, 2021 15:38
@SweetOps
Copy link
Contributor Author

/test all

@SweetOps
Copy link
Contributor Author

/test all

@SweetOps SweetOps marked this pull request as ready for review May 25, 2021 08:44
@SweetOps SweetOps merged commit 03eec08 into master Jun 15, 2021
@SweetOps SweetOps deleted the CPCO-409 branch June 15, 2021 17:02
@z0rc
Copy link

z0rc commented Jun 22, 2021

Please explain why options existing_security_groups and use_existing_security_groups were removed.

@nitrocode
Copy link
Member

nitrocode commented Jun 22, 2021

@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 #123 (comment))

@Nuru
Copy link
Contributor

Nuru commented Jun 22, 2021

@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
Copy link
Member

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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants