#pr-reviews (2023-03)
Pull Request Reviews for Cloud Posse Projects
2023-03-01
![Gocho avatar](https://secure.gravatar.com/avatar/1b35335d3da8b23ebfc69247c96c3451.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0012-72.png)
@Gocho has joined the channel
2023-03-02
![cinacio avatar](https://secure.gravatar.com/avatar/acd331270c3a8ac041684b777f04d0f7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0010-72.png)
Hi. I’ve been sent to this channel in case my PR needs a nudge, so here it is: https://github.com/cloudposse/terraform-aws-ec2-instance/pull/142
It’s almost there, but for some reason auto-format
fails
what
• Only set iops and throughput arguments for volume types supporting those options.
why
• According to the documentation for the aws_ebs_volume resource, the iops
argument is only valid for volumes types io1
, io2
& gp3
.
• According to the same documentation, the throughput
argument is only valid for volume type gp3
.
• Setting the throughput of a gp2
volume to “0” triggers the following error
Error: expected throughput to be in the range (125 - 1000), got 0
with module.instance.aws_ebs_volume.default[0],
on .terraform/modules/instance/main.tf line 178, in resource “aws_ebs_volume” “default”:
178: throughput = local.ebs_throughput
references
• closes #137
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Try running make pr/auto-format
locally
what
• Only set iops and throughput arguments for volume types supporting those options.
why
• According to the documentation for the aws_ebs_volume resource, the iops
argument is only valid for volumes types io1
, io2
& gp3
.
• According to the same documentation, the throughput
argument is only valid for volume type gp3
.
• Setting the throughput of a gp2
volume to “0” triggers the following error
Error: expected throughput to be in the range (125 - 1000), got 0
with module.instance.aws_ebs_volume.default[0],
on .terraform/modules/instance/main.tf line 178, in resource “aws_ebs_volume” “default”:
178: throughput = local.ebs_throughput
references
• closes #137
2023-03-03
![Nitin avatar](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Terraform test is getting failed on repos because vpc limit is reached for the enviroment.
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Thanks! Yea we will get this fixed next week. Not much happening this weekend
![Nitin avatar](https://secure.gravatar.com/avatar/b5ebdeb86a36df979cbf426d73d1ff00.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0017-72.png)
Thanks @Erik Osterman (Cloud Posse) for your prompt reply
2023-03-04
2023-03-07
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
another quick one. thanks in advance https://github.com/cloudposse/terraform-aws-ec2-instance/pull/151
what
• Use data.aws_ami.info only if it’s actually required
why
• Fix Your query returned no results” error once the AMI ID is not listed within AWS marketplace
references
2023-03-08
![Quentin BERTRAND avatar](https://avatars.slack-edge.com/2022-08-29/4028696878592_774493d7ba3d4c45009e_72.jpg)
Hello, I have this PR to review:
Feat: add option to not create target group
I guess it will need a procedure migration of state is this PR is accepted
what
Add option to not create default target group
why
Default target group can be useless.
references
I guess it will need a procedure migration of state is this PR is accepted.
2023-03-09
![Josh Baird avatar](https://avatars.slack-edge.com/2023-03-09/4910156913175_8cf5a146dddf6342ff31_72.jpg)
Hi - looking to get this reviewed, please! https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/189
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
@Josh Baird reviewed the PR, thanks for it, left a few comments )one of them is an annoying Tf feature, but we have to deal with it)
![Josh Baird avatar](https://avatars.slack-edge.com/2023-03-09/4910156913175_8cf5a146dddf6342ff31_72.jpg)
Interesting, because the code (as-is) does work.
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
hmm maybe diff TF versions (and maybe TF improved it in the latest versions)
2023-03-14
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
2023-03-18
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/50 https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/45
Some good additions to documentdb module
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
For the one regarding master password, maybe an optional resource shoudl be aded to store in SSM like the RDS module does.
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Yea better to write to SSM
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Any thoughts on this?
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Ben Smith (Cloud Posse) @Jeremy White (Cloud Posse) @Dan Miller (Cloud Posse)
![Dan Miller (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-08-12/2389147782305_5729c9d69c393852d209_72.jpg)
Yes we try to avoid writing any secrets to Terraform state. Instead write to SSM as Erik suggested and output the path to the parameter
![Jeremy White (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-10-14/4236950492513_ceab13cebd77d26f2ef6_72.jpg)
the performance insights fellow seems straightforward.
![Dan Miller (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-08-12/2389147782305_5729c9d69c393852d209_72.jpg)
agreed also. Approved and merging the performance insights PR
![Jeremy White (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-10-14/4236950492513_ceab13cebd77d26f2ef6_72.jpg)
amazon is weird though. They call the flag in aurora performance_insights_enabled
![Jeremy White (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-10-14/4236950492513_ceab13cebd77d26f2ef6_72.jpg)
no fault of the PR, obviously. But a reason to consider a naming convention to unite both at a future date
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Our best practice on booleans: https://docs.cloudposse.com/reference/best-practices/terraform-best-practices/#use-feature-flags-to-enabledisable-functionality
Our opinionated best-practices for Terraform
2023-03-21
![Dan Miller (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-08-12/2389147782305_5729c9d69c393852d209_72.jpg)
@Dan Miller (Cloud Posse) has joined the channel
![Jeremy White (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-10-14/4236950492513_ceab13cebd77d26f2ef6_72.jpg)
@Jeremy White (Cloud Posse) has joined the channel
![Ben Smith (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-08-11/2383898637441_289b6cfcbd0d178c8183_72.png)
@Ben Smith (Cloud Posse) has joined the channel
2023-03-22
![Jeremy White (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-10-14/4236950492513_ceab13cebd77d26f2ef6_72.jpg)
quick reminder that you can press period .
while on a pull request to let you add things like terraform highlighting, or to mark directories as ‘read’…
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Linda Pham (Cloud Posse) can you find someone to help review https://github.com/cloudposse/terraform-aws-eks-node-group/pull/139
![attachment image](https://user-images.githubusercontent.com/83597/210409914-789aa758-3d14-4e27-abdc-7694ec17cdb7.png)
what
• Adds support for 2019/2022 (windows lts) ami-types as per the aws sdk docs • Docs to support usage and link to ref docs https://docs.aws.amazon.com/eks/latest/userguide/windows-support.html
why
• Windows EKS Managed node support aws/containers-roadmap#584
references
• https://docs.aws.amazon.com/eks/latest/userguide/windows-support.html • https://docs.aws.amazon.com/eks/latest/userguide/eks-ami-versions-windows.html
Tested
module "eks_windows_node_group" {
# source = "cloudposse/eks-node-group/aws"
# version = "2.6.1"
source = "github.com/ChrisMcKee/terraform-aws-eks-node-group"
instance_types = ["t3.large", "t3a.large", "c5.large", "c6i.large", "m6i.large", "r6i.large"]
subnet_ids = [data.terraform_remote_state.network.outputs.private_subnets[1]]
min_size = 1
max_size = 1
desired_size = 1
cluster_name = module.eks_cluster.eks_cluster_id
kubernetes_version = var.kubernetes_version == null || var.kubernetes_version == "" ? [] : [var.kubernetes_version]
kubernetes_labels = var.labels
ami_type = "WINDOWS_CORE_2019_x86_64"
update_config = [{ max_unavailable = 1 }]
capacity_type = "SPOT"
kubernetes_taints = [{
key = "OS"
value = "Windows"
effect = "NO_SCHEDULE"
}]
node_role_arn = [aws_iam_role.worker_role_nt.arn]
node_role_cni_policy_enabled = false #We use the Service Account as per best practice
associated_security_group_ids = [data.terraform_remote_state.network.outputs.ops_ssh, aws_security_group.workers.id]
# Enable the Kubernetes cluster auto-scaler to find the auto-scaling group
cluster_autoscaler_enabled = true
context = module.windowslabel.context
# Ensure the cluster is fully created before trying to add the node group
module_depends_on = [module.eks_cluster.kubernetes_config_map_id]
# Ensure ordering of resource creation to eliminate the race conditions when applying the Kubernetes Auth ConfigMap.
# Do not create Node Group before the EKS cluster is created and the `aws-auth` Kubernetes ConfigMap is applied.
depends_on = [module.eks_cluster, module.eks_cluster.kubernetes_config_map_id]
create_before_destroy = true
node_role_policy_arns = ["arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore"]
block_device_mappings = [
{
"delete_on_termination" : true,
"device_name" : "/dev/xvda",
"encrypted" : true,
"volume_size" : 80,
"volume_type" : "gp3"
}
]
node_group_terraform_timeouts = [{
create = "40m"
update = null
delete = "20m"
}]
#Valid types are "instance", "volume", "elastic-gpu", "spot-instances-request", "network-interface".
resources_to_tag = ["instance", "volume", "spot-instances-request", "network-interface"]
}
![Linda Pham (Cloud Posse) avatar](https://avatars.slack-edge.com/2022-04-13/3374321180711_438a91ddd259ebac3590_72.jpg)
@Linda Pham (Cloud Posse) has joined the channel
2023-03-23
2023-03-28
![Anthony B avatar](https://secure.gravatar.com/avatar/6b3dd2a4d55567e7e33a1c3e8794a1c3.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0018-72.png)
hello! I was looking to get this PR reviewed https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/95 however there does seem to be an issue with permissions it looks like getting a github action to successfully run on my fork. Would I be able to get some advice? Thank you!
what
• custom origins can be created without error for using s3_origin_config • add the ability to add an s3 origin for use in routing traffic to error pages/custom error responses
why
• custom origins do not work with the current version of this module • allows a custom origin with s3 to be used as a secondary origin for error handling
references
• #74
• closes #74
![Joe Niland avatar](https://secure.gravatar.com/avatar/b90c8e752dd648ef229096c60ba2408f.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0022-72.png)
Hi @Anthony B I fixed that issue. The Github workflows were out of date, as they’ve been updated recently.
what
• custom origins can be created without error for using s3_origin_config • add the ability to add an s3 origin for use in routing traffic to error pages/custom error responses
why
• custom origins do not work with the current version of this module • allows a custom origin with s3 to be used as a secondary origin for error handling
references
• #74
• closes #74
![Anthony B avatar](https://secure.gravatar.com/avatar/6b3dd2a4d55567e7e33a1c3e8794a1c3.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0018-72.png)
Hey Joe! No problem! I appreciate you checking that out!
2023-03-30
2023-03-31
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
what
• remove dependency on template provider
why
• arm64 • also this provider was not pinned in versions.tf so that had to be fixed somehow
references
• closes #617