Making logging configurable

[mohsen0]

No description provided.

[mohsen0]

It seems something is wrong with CI setup, can you take a look?

The command "terraform fmt -check=true" exited with 0.
0.64s$ terraform validate -var "region=${AWS_REGION}" -var "subnets=[]" -var "vpc_id=vpc-abcde012" -var "load_balancer_name=my-lb" -var "log_bucket_name=my-log-bucket" -var "security_groups=[]"
Error: provider.aws: "region": required field is not set
The command "terraform validate -var "region=${AWS_REGION}" -var "subnets=[]" -var "vpc_id=vpc-abcde012" -var "load_balancer_name=my-lb" -var "log_bucket_name=my-log-bucket" -var "security_groups=[]"" exited with 1.

[mohsen0]

Hi @antonbabenko / @brandoconnor Can this be reviewed? sorry to bother you but I want to see if it is possible to get this faster since it is such a small thing

[antonbabenko]

Hi @mohsen0 ! Yes, I can review, but I am not feeling comfortable with everything that is going after (eg, automated tests, release).

@brandoconnor usually reviews such PRs pretty fast.

[antonbabenko]

I would prefer to set the default to true to keep backward compatiblity for people who are using master branch of this module.

[mohsen0]

ok, changed

[mohsen0]

Hi @brandoconnor , can you review this work?
Thanks
Mohsen

[brandonjbjelland]

@mohsen0 - I've had a busy week but should be able to release this by end of day.

I think the change looks fine. I wonder if there are any combinations of log_bucket_name, log_location_prefix, and log_enable that won't work together... If so, those should be mentioned in the README. What have you tried?

[brandonjbjelland]

RE: CI - I don't know how to allow PRs to initiate CI using the secret environment variables provided to Travis. There are some docs on it here:
https://docs.travis-ci.com/user/pull-requests/
https://docs.travis-ci.com/user/environment-variables/

None of that seems to indicate there's a way to allow PRs to use environmnet variables that are secrets but maybe I've misread. That's the reason why CI is failing.

[brandonjbjelland]

I'm going to make a change or two once this is merged but pulling this in now. Thanks for the contribution @mohsen0

[brandonjbjelland]

Ugh and the problem was not too far from my comment. If you try to specify false for this variable but don't provide a valid bucket for log_bucket_name, apply fails. This leaves the user needing to specify a valid bucket, or even more awkward, creating one within the module just to satisfy this requirement.

@mohsen0 did you test any of that before submitting? I can think of different ways to make this work happen but it's somewhat clunky. I won't be cutting this in a release until the problem is resolved.

[mohsen0]

Sorry not back at this yesterday, will respond to your points later today

[brandonjbjelland]

It's a shame to see contributors not taking ownership of their changes. I'll be rolling this back now.

[mohsen0]

I reverted back my changes, I did spend time on this, it will be complicated to keep the current default variables the same thing and make this change work.
There is another way to make this configurable and having tworesource "aws_lb" "application" with and without access_logs section and using count condition which one to be created.

I can make that change if you are happy with that :),
anyhow I will raise another PR with the revert so you merge that for now.

[mohsen0]

#62

[brandonjbjelland]

Thank you. Accepting that PR now.

Re: the count on the two aws_lb resources - I'm fine with going this route. It adds some complexity that might spiral out of control since other resources have explicit dependencies on that resource and outputs must also be juggled with a lot of interpolation. If it works, I'd take it because as an end user, it's just an implementation detail that you shouldn't need to worry about. That's the whole point, right? Abstract away the details.

[github-actions]

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.