#pr-reviews (2023-03)
Pull Request Reviews for Cloud Posse Projects
2023-03-01
@Gocho has joined the channel
2023-03-02
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
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
Terraform test is getting failed on repos because vpc limit is reached for the enviroment.
Thanks! Yea we will get this fixed next week. Not much happening this weekend
Thanks @Erik Osterman (Cloud Posse) for your prompt reply
2023-03-04
2023-03-07
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
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
Hi - looking to get this reviewed, please! https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/189
@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)
Interesting, because the code (as-is) does work.
hmm maybe diff TF versions (and maybe TF improved it in the latest versions)
2023-03-14
2023-03-18
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
For the one regarding master password, maybe an optional resource shoudl be aded to store in SSM like the RDS module does.
Yea better to write to SSM
Any thoughts on this?
@Ben Smith (Cloud Posse) @Jeremy White (Cloud Posse) @Dan Miller (Cloud Posse)
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
the performance insights fellow seems straightforward.
agreed also. Approved and merging the performance insights PR
amazon is weird though. They call the flag in aurora performance_insights_enabled
no fault of the PR, obviously. But a reason to consider a naming convention to unite both at a future date
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) has joined the channel
@Jeremy White (Cloud Posse) has joined the channel
@Ben Smith (Cloud Posse) has joined the channel
2023-03-22
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’…
@Linda Pham (Cloud Posse) can you find someone to help review https://github.com/cloudposse/terraform-aws-eks-node-group/pull/139
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) has joined the channel
2023-03-23
2023-03-28
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
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
Hey Joe! No problem! I appreciate you checking that out!
2023-03-30
2023-03-31
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