cloudposse / terraform-aws-elasticache-redis

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

fix: Default transit_encryption_mode to null if var.transit_encryption_enabled is false #238

Closed amontalban closed 5 months ago

amontalban commented 5 months ago

what

Fixing this problem when var.transit_encryption_enabled is false the transit_encryption_mode value should be null.

why

I was blindsided with my use case of encrypting everything and I should have covered the default use case.

references

z0rc commented 5 months ago

These changes were released in v1.4.1.

nitrocode commented 5 months ago

@z0rc couldn't you set this for redis 6.2.6

  transit_encryption_enabled = true
  transit_encryption_mode    = null

or would it be better to default the transit_encryption_mode to null ?

z0rc commented 5 months ago

@nitrocode I'd prefer transit_encryption_mode to be null by default. In provider it's computed field, so default preferred will be applied when possible. If user wants to have it as required, then this is what this parameter is for.

z0rc commented 5 months ago

Also I think module's variable validation isn't needed, as it duplicates validation built in provider.

nitrocode commented 5 months ago

Yes that makes sense to me. Since this pr is already open, could you make these changes and regenerate the readme @amontalban ? Thank you so much for your contributions

amontalban commented 5 months ago

@nitrocode I have just pushed the change and also validated with the following changes to the fixtures and it worked fine.

diff --git a/examples/complete/fixtures.us-east-2.tfvars b/examples/complete/fixtures.us-east-2.tfvars
index 62676be..78164b8 100644
--- a/examples/complete/fixtures.us-east-2.tfvars
+++ b/examples/complete/fixtures.us-east-2.tfvars
@@ -15,9 +15,9 @@ instance_type = "cache.m6g.large"

 cluster_size = 1

-family = "redis7"
+family = "redis6.x"

-engine_version = "7.0"
+engine_version = "6.2"

 at_rest_encryption_enabled = false
nitrocode commented 5 months ago

/terratest