#pr-reviews (2021-06)

Pull Request Reviews for Cloud Posse Projects

2021-06-02

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-topicattachment 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 cloudposse/terraform-aws-s3-bucket master

jose.amengual avatar
jose.amengual
cloudposse/terraform-aws-cloudfront-s3-cdnattachment 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-bucketattachment 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-04

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

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

GitHubattachment 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-ssoattachment 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-ssoattachment 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/actionsattachment 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-ssoattachment 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-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-taskattachment 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…

1

2021-06-11

Jakub Rosa avatar
Jakub Rosa
Add possible disable logs for s3 by ByJacob · Pull Request #63 · cloudposse/terraform-aws-s3-websiteattachment 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

1

2021-06-14

nnsense avatar
nnsense

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

EBS volumes will be created if enabled = false · Issue #74 · cloudposse/terraform-aws-ec2-instanceattachment image

When the variable enabled = false is set the instance is not created but any count of the EBS volumes is. It is expected from the documentation of enabled that setting this to false will stop creat…

Fix for EBS volumes created when the instance has been disabled, plus some vars description improvement by nnsense · Pull Request #102 · cloudposse/terraform-aws-ec2-instanceattachment image

Fixing #74 (EBS volumes will be created if enabled = false) Fixing a wrong description provided for ebs_volume_encrypted in variables.tf Changing some EBS related description to be clear those are …

1
1
1
Matt Gowie avatar
Matt Gowie

@nnsense small set of changes requested by @Yonatan Koren (Codefresh) on this one and then we’ll get it merged

EBS volumes will be created if enabled = false · Issue #74 · cloudposse/terraform-aws-ec2-instanceattachment image

When the variable enabled = false is set the instance is not created but any count of the EBS volumes is. It is expected from the documentation of enabled that setting this to false will stop creat…

Fix for EBS volumes created when the instance has been disabled, plus some vars description improvement by nnsense · Pull Request #102 · cloudposse/terraform-aws-ec2-instanceattachment image

Fixing #74 (EBS volumes will be created if enabled = false) Fixing a wrong description provided for ebs_volume_encrypted in variables.tf Changing some EBS related description to be clear those are …

nnsense avatar
nnsense

@Matt Gowie @Yonatan Koren (Codefresh) all done, thanks

nnsense avatar
nnsense
Setting ca_cert_identifier default value to null by nnsense · Pull Request #115 · cloudposse/terraform-aws-rdsattachment image

what Setting ca_cert_identifier default value to null, leaving its resolution up to the AWS resource. why Creating RDS in newer regions such as eu-south-1, ap-east-1, me-south-1, af-south-1 errors …

1
1
nnsense avatar
nnsense

Thanks @Matt Gowie

Setting ca_cert_identifier default value to null by nnsense · Pull Request #115 · cloudposse/terraform-aws-rdsattachment image

what Setting ca_cert_identifier default value to null, leaving its resolution up to the AWS resource. why Creating RDS in newer regions such as eu-south-1, ap-east-1, me-south-1, af-south-1 errors …

2021-06-15

mihai avatar

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

add the option for certificate_transparency_logging_preference by marcelobartsch · Pull Request #32 · cloudposse/terraform-aws-acm-request-certificateattachment image

what Add the option to DISABLE or ENABLE certificate_transparency_logging_preference parameter when creating the certificate why Some cases you don't want to disclose certificate names, as a…

2021-06-26

MattyB avatar

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.

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

@matt.bernard2006 this was committed w/o proper testing, we’ll fix it

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

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

Release v0.37.0 · cloudposse/terraform-aws-ec2-instanceattachment image

Adding SSM patch support @jamengual (#103) what Add SSM log bucket access Add SSM policy for Patch Manager Allow for Custom SSM policy why To be able to integrate with SSM patch and log the patc…

matt.bernard2006 avatar
matt.bernard2006

@MattyB

party_parrot1
1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

oops sorry, @MattyB ^

2021-06-28

kevcube avatar
kevcube

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

Remove unused template provider by kevcube · Pull Request #109 · cloudposse/terraform-aws-elasticsearchattachment image

what Remove deprecated and unused template plugin why This provider wasn't being used at all, and a build isn't provided for darwin-arm64, so it broke usage of this module on that platfo…

1
1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

@kevcube thank you for the PR

Remove unused template provider by kevcube · Pull Request #109 · cloudposse/terraform-aws-elasticsearchattachment image

what Remove deprecated and unused template plugin why This provider wasn't being used at all, and a build isn't provided for darwin-arm64, so it broke usage of this module on that platfo…

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)
cloudposse/terraform-aws-elasticsearchattachment image

Terraform module to provision an Elasticsearch cluster with built-in integrations with Kibana and Logstash. - cloudposse/terraform-aws-elasticsearch

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

and can you please also run

make init
make github/init
make readme
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

to update all GitHub actions to the latest versions

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

thanks

kevcube avatar
kevcube

make readme is failing on my machine, but I was able to run make github/init

1
    keyboard_arrow_up