17 messages
Pull Request Reviews for Cloud Posse Projects
azecover 4 years ago
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.
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.
azecover 4 years ago
1-st one I got is: https://github.com/cloudposse/terraform-aws-sns-topic/pull/32
azecover 4 years ago
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.
azecover 4 years ago
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
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).A
azecover 4 years ago
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
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.azecover 4 years ago
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
azecover 4 years ago
I would just need someone to help test that out then for us.
azecover 4 years ago
Here is an attempt of that: https://github.com/cloudposse/terraform-aws-s3-bucket/pull/91
azecover 4 years ago
Hello @PePe Amengual! Thanks for approving that PR above.
I am curious why
I am curious why
test/readme has failed …. anyone else familiar with this ?
MattBover 4 years ago
https://github.com/cloudposse/terraform-aws-sso/pull/13 anyone have a status on this one?
Jakub Rosaover 4 years ago
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.
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.
nnsenseover 4 years ago
Here we go again 😄 At your kind attention 🙂
Quick fix for #74 (tested)
https://github.com/cloudposse/terraform-aws-ec2-instance/pull/102
Quick fix for #74 (tested)
https://github.com/cloudposse/terraform-aws-ec2-instance/pull/102
Mover 4 years ago
hello Terraform friends, another quick one for your attention https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/32
MattyBover 4 years ago
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.
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.
kevcubeover 4 years ago
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