search

trussworks / terraform-aws-alb-web-containers

Creates an ALB for serving a web app.
https://registry.terraform.io/modules/trussworks/alb-web-containers
BSD 3-Clause "New" or "Revised" License
4 stars 8 forks source link

Support optional access logs #108

Closed brainsik closed 3 years ago

brainsik commented 3 years ago

Makes it possible to create an ALB that doesn't log to S3. The default to log (because you should) remains. This change makes it easier to build stacks for teaching/learning where you want things fairly stripped down at the beginning.

This supersedes #85 (which I can not reopen due to the deletion of original branch it forked from). That old PR eventually got closed due to a bug in Terraform we were hitting that was fixed in hashicorp/terraform#28424.

cblkwell commented 3 years ago

Actually...do we want to add a test for the empty set too?

LinuxBozo commented 3 years ago

I would think yes, we would want a test. I had added one in the previous PR https://github.com/trussworks/terraform-aws-alb-web-containers/pull/85/files#diff-cc7be96ea34a5af11800f3b1089566fc523e6ff2b0910759d1bce7898ca8f697

brainsik commented 3 years ago

Leave it to Chas to identify the two topics I'm waffling on. :)

Do we need to increment a major version since we're increasing the required Terraform version considerably...?

I wasn't sure (I can come up with a story for either major or minor), but I'm leaning towards yes to be on the safe side of not breaking anyone who may be auto-updating minor versions.

Actually...do we want to add a test for the empty set too?

This I was also waffling on (and had briefly pulled in @LinuxBozo's code!). This new option is not recommended for production systems and will likely be rarely used, so it felt a little weird to explicitly exercise this path (when we currently only exercise a single one). Of course, my testing roots say "TEST ALL THE THINGS!" So I'm easily convinced to add the test in.

cblkwell commented 3 years ago

I wasn't sure (I can come up with a story for either major or minor), but I'm leaning towards yes to be on the safe side of not breaking anyone who may be auto-updating minor versions.

I think that's probably the safest bet in this case.

This I was also waffling on (and had briefly pulled in @LinuxBozo's code!). This new option is not recommended for production systems and will likely be rarely used, so it felt a little weird to explicitly exercise this path (when we currently only exercise a single one). Of course, my testing roots say "TEST ALL THE THINGS!" So I'm easily convinced to add the test in.

My feeling is that anything that uses toggles or weird dynamic stuff, especially when it's potentially breaking like this can be, should probably get a test.