-
-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature(access_logs): make possible to specify an external s3 bucket … #83
feature(access_logs): make possible to specify an external s3 bucket … #83
Conversation
main.tf
Outdated
@@ -39,6 +39,8 @@ resource "aws_security_group_rule" "https_ingress" { | |||
} | |||
|
|||
module "access_logs" { | |||
count = var.access_logs_s3_bucket_id == "" ? 1 : 0 | |||
|
|||
source = "cloudposse/lb-s3-bucket/aws" | |||
version = "0.11.4" | |||
enabled = module.this.enabled && var.access_logs_enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reasons to add count
you can just extend condition in the enabled
@scream314, let us know if you can keep working on this. I can take it, otherwise. |
@adamantike I hope I can address the change requests next week. I added a reminder in my calendar for Wednesday. |
d885c92
to
64906e6
Compare
This pull request is now in conflict. Could you fix it @scream314? 🙏 |
This Pull Request has been updated, so we're dismissing all reviews.
7db6f0b
to
773f223
Compare
This pull request is now in conflict. Could you fix it @scream314? 🙏 |
@SweetOps, I think this is ready for another review! |
/test all |
This pull request is now in conflict. Could you fix it @scream314? 🙏 |
/test all |
Thanks @scream314 -- released as |
…for logs
what
access_logs
module is skipped.why
references