#pr-reviews (2021-07)

Pull Request Reviews for Cloud Posse Projects

2021-07-16

azec avatar
azec avatar

It was previously opened under #32 but because I messed up fork rebasing badly by pointing it to wrong cloudposse repo when adding upstream git remote, I had to reopen it after sorting out our fork….

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

Updated PR to fix tests and docs …

2021-07-19

azec avatar

Ok, I am now waiting for some more and from administrators on the above 2 PRs …

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

i’ll review later today, thanks @azec

azec avatar

thanks!

azec avatar

@Andriy Knysh (Cloud Posse) & @jose.amengual, thanks for helping merge #35 .

I have rebased , reworked #34 based on your comments: https://github.com/cloudposse/terraform-aws-sns-topic/pull/34

Making Encryption configuration of SNS easier by azec-pdx · Pull Request #34 · cloudposse/terraform-aws-sns-topicattachment image

NOTE: Previously opened and discussed as #32 , but had to open new PR because of the bad upstream fork rebasing (accidentally pointed to another unrelated CP TF module and ruined git history). what…

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

@azec thanks again, reviewed

azec avatar

@Andriy Knysh (Cloud Posse), I have made updates again, please check it out: https://github.com/cloudposse/terraform-aws-sns-topic/pull/34/files

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

approved, thanks @azec

azec avatar

Thank you again!

2021-07-20

Florian SILVA avatar
Florian SILVA

Hello guys ! Could somebody take time for a quick review of this MR I did a moment ago ? https://github.com/cloudposse/terraform-aws-elastic-beanstalk-environment/pull/182 It has been ready for a while and is now well tested on my side. Only a strange bridgecrew error failing but with no error.

Feat/add nlb by florian0410 · Pull Request #182 · cloudposse/terraform-aws-elastic-beanstalk-environmentattachment image

what Add NLB support in the module Set default protocol to TCP in case that loadbalancer_type == "network" S3 logs and Security Groups are not valid for Network ELB. HealthCheckPath appl…

1
Matt Gowie avatar
Matt Gowie
02:50:55 PM

@Florian SILVA Sorry it’s small but here is the thread with BridgeCrew regarding why they’re failing PR checks.

The tl;dr; is that BridgeCrew fails when the PR is from a fork (which is all PRs that don’t come from folks in this channel, so 99% of em).

1
kevcube avatar
kevcube

Hey gang, made a PR to aws-ecs-web-app which enables setting a custom policy/role for the tasks. Please when you can! TY https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/155

Enable setting custom task policy/role by kevcube · Pull Request #155 · cloudposse/terraform-aws-ecs-web-appattachment image

what We needed to give our container IAM permissions to perform AWS tasks. why We're deploying an internal app that needed to be able to operate on S3 files. We needed to be able to give the…

1
1
kevcube avatar
kevcube

Hey @Matt Gowie make readme succeeded this time

My Go setup on personal machine wasn’t working right, ran it in docker and it’s good to go now

Enable setting custom task policy/role by kevcube · Pull Request #155 · cloudposse/terraform-aws-ecs-web-appattachment image

what We needed to give our container IAM permissions to perform AWS tasks. why We're deploying an internal app that needed to be able to operate on S3 files. We needed to be able to give the…

Matt Gowie avatar
Matt Gowie

Checking… Thanks for the Ping @kevcube

1
Matt Gowie avatar
Matt Gowie

Merged

2021-07-21

2021-07-22

2021-07-28

Paul Robinson avatar
Paul Robinson

hi all, please could I have on my PR? https://github.com/cloudposse/terraform-aws-transit-gateway/pull/11 @Andriy Knysh (Cloud Posse) @Maxim Mironenko (Cloud Posse) this is my first contribution of a few in this repo and I may need some input on my follow on (specifically how to achieve multiple route tables/associations) if you have some time please?

Fix module disabled still creates infrastructure by paulrob-100 · Pull Request #11 · cloudposse/terraform-aws-transit-gatewayattachment image

what Module should not create infrastructure if enabled = false Fixed test failing due to tgw apply dependency on the vpc/subnets apply Added new test/fixtures for enabled/disabled variables Base …

Paul Robinson avatar
Paul Robinson

@Andriy Knysh (Cloud Posse) @RB (Ronak) (Cloud Posse) the failing test is a problem that will prevent the merge. I’ve seen this a few times now in my local testing. The tgw id exists but the API calls in the AWS provider barfs. What are your thoughts on how to proceed please?

Fix module disabled still creates infrastructure by paulrob-100 · Pull Request #11 · cloudposse/terraform-aws-transit-gatewayattachment image

what Module should not create infrastructure if enabled = false Fixed test failing due to tgw apply dependency on the vpc/subnets apply Added new test/fixtures for enabled/disabled variables Base …

Paul Robinson avatar
Paul Robinson

This might be related, but this failing test is run in a single account. https://github.com/hashicorp/terraform-provider-aws/issues/10025

EC2 Transit Gateway VPC Attachment "Transitgateway ID ' deos not exist" when attaching to Shared Transit Gateway from another AWS Account · Issue #10025 · hashicorp/terraform-provider-awsattachment image

Hi, I am having issues with attaching a Transit Gateway to a VPC as it is being provisioned. The first time I run an apply I get the error message for each route table. module.vpc.aws_route.vpn_rou…

Paul Robinson avatar
Paul Robinson
aws_route: better error handling for invalid TGW IDs · Issue #13830 · hashicorp/terraform-provider-awsattachment image

Description I noticed an area where error handling can be improved for aws_route: I accidentally used the id attribute of a aws_ec2_transit_gateway_vpc_attachment instead of the transit_gateway_id …

Paul Robinson avatar
Paul Robinson

I pushed a potential fix after reviewing that last link

Paul Robinson avatar
Paul Robinson

@Andriy Knysh (Cloud Posse) @RB (Ronak) (Cloud Posse) I think this PR is just waiting the final nod before merge. Anything else outstanding?

2021-07-29

z0rc3r avatar

Hi, looking for review https://github.com/cloudposse/terraform-aws-eks-cluster/pull/123. This is quite crucial, as kube_exec_auth method doesn’t work with latest AWS CLI v1 without this change.

Allow to set api_version for kube_exec_auth by z0rc · Pull Request #123 · cloudposse/terraform-aws-eks-clusterattachment image

what Allow to set api_version for kube_exec_auth. Maintain backwards compatibility by using older API version. why Latest AWS CLI switched API version from client.authentication.k8s.io/v1alpha1 to …

1
    keyboard_arrow_up