#pr-reviews (2021-08)

Pull Request Reviews for Cloud Posse Projects

2021-08-26

Florian SILVA avatar
Florian SILVA

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-environment attachment 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

Thank you for your quick review @RB (Ronak) (Cloud Posse). Just answered to your questions and solved your inputs.

Feat/instance profile iam by florian0410 · Pull Request #187 · cloudposse/terraform-aws-elastic-beanstalk-environment attachment 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 (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

np. commented and deferred to other reviewers

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

thanks again for the contribution. hope it gets approved soon

2021-08-24

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@RB (Ronak) (Cloud Posse) you’re knocking out tons of fixes!

fidget_spinner1

2021-08-17

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

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-cluster attachment 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 (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Use `templatefile` by nitrocode · Pull Request #94 · cloudposse/terraform-aws-tfstate-backend attachment image

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

1
1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Add zone id by nitrocode · Pull Request #49 · cloudposse/terraform-aws-acm-request-certificate attachment 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-14

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Ensure target group and load balancer names cannot exceed their max length by nitrocode · Pull Request #96 · cloudposse/terraform-aws-alb attachment 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-alb attachment 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-11

2021-08-10

Makeshift (Connor Bell) avatar
Makeshift (Connor Bell)

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-label attachment 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

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 systems attachment 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-bucket attachment 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-bucket attachment 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-bucket attachment 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-06

Gerald avatar
Gerald
task definition volume by jtdev-gerald · Pull Request #142 · cloudposse/terraform-aws-ecs-container-definition attachment 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-03

Scott avatar
Scott

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-bucket attachment 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

commented

Add kms actions by bwmetcalf · Pull Request #32 · cloudposse/terraform-aws-vpc-flow-logs-s3-bucket attachment 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-02

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)
Test `cache_bucket_suffix_enabled` by nitrocode · Pull Request #92 · cloudposse/terraform-aws-codebuild attachment image

what Test cache_bucket_suffix_enabled why Ensure it works references Closes #91

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

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-codepipeline attachment 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
    keyboard_arrow_up