#pr-reviews - June 2021

Hi there! My name is Amer.
I have just joined this Slack community.
I would like to get help in PR reviews for some upstream contributions we are trying to make.

1-st one I got is: https://github.com/cloudposse/terraform-aws-sns-topic/pull/32

I have tagged few people about 20-ish days ago based on previous commit history. But I haven’t got any response, so trying to kick it up in Slack.

2-nd one would be related to terraform-aws-s3-bucket module but I need some guidance here.
We tried using that module and quickly realized it doesn’t support static website hosting config.
Then we learned there is terraform-aws-s3-website , but the problem with that is it doesn’t support modifying s3 bucket policy.
What we needed in our use-case was: Publicly accessible S3 bucket, with S3 policy that allows serving content based on aws:referer key passed on requests and also to allow specific account-local AWS IAM principals to modify bucket content (objects).

So we ended up with small modification of 1st module above on our fork to support website hosting config, since it already supports custom bucket policies.
Unfortunately, we had to rebase this change on top of the 0.31.0 tag , and not the latest ( 0.37.0 at the time of this writing) because we couldn’t take the risk of bumping to 0.37.0 yet without evaluating impact on all of our buckets. The diff is relatively small below.

I am noticing that there hasn’t been website configuration contributed to master in terraform-aws-s3-bucket yet, so my 1st take on this would be to open new branch based on master (synced) on our fork and then add those changes and open PR against cloudposse/terraform-aws-s3-bucket master

I would just need someone to help test that out then for us.

Here is an attempt of that: https://github.com/cloudposse/terraform-aws-s3-bucket/pull/91

Hello @PePe Amengual! Thanks for approving that PR above.
I am curious why test/readme has failed …. anyone else familiar with this ?

https://github.com/cloudposse/terraform-aws-sso/pull/13 anyone have a status on this one?

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/119
Hi,
I wanted to add another ignore_changes option in aws_ecs_service, but noticed someone else added this for their use case (https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/116), and mine the change would result in another dozen or so copies of the code block. I figured maybe instead of adding code to the library, you could derive the appropriate variables and tell users to add an aws_ecs_service block to their terraform file. I would like to know your views on this.

https://github.com/cloudposse/terraform-aws-s3-website/pull/63 🙂

Here we go again 😄 At your kind attention 🙂
Quick fix for #74 (tested)
https://github.com/cloudposse/terraform-aws-ec2-instance/pull/102

Up! 🙂
https://github.com/cloudposse/terraform-aws-rds/pull/115

hello Terraform friends, another quick one for your attention https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/32

Hi CloudPosse team,

I'm curious about your decision to allow the PR located here. I don't see the benefit in having these policies included in your module. The user should have the ability to do this in their code with the output "role" that you provide.. I think you typically provide great defaults and options, but don't really understand this choice. main.tf L#147 and 153 also seem to be broken in this commit due to the role attributes not accepting proper input (missing .name at the end). I'm trying to prompt a discussion of module design best practice. My apologies if this came off harsh at all.

Hey guys! Made a quick remove-dead-code PR in the elasticsearch module, would appreciate a quick review 😁 https://github.com/cloudposse/terraform-aws-elasticsearch/pull/109