Closed cdsre closed 1 year ago
This PR has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this PR will be closed in 10 days
this is not a change we are prepared to accept as its written. Lets work out any details through an issue first
Thanks Bryant. I have issue 90 open for this work. Happy to chat there about it more
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.
Description
This change is to remove the code duplication that occurs over the 4 VPN resource flavours. These resources had about 90% duplicate code and changes others made needed to be made in several places. This refactor consolidates to a single resource utilising the feature from terraform 0.12.0 passing null as a value for a resource attribute is like not passing the value at all. This means we can just put all the attributes into a single resource and default the values to null. so only those attributes a user sets will be passed to the resource.
Motivation and Context
Reading the module source when not familiar with it can be confusing for new users and because there are several flavours that all repeat 90% of the code. Making a small change currently actually requires you to update that code change in multiple places. this can lead to users not realising the code needs replicated in other resources.
Addresses https://github.com/terraform-aws-modules/terraform-aws-vpn-gateway/issues/90
Breaking Changes
This will be a breaking change as it changes the input default for 4 variables from a default of empty string to a default of null
This is likely to be a rare use case where by a user would explicitly pass one of the values as an empty string. However it should be change of the major version since its a change of the public interface. While this could have been handeled in local variables by evaluating the value and if its an empty string set it to null. However the best practice here is to set as null as the default variable as it bring its in line with terraform practices and the default values for other variables. It also reduces code complexity in the module source.
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectscomplete-vpn-gateway
example to pass empty strings to the module. I have written terratest file to test the main changes here which are around the 4 variables above. These tests all passedpre-commit run -a
on my pull request