Conversation
iloveicedgreentea
left a comment
iloveicedgreentea
left a comment
There was a problem hiding this comment.
thanks Roy I made some comments. Can you please take a look
terraform/backend/module/alb.tf
Outdated
| @@ -1,12 +1,41 @@ | |||
| data "aws_region" "current" {} | |||
| data "aws_caller_identity" "current" {} | |||
There was a problem hiding this comment.
these are/should be in data.tf
| } | ||
|
|
||
| data "template_file" "log-bucket-policy" { | ||
| template = "${file("${path.module}/log-bucket-policy.json.tpl")}" |
There was a problem hiding this comment.
These vars look like it was made/linted with tf11
There was a problem hiding this comment.
If looking to add in-line policy yo the resource, it should resolve the TF linting issue.
But yeah, my env is set on 11 =
Nonetheless, feel free to pull & commit proper TF 12 linting and move the bucket policy to the same alb.tf resource.
|
|
||
| resource "aws_s3_bucket_policy" "attach-policy-to-log-bucket" { | ||
| bucket = "${aws_s3_bucket.log-bucket.id}" | ||
| policy = "${data.template_file.log-bucket-policy.rendered}" |
There was a problem hiding this comment.
would be better to put this policy in HCL https://github.com/codersagainstcovidorg/infra/blob/master/terraform/frontend/module/iam.tf#L65
There was a problem hiding this comment.
You want to add it as in-line bucket policy rather than templatizing as a separated document?
terraform/backend/module/alb.tf
Outdated
|
|
||
| # alb | ||
| resource "aws_lb" "backend" { | ||
| depends_on = [ |
There was a problem hiding this comment.
not needed since you declare the resource in the block below
There was a problem hiding this comment.
Truth, the dependency declared for logical reasons, but removing it should not create any impact.
I'll follow your request
| security_groups = [aws_security_group.lb_sg.id] | ||
| enable_deletion_protection = false | ||
| enable_cross_zone_load_balancing = false | ||
| access_logs { |
There was a problem hiding this comment.
can you make this optional with a var like enable_s3_access_logs and add it to the configs?
2 participants
No description provided.