#pr-reviews (2021-06)

Pull Request Reviews for Cloud Posse Projects

2021-06-11

Jakub Rosa avatar
Jakub Rosa
Add possible disable logs for s3 by ByJacob · Pull Request #63 · cloudposse/terraform-aws-s3-website attachment image

what add possible disable create logs bucket and configuration for s3 why sometimes logs for s3 website isn’t required :) references don’t create issue

2021-06-09

Jakub Rosa avatar
Jakub Rosa

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.

Add possible disable created service and propose new approach about customize ignore_changes by ByJacob · Pull Request #119 · cloudposse/terraform-aws-ecs-alb-service-task attachment image

what Add possible disable created service why Sometimes there is a need to add ignore_changes to sites. The current approach is to copy the entire block. I propose to output the necessary variabl…

2021-06-07

azec avatar

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

MattyB avatar
MattyB

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

GitHub attachment image

GitHub is where people build software. More than 65 million people use GitHub to discover, fork, and contribute to over 200 million projects.

azec avatar

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

1
azec avatar

It seems that test steps in the workflow only run after there is at least 1 approval for 1st time contributors …

matt.bernard2006 avatar
matt.bernard2006
issue 12 possible fix by innominatus · Pull Request #13 · cloudposse/terraform-aws-sso attachment image

what a.permission_set_arn is providing a unique value to the account_assignment name. However the permission_set_arn can not be determined until after the apply of the permission sets. Using a.per…

jose.amengual avatar
jose.amengual

you need to look at the test and see why they failed

issue 12 possible fix by innominatus · Pull Request #13 · cloudposse/terraform-aws-sso attachment image

what a.permission_set_arn is providing a unique value to the account_assignment name. However the permission_set_arn can not be determined until after the apply of the permission sets. Using a.per…

matt.bernard2006 avatar
matt.bernard2006

Cant find the TF version. Looks ok to me

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

The github action cannot seem to find the TF version ^

Looks like it’s failing because terraform-config-inspect --json is returning null ?

https://github.com/cloudposse/actions/blob/77043a3f48af65866947f1e4a058425e783f9496/.github/workflows/test-command.yml#L216

cloudposse/actions attachment image

Our Library of GitHub Actions. Contribute to cloudposse/actions development by creating an account on GitHub.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

i wonder if the test is running in a different directory

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I just added the file. I’ll kick off the tests again

matt.bernard2006 avatar
matt.bernard2006

Awesome. Thanks.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

yikes… it’s failing on no test dir

matt.bernard2006 avatar
matt.bernard2006

That doesn’t sound good

matt.bernard2006 avatar
matt.bernard2006

@Erik Osterman (Cloud Posse) they are. Thanks

matt.bernard2006 avatar
matt.bernard2006

@RB (Ronak) (Cloud Posse) where you able to figure this out yet?

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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.

cloudposse/terraform-aws-sso attachment image

Terraform module to configure AWS Single Sign-On (SSO) - cloudposse/terraform-aws-sso

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

also, were you able to test this worked as expected ?

matt.bernard2006 avatar
matt.bernard2006

I’m able to test with my current environment and it works for me as expected

1

2021-06-04

2021-06-03

azec avatar

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.

azec avatar
feature/kms-optional: Making Encryption configuration of SNS less constraining (also to opt out) by azec-pdx · Pull Request #32 · cloudposse/terraform-aws-sns-topic attachment image

what I was using cloudposse/terraform-aws-sns-topic to deploy SNS Topic and subscriber SQS queues for routing Bounce and Complaint notifications from AWS SES service. AWS SES won't accept SNS …

azec avatar

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.

azec avatar

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).

azec avatar
azec
12:50:02 AM

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.

azec avatar
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 <https://github.com/cloudposse/terraform-aws-s3-bucket cloudposse/terraform-aws-s3-bucket](https://github.com/cloudposse/terraform-aws-s3-bucket) master
jose.amengual avatar
jose.amengual
cloudposse/terraform-aws-cloudfront-s3-cdn attachment image

Terraform module to easily provision CloudFront CDN backed by an S3 origin - cloudposse/terraform-aws-cloudfront-s3-cdn

jose.amengual avatar
jose.amengual

usually people use Cloudfront on front of the bucket since is far more feature rich than pure s3 hosting

azec avatar

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.

jose.amengual avatar
jose.amengual

I c ok, in that case you need the module you are using

azec avatar

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

azec avatar
Ability to specify website hosting configuration by azec-pdx · Pull Request #91 · cloudposse/terraform-aws-s3-bucket attachment image

what We already use this module for S3 and want to be able to use it to deploy S3 buckets with static website hosting configs The change in this PR adds that support why Would like to be able to…

2021-06-02

    keyboard_arrow_up