dynamically create website inputs with the new provider and its colli… #142
Conversation
|
We don't have a test for this yet. If that could be added to the completr test then that would ensure this functionality works correctly going forward |
| redirect_all_requests_to = string | ||
| routing_rules = string | ||
| })) | ||
| type = map(any) | ||
| default = null |
There was a problem hiding this comment.
could we keep the variable type the same for backwards compatibility?
There was a problem hiding this comment.
Sure, but as linked 'redirect_all_requests_to' is mutually exclusive with the other 3 options.. So, you are either setting 'redirect_all_requests_to' or 'index_document', 'error_document' and 'routing_rules'. Not sure how to handle that? I saw there is an experimental 'optional' feature but that is subject to change.
There was a problem hiding this comment.
Hi, I would really appreciate some feedback to this, not sure how to handle it.
There was a problem hiding this comment.
the workaround has been to set unused options to null but maybe your way is the correct way forward, provided backwards compatibility is maintained
There was a problem hiding this comment.
Since this never worked in v2.0, I'm OK with having a breaking change here. Unfortunately, the "optional" feature is not usable (changes to it have been announced).
Let's make this 2 inputs.
website_redirect_all_requests_towhich is alist(object)withhost_nameandprotocoland a validation oflength <= 2and default[]website_configurationwhich islist(object)with the remaining configuration, including a list ofrouting_ruleobjects, and a validation oflength <= 2and default[]
What do you think @jurgen-weber-deltatre and @nitrocode ?
ok, never used that tool but I Can have a look tomorrow. |
the tests are built into the module and many tests are built into the examples/complete module and verified via test/src directory. If both can be expanded to include website inputs, then we can verify the changes coming in from this PR |
|
/test test/bats |
|
/test test/readme |
|
/test test/terratest |
mergify
bot
dismissed
nitrocode’s stale review
This Pull Request has been updated, so we're dismissing all reviews.
| @@ -90,24 +90,36 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "default" { | |||
| } | |||
|
|
|||
| resource "aws_s3_bucket_website_configuration" "default" { | |||
| for_each = local.enabled && var.website_inputs != null ? toset(var.website_inputs) : toset([]) | |||
| for_each = local.enabled && var.website_inputs != null ? var.website_inputs : {} | |||
There was a problem hiding this comment.
This cannot and should not be for_each, because you can only have one configuration per bucket
| redirect_all_requests_to = string | ||
| routing_rules = string | ||
| })) | ||
| type = map(any) | ||
| default = null |
There was a problem hiding this comment.
Since this never worked in v2.0, I'm OK with having a breaking change here. Unfortunately, the "optional" feature is not usable (changes to it have been announced).
Let's make this 2 inputs.
website_redirect_all_requests_towhich is alist(object)withhost_nameandprotocoland a validation oflength <= 2and default[]website_configurationwhich islist(object)with the remaining configuration, including a list ofrouting_ruleobjects, and a validation oflength <= 2and default[]
What do you think @jurgen-weber-deltatre and @nitrocode ?
|
@jurgen-weber-deltatre After implementing Please also add |
mergify
bot
dismissed
Nuru’s stale review
This Pull Request has been updated, so we're dismissing all reviews.
…ding options
what
why
references