provider/aws: Add Ability To Enable / Disable ALB AccessLogs

[jvasallo]

Hey all,

This is my first major swing at committing to Terraform, saw the issue below and I think this fixes it. Going to do some more testing on my end, but please provide feedback?

Related to #9279

[stack72]

Hi @jvasallo

Thanks for the PR and a huge thanks for your first contribution :)

1 small question inline but it's looking good!

Paul

[stack72]

why change this to the formatting of a bool?

[jvasallo]

Good catch, I think I was initially going to convert everything to be a aws.Bool until I looked at the AWS SDK and noticed they wanted String types. Since a user will never be setting this, i'll go ahead and change this back!

[jvasallo]

I have finished the code and I have tested it out in my local TF/AWS environment. It works for me!

I did have a question about the default value for access_logs.s3.enabled that stems from resource_aws_elb.go. Is there a reason it was set to true by default? I figured it would make more sense for this to be false by default since not everyone uses access_logs in AWS.

Working on updating the docs now.

[jvasallo]

Added docs, and I just thought about it a bit more. By specifying access_logs you are essentially wanting this to be true. If you want to disable them in the future, that is where enabled comes into play.

[jen20]

Hi @jvasallo! Could we accomplish this without an explicit enabled and detect removal instead?

[jvasallo]

@jen20 do you mean if I were to remove the access_logs block? I think that functionality is already built in before this request? Correct me if I am wrong though! :)

Issue #9279 I think was asking for an enabled type functionality; similar to that of the elb resource. I could see it being useful when you want to have a bucket on standby, but not always writing to it for cost reasons.

[jvasallo]

Anything holding this up? Just want to know before I jump onto the next issue! Thanks!

[stack72]

Hi @jvasallo

Thanks for the work here - I have just pulled and merged this locally. I had to make 1 small change to it to allow the tests to work as expected - you can see the change here b3a0145

Thanks for the work here

Paul

[ghost]

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.