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

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

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

amontalban
Copy link
Contributor

@amontalban amontalban commented Jun 14, 2024

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

@amontalban amontalban requested review from a team as code owners June 14, 2024 11:21
@mergify mergify bot added the triage Needs triage label Jun 14, 2024
@z0rc
Copy link

z0rc commented Jun 14, 2024

These changes were released in v1.4.1.

@nitrocode
Copy link
Member

@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 ?

@nitrocode nitrocode removed the triage Needs triage label Jun 19, 2024
@z0rc
Copy link

z0rc commented Jun 19, 2024

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

z0rc commented Jun 19, 2024

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

@nitrocode
Copy link
Member

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
Copy link
Contributor Author

@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

main.tf Outdated Show resolved Hide resolved
@amontalban amontalban requested a review from z0rc June 21, 2024 16:22
@nitrocode
Copy link
Member

/terratest

@nitrocode nitrocode enabled auto-merge (squash) June 21, 2024 21:05
@nitrocode nitrocode merged commit 23723dd into cloudposse:main Jun 21, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
3 participants