#pr-reviews (2021-06)
Pull Request Reviews for Cloud Posse Projects
2021-06-02
2021-06-03
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 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 will recommend to use this module : https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn
usually people use Cloudfront on front of the bucket since is far more feature rich than pure s3 hosting
Hi PePe! I believe that is a good module for 80% users. In our use case we needed public S3 bucket with website hosting to hold the content which we want to reverse proxy with NGINX. We have CloudFront pointed to NGINX as origin which can then serve some things dynamically and some static things grabbed and interpolated from S3.
I c ok, in that case you need the module you are using
I would just need someone to help test that out then for us.
2021-06-04
2021-06-07
Hello @jose.amengual! Thanks for approving that PR above.
I am curious why test/readme has failed …. anyone else familiar with this ?
I’m not affiliated with CP but it looks like the bot may have failed to commit the formatting changes to your PR branch. Possibly due to permissions on your side?
link: https://github.com/cloudposse/terraform-aws-s3-bucket/runs/2742319215?check_suite_focus=true
Ok I have made the updates discussed here, whenever someone has a chance to approve again , that would be great: https://github.com/cloudposse/terraform-aws-s3-bucket/pull/91
It seems that test steps in the workflow only run after there is at least 1 approval for 1st time contributors …
https://github.com/cloudposse/terraform-aws-sso/pull/13 anyone have a status on this one?
you need to look at the test and see why they failed
Cant find the TF version. Looks ok to me
The github action cannot seem to find the TF version ^
Looks like it’s failing because terraform-config-inspect --json is returning null ?
I ran the command locally from examples/complete and it returns a non null error
$ terraform-config-inspect --json . | jq -r '.required_core[]'
>= 0.13.0
i wonder if the test is running in a different directory
ahh i see what the issue is. there is no [versions.tf](http://versions.tf) file in this repo and that’s why it returns a null value
I just added the file. I’ll kick off the tests again
Awesome. Thanks.
This repo has a few issues to solve and I believe it’s due to some recent updates. Now it’s failing on a different reason
yikes… it’s failing on no test dir
That doesn’t sound good
@Erik Osterman (Cloud Posse) they are. Thanks
@RB where you able to figure this out yet?
I have not had the time to figure out the tests and have not found a good way to test your changes unfortunately. (I did run into the same issue that youre trying to fix fairly recently).
However, it looks like the PR no longer has a failing test. Have the pr tests action been disabled from this repo recently? I don’t see a recent change in the workflows.
also, were you able to test this worked as expected ?
I’m able to test with my current environment and it works for me as expected
2021-06-09
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.
2021-06-11
2021-06-14
Here we go again At your kind attention Quick fix for #74 (tested) https://github.com/cloudposse/terraform-aws-ec2-instance/pull/102
@nnsense small set of changes requested by @Yonatan Koren on this one and then we’ll get it merged
@Matt Gowie @Yonatan Koren all done, thanks
2021-06-15
hello Terraform friends, another quick one for your attention https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/32
2021-06-26
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.
@matt.bernard2006 this was committed w/o proper testing, we’ll fix it
we made the new features a pre-release until a fix is in place https://github.com/cloudposse/terraform-aws-ec2-instance/releases/tag/0.37.0
oops sorry, @MattyB ^
2021-06-28
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
@kevcube thank you for the PR
can you please also remove it from all the examples like here https://github.com/cloudposse/terraform-aws-elasticsearch/blob/master/examples/complete/versions.tf ?
and can you please also run
make init
make github/init
make readme
to update all GitHub actions to the latest versions
thanks