#pr-reviews (2021-08)

Pull Request Reviews for Cloud Posse Projects

2021-08-02

RB avatar
RB
12:36:06 PM

quick test pr review https://github.com/cloudposse/terraform-aws-codebuild/pull/92

Test `cache_bucket_suffix_enabled` by nitrocode · Pull Request #92 · cloudposse/terraform-aws-codebuildattachment image

what Test cache_bucket_suffix_enabled why Ensure it works references Closes #91

1
RB avatar
RB
12:36:23 PM

and one liner revert to keep backwards compatibility https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/81

Set `cache_bucket_suffix_enabled` to true again by nitrocode · Pull Request #81 · cloudposse/terraform-aws-ecs-codepipelineattachment image

what Set cache_bucket_suffix_enabled to true again why cache_bucket_suffix_enabled=true seems to apply correctly in the terraform-aws-codebuild repo (see cloudposse/terraform-aws-codebuild#92) …

1

2021-08-03

Scott avatar
Scott
05:51:27 PM

Not my PR, but am effected by the issue and following this resolution.. https://github.com/cloudposse/terraform-aws-vpc-flow-logs-s3-bucket/pull/32

Add kms actions by bwmetcalf · Pull Request #32 · cloudposse/terraform-aws-vpc-flow-logs-s3-bucketattachment image

what Add permissions to allow key to be used why The current policy does not allow key to be used to access encrypted objects. Added kms:Decrypt kms:GenerateDataKey as most if not all other a…

1
1
jose.amengual avatar
jose.amengual
03:14:29 AM

commented

Add kms actions by bwmetcalf · Pull Request #32 · cloudposse/terraform-aws-vpc-flow-logs-s3-bucketattachment image

what Add permissions to allow key to be used why The current policy does not allow key to be used to access encrypted objects. Added kms:Decrypt kms:GenerateDataKey as most if not all other a…

1

2021-08-06

Gerald avatar
Gerald
04:07:51 PM

Hi please take a look on this..thanks https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/142

task definition volume by jtdev-gerald · Pull Request #142 · cloudposse/terraform-aws-ecs-container-definitionattachment image

what volume argument not supported in task definition why I need to mount my container on task definition volume to support lookup on docker Labels references https://registry.terraform.io/provider…

1

2021-08-10

Makeshift (Connor Bell) avatar
Makeshift (Connor Bell)
08:55:23 AM

Could I get a sanity check on this one when people are awake? Thanks https://github.com/cloudposse/terraform-null-label/pull/131

Add a 'static_tags' option to disable automatic merging of tags by Makeshift · Pull Request #131 · cloudposse/terraform-null-labelattachment image

what Adds a new variable static_tags, set to false by default, which stops the module from merging in new tags and instead uses the tags variable in the input as-is, including handing it off to ot…

Julian Gog avatar
Julian Gog
02:59:00 PM

cross-post BUG:wave: I’m here! What’s up? I was about to create a bug ticket and and saw the link to your slack. So I want to make sure its a Bug before opening a ticket. its about the terraform-aws-s3-bucket. if you specify the privileged_principal_arns option it will never create a bucket policy. Is this a wanted behaviour, since the a aws_iam_policy_document is created? My guess is that in the the privileged_principal_arns is missing in the count option here:

resource "aws_s3_bucket_policy" "default" {
  count      = local.enabled && (var.allow_ssl_requests_only || var.allow_encrypted_uploads_only || length(var.s3_replication_source_roles) > 0 || var.policy != "") ? 1 : 0
  bucket     = join("", aws_s3_bucket.default.*.id)
  policy     = join("", data.aws_iam_policy_document.aggregated_policy.*.json)
  depends_on = [aws_s3_bucket_public_access_block.default]
}

ok I am almost 100% sure its a bug, so here are the issue and the PR
Bug-Issue//github.com/cloudposse/terraform-aws-s3-bucket/issues/100>
PR//github.com/cloudposse/terraform-aws-s3-bucket/pull/101>

GitHub - cloudposse/terraform-aws-s3-bucket: Terraform module that creates an S3 bucket with an optional IAM user for external CI/CD systemsattachment image

Terraform module that creates an S3 bucket with an optional IAM user for external CI/CD systems - GitHub - cloudposse/terraform-aws-s3-bucket: Terraform module that creates an S3 bucket with an opt…

terraform-aws-s3-bucket/main.tf at 3c2cde977d9192f8c7d1c0b42d63c406f5e0a8cc · cloudposse/terraform-aws-s3-bucketattachment image

Terraform module that creates an S3 bucket with an optional IAM user for external CI/CD systems - terraform-aws-s3-bucket/main.tf at 3c2cde977d9192f8c7d1c0b42d63c406f5e0a8cc · cloudposse/terraform-…

Bucket policy not created when · Issue #100 · cloudposse/terraform-aws-s3-bucketattachment image

Describe the Bug If the privileged_principal_arns option is used it will never create a bucket policy. Expected Behavior If the privileged_principal_arns option is used it will create a bucket poli…

fix privileged_principal_arns not creating bucket policy by avendretter · Pull Request #101 · cloudposse/terraform-aws-s3-bucketattachment image

what The privileged_principal_arns option is not creating a bucket policy. why A check at the s3_bucket_policy is missing references #100

1

2021-08-11

2021-08-14

RB avatar
RB
09:34:27 AM

Could i please get a review on these small prs

https://github.com/cloudposse/terraform-aws-alb/pull/96/files

And

https://github.com/cloudposse/terraform-aws-alb/pull/97/files

Ensure target group and load balancer names cannot exceed their max length by nitrocode · Pull Request #96 · cloudposse/terraform-aws-albattachment image

what Ensure target group and load balancer names cannot exceed their max length Expose *_max_length variables in case AWS changes their max length in the future Override load_balancer_name, simila…

Add `module.this.enabled` check to listener's `count` by nitrocode · Pull Request #97 · cloudposse/terraform-aws-albattachment image

what Add module.this.enabled check to listener’s count why To ensure this module can be properly disabled using enabled = false references Closes #90 Closes #94

1

2021-08-17

RB avatar
RB
02:02:47 AM

This pr adds a timeout after the eks is created in order to install coredns kube addon.

Please review

https://github.com/cloudposse/terraform-aws-eks-cluster/pull/126

More addons by nitrocode · Pull Request #126 · cloudposse/terraform-aws-eks-clusterattachment image

what Test all recommended addons coredns, kube-proxy, and vpc-cni why It's possible additional changes may need to be made to support these addons We may need to add a depends_on to the eks …

RB avatar
RB
02:41:51 AM

And this is a small pr for template file

https://github.com/cloudposse/terraform-aws-tfstate-backend/pull/94

Use `templatefile` by nitrocode · Pull Request #94 · cloudposse/terraform-aws-tfstate-backendattachment image

what Use templatefile why template_file is a deprecated data source references Closes #34

1
1
RB avatar
RB
05:00:41 AM

This pr fixes the acm terratests and adds zone id

https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/49/files

Add zone id by nitrocode · Pull Request #49 · cloudposse/terraform-aws-acm-request-certificateattachment image

what Add zone_id why Create an implicit link between zone creation and acm creation This gives the consumer the option to use domain name, zone name, or zone id to use the data source to retriev…

1
1

2021-08-24

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)
02:33:06 AM

@RB you’re knocking out tons of fixes!

1

2021-08-26

Florian SILVA avatar
Florian SILVA
02:31:39 PM

Hello guys, When somebody is ready, could I have a review on this feature ? https://github.com/cloudposse/terraform-aws-elastic-beanstalk-environment/pull/187

Feat/instance profile iam by florian0410 · Pull Request #187 · cloudposse/terraform-aws-elastic-beanstalk-environmentattachment image

what Enhancement of #107 + #113, due to original developer seemingly abandoning the original PR. Adds service_role_name as another 'override', like instance_role_name is in the original P…

1
Florian SILVA avatar
Florian SILVA
03:28:58 PM

Thank you for your quick review @RB. Just answered to your questions and solved your inputs.

Feat/instance profile iam by florian0410 · Pull Request #187 · cloudposse/terraform-aws-elastic-beanstalk-environmentattachment image

what Enhancement of #107 + #113, due to original developer seemingly abandoning the original PR. Adds service_role_name as another 'override', like instance_role_name is in the original P…

RB avatar
RB
03:43:46 PM

np. commented and deferred to other reviewers

RB avatar
RB
03:43:58 PM

thanks again for the contribution. hope it gets approved soon

    keyboard_arrow_up