#pr-reviews (2022-04)

Pull Request Reviews for Cloud Posse Projects

2022-04-08

Chris Dobbyn avatar
Chris Dobbyn
05:18:12 PM

https://github.com/cloudposse/terraform-aws-multi-az-subnets/pull/55

what

• Add IPv6 subnet provisioning • Assumes that the subnet created at the end must be a /64 (as per AWS). • Only applies to public subnets. This could be added to private (with no default destination route ::/0) if others think this is necessary. There is no concept of NAT in AWS VPC IPv6.

why

• We have a requirement to offer dualstack workloads. • Today we have to do this manually and if using this module, separate subnets need to be created.

references

• closes #43

1

2022-04-11

jose.amengual avatar
jose.amengual
11:25:44 PM

https://github.com/cloudposse/terraform-aws-cloudwatch-logs/pull/32

what

• Add policy boundary to cloudwatch IAM resources • Updating to new tags_enabled variable from I am role module

why

• to pass policy_boundary and to add the ability to disable tags for IAM resources

references

• cloudposse/terraform-aws-iam-role#44

jose.amengual avatar
jose.amengual
05:47:55 AM

https://github.com/cloudposse/terraform-aws-lambda-function/pull/7

what

• Adding function_name output

why

• Used for API gateway and others

2022-04-14

nnsense avatar
nnsense
10:31:39 AM

Hi guys, here’s a PR for adding a “single NGW” option to subnets module, useful during development to save some money. Feel free to reject it if you don’t feel this option should be available, or let me know if there’s anything I should change, thanks! https://github.com/cloudposse/terraform-aws-dynamic-subnets/pull/154

PS - I’ve tested it creating a VPC with a single NGW, then changed to multiple and back to single changing the new variable

what

• Add the option to deploy a single NGW instead of multiple across AZs

why

• Save money during development phase.

jose.amengual avatar
jose.amengual
05:02:54 PM

in this scenario the instances in the AZ different than the NAT gateway will pay for network traffic, no? and they will be a pit slower to reach the internet

what

• Add the option to deploy a single NGW instead of multiple across AZs

why

• Save money during development phase.

nnsense avatar
nnsense
05:15:07 PM

Sure, but the idea behind this is to have the option to deploy just one NGW when you’re developing/testing using just a single instance. In my case having 3 NGW was just a waste of money, we (as a company) were paying over $60 a month for the two unused NGWs, it may seems not much but for our tiny small company it was something to consider

jose.amengual avatar
jose.amengual
05:19:14 PM

do not get me wrong, I agree and I still do it

nnsense avatar
nnsense
05:21:17 PM

Nono, you made a good point, I know my case is a bit uncommon (at least for companies with money) that’s why it’s a proposal

jose.amengual avatar
jose.amengual
05:22:05 PM

lol

jose.amengual avatar
jose.amengual
05:22:45 PM

I have work in some many companies that did not have money for nat gateways……and they wanted redundancy

2022-04-19

Chris Dobbyn avatar
Chris Dobbyn
04:59:13 AM

Hey all, here’s an update to an old PR (from before I knew this channel existed). It adds the ability for multi-az-subnets module to use full, short, or fixed naming (this is copying the work done in dynamic subnets module): https://github.com/cloudposse/terraform-aws-multi-az-subnets/pull/56

I didn’t really change anything except for adding tests.

Defaulted to “full” in order to not break previous deploys.
This was all @Nuru, I’m just copying it to here.
cloudposse/terraform-aws-dynamic-subnets#100

what

• This allows availability zones to be changed to decrease their length

why

• Depending on how you configure the null label / context you could end up with resources with very long names.

references

n/a

1
Chris Dobbyn avatar
Chris Dobbyn
04:58:20 PM

@jose.amengual hoping you can have another look at this one.

Defaulted to “full” in order to not break previous deploys.
This was all @Nuru, I’m just copying it to here.
cloudposse/terraform-aws-dynamic-subnets#100

what

• This allows availability zones to be changed to decrease their length

why

• Depending on how you configure the null label / context you could end up with resources with very long names.

references

n/a

2022-04-20

2022-04-22

RB avatar
RB
01:11:22 PM

https://github.com/cloudposse/terraform-aws-s3-bucket/pull/148

what

• Use object lock enabled

why

• Deprecation of dynamic object_lock_configuration for object_lock_enabled

│ Warning: Argument is deprecated
│
│   with module.bucket.aws_s3_bucket.default,
│   on .terraform/modules/bucket/main.tf line 30, in resource "aws_s3_bucket" "default":
│   30: resource "aws_s3_bucket" "default" {
│
│ Use the top-level parameter object_lock_enabled and the aws_s3_bucket_object_lock_configuration resource instead

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket#object_lock_enabled • Previous PR #144 (this did not use object_lock_enabled and only removed the dynamic)

RB avatar
RB
11:02:32 PM

cc: @Jeremy G (Cloud Posse) we got 2 prs before for a similar fix so id like to merge to prevent future prs for this but it only solves a deprecation.

would you rather we set this as a no-release and it will be auto released with the next change? or is it fine to release as is?

what

• Use object lock enabled

why

• Deprecation of dynamic object_lock_configuration for object_lock_enabled

│ Warning: Argument is deprecated
│
│   with module.bucket.aws_s3_bucket.default,
│   on .terraform/modules/bucket/main.tf line 30, in resource "aws_s3_bucket" "default":
│   30: resource "aws_s3_bucket" "default" {
│
│ Use the top-level parameter object_lock_enabled and the aws_s3_bucket_object_lock_configuration resource instead

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket#object_lock_enabled • Previous PR #144 (this did not use object_lock_enabled and only removed the dynamic)

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)
12:10:10 AM

@RB I want some serious testing done to make sure the change does not cause Terraform to try to replace existing buckets. (See comment on the PR.) Imagine the response if someone upgrades the module from 1.0.0 to 1.0.1 and it results in their S3 bucket being destroyed and replaced. Other than that, though, it looks good.

RB avatar
RB
01:51:35 PM

https://github.com/cloudposse/terraform-aws-iam-role/pull/45

what

☑︎ Add enabled check to data source ☑︎ Add TestExamplesCompleteDisabled check

why

• Prevent creation if enabled is false

references

• cloudposse/terraform-aws-s3-bucket#148

1
RB avatar
RB
11:01:21 PM

thanks @Matt Gowie!

what

☑︎ Add enabled check to data source ☑︎ Add TestExamplesCompleteDisabled check

why

• Prevent creation if enabled is false

references

• cloudposse/terraform-aws-s3-bucket#148

1
RB avatar
RB
02:04:05 PM

https://github.com/cloudposse/terraform-aws-sqs-queue/pull/2

what

• Initialize sqs-queue module • Used list(string) instead of string due to new conventions seen in security-group module (see ref below)

why

• Wrap sqs queue with context to take advantage of name, namespace, environment, attributes, enabled, etc

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sqs_queue • See explanation in the sg release of why we use single item list(string) https://github.com/cloudposse/terraform-aws-security-group/releases/tag/0.4.0

commands

$ make readme
$ cd test/src
$ make all
--- PASS: TestExamplesComplete (31.26s)
PASS
ok  	github.com/cloudposse/terraform-aws-sqs-queue	31.444s
    keyboard_arrow_up