Closed tvaughan77 closed 1 year ago
Hi @tvaughan77, thank you for contributing. I think that you can keep this enabled = var.enable_s3_logs
as a method to enable or disable sending logs regardless of how the target bucket was created.
Then, this module module "lb_logs_s3"
can be created based on a condition like if var.enable_s3_logs and var.log_bucket_id == ""
I'll place comments in the PR too.
Hi @tvaughan77, the code looks good to me, but the plan job for test example is failing.
│ Error: Invalid count argument
│
│ on ../../main.tf line 8, in module "lb_logs_s3":
│ 8: count = var.enable_s3_logs && var.log_bucket_id == null ? 1 : 0
│
│ The "count" value depends on resource attributes that cannot be determined
│ until apply, so Terraform cannot predict how many instances will be
│ created. To work around this, use the -target argument to first apply only
│ the resources that the count depends on.
Hi @jnonino - I think I've got the build working, except it's missing some API Key for a cost based service I don't have?
Error: No INFRACOST_API_KEY environment variable is set.
Hi @tvaughan77, don't worry about it. It is now released in the new version.
Our organization has a single bucket designated for LB access logs in each environment, and we'd like to avoid having a 1:1 ratio between LBs and log buckets.
This commit enables the module user to provide the ID of a pre-existing S3 bucket that the LB is configured to use. This becomes mutually exclusive with the existing "enable_s3_logs" boolean.
One weird thing about this pull request is that because the current default is "true" for
enable_s3_logs
, to use this new bring-your-own-bucket functionality, the module definition looks like this:However, because this module has defaulted that value to
true
and this is a really popular module, I think it'd be a really bad idea to suddenly switch the default fromtrue
tofalse
because that might end up deleting a bunch of people's logging buckets who aren't paying 100% attention to theirterraform plan
outputs.What do you think? Is there a more elegant solution for this?