#pr-reviews (2022-06)
Pull Request Reviews for Cloud Posse Projects
2022-06-01
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
Hello, i have a PR that needs a review, https://github.com/cloudposse/terraform-aws-rds-cluster/pull/141 for missing support storage variables in rds cluster. thank you
2022-06-03
![Brian Ojeda avatar](https://avatars.slack-edge.com/2021-05-21/2091552027955_105f68e1608ac63c274a_72.jpg)
what
• add availability_zone_relocation_enabled
why
• allow users to enable
• defaults to null
if the aws provider is v4.6 or newer
• this is not a desired affect so allow it to be configiable
references
2022-06-06
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
cc: @matharoo
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
I looked at this PR and it seems like we need to update the examples/complete and other examples in order to get the tests to pass
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
The tests install the aws 3.x provider instead of the 4.x provider and the 4.x provider is the version that contains the new inputs
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
thanks for the review i have updated tests and updated aws provider
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
please confirm tests bats works locally, it appears both are failing
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
most likely due to a module version within examples that needs to be updated
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
ok thanks, I have updated the module versions and can confirm the test passes on my local:
ok 5 check if terraform code is valid
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
@RB the test failed again due to moved block in subnet module, i have updated the tf version for test to get that fixed. Need to re-run the test
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Please run the tests locally to confirm they work
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
I ran them in the PR, made some changes, and I see this error now
│Error: error creating RDS cluster: InvalidParameterCombination: StorageType isn’t supported for DB engine aurora.
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
It’s using the latest aws provider
TestExamplesComplete 2022-06-07T2050Z command.go - Installed hashicorp/aws v4.17.1 (signed by HashiCorp)
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
I can see this exists but unsure why this error is thrown https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/rds_cluster#storage_type
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Perhaps the storage type is not available for db engine aurora, maybe it’s only available for other db types
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
yes that seems like it, it does not seem to support the storage_type option when aurora, in their example also the engine is set to mysql
when the storage_type is set to io1
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
also found this https://stackoverflow.com/a/55960215/1380358
I am currently working on deploying a Aurora postgres instance in AWS thanks to Terraform. Here is my declaration resource “aws_db_instance” “postgreDatabase” { name = “validName” storage_typ…
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
i set it to null by default
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
sounds good thanks
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
i don’t know why test/bats is failing
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
yeah i tried looking into it but cannot get similar results locally.
ON the other hand terratest
is failling due to extra quotes that are being added by the terraform to output:
examples_complete_test.go:42:
Error Trace: examples_complete_test.go:42
Error: Not equal:
expected: "172.16.0.0/16"
actual : "\"172.16.0.0/16\""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-172.16.0.0/16
+"172.16.0.0/16"
....
....
....
examples_complete_test.go:58:
Error Trace: examples_complete_test.go:58
Error: Not equal:
expected: "eg-test-rds-cluster-27829"
actual : "\"eg-test-rds-cluster-27829\""
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-eg-test-rds-cluster-27829
+"eg-test-rds-cluster-27829"
It looks like this started happening after v0.13 of terraform.. Some people have reported similar issue here
So the examples_complete_test.go
needs to be updated if this is the case to not include quotes and then re run the test
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
but not including quotes there leads to invalid code. maybe a bug in the https://github.com/stretchr/testify stack.
A toolkit with common assertions and mocks that plays nicely with the standard library
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
Hi @RB, all tests pass on the PR , is there anything else that needs to be done for the change to get reviewed and merged for next release?
what
☑︎ Add support for storage variables storage_type
, iops
and allocated_storage
.
☑︎ Fix broken tests
why
• Be able to specify the size and type of storage of the database in the cluster.
references
• closes #140
• aws_rds_cluster
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
![matharoo avatar](https://avatars.slack-edge.com/2022-06-01/3609701989443_371835b80cdab8322f8a_72.png)
2022-06-07
2022-06-08
2022-06-09
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
request review for https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/183
This PR contains the following updates:
Configuration
Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).
Automerge: Disabled by config. Please merge this manually once you are satisfied.
Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.
Ignore: Close this PR and you won’t be reminded about this update again.
☐ If you want to rebase/retry this PR, click this checkbox. Warning: custom changes will be lost.
This PR has been generated by Mend Renovate. View repository job log here.
2022-06-14
2022-06-15
![Dmytro avatar](https://secure.gravatar.com/avatar/7dca0745e849a88034dbf43f0d2df862.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0002-72.png)
Hi everyone! Please see this PR: https://github.com/cloudposse/terraform-aws-elasticache-memcached/pull/49
fix conflict with using custom security group in associated_security_group_ids and parameter create_security_group is false
2022-06-16
![Bartłomiej Szostek avatar](https://avatars.slack-edge.com/2021-11-22/2753448276549_34b81da4fcc87eca1c84_72.jpg)
Hi all! Could I get some attention on this PR? All comments and merge conflicts have been resolved. It’s just waiting for approval and merge https://github.com/cloudposse/terraform-aws-route53-cluster-zone/pull/49
what
• Provide variables to configure TTL for SOA and NS records • Original PR: Change default values of TTL for NS from 30 seconds to 2 days (172800 seconds) • Original PR: Change default values of TTL for SOA from 60 seconds to 15 minutes (900 seconds) • Preserve previously-hardcoded TTLs as defaults (@osterman @korenyoni) — see below • Added the tenant context variable to the zone name template - maybe someone will need it :)
why
Original PR: These values were hardcoded and with too small default values. DNS is supposed to be a cache, and having a low TTL of records like NS or SOA contradicts that idea.
The previously-hardcoded low TTLs are values preferred by Cloud Posse as low TTLs lead to better availability in the event of a negative DNS response, such that the negative DNS answer will not be cached for an extended period of time (see @osterman’s comments in this thread).
AWS defaults for TTL:
NS - 172800
SOA - 900
references
Closes #46
Previous PR has some weird permission issues which I couldn’t solve, so I forked it again into a new repo and applied the same changes.
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Approved and ran tests
what
• Provide variables to configure TTL for SOA and NS records • Original PR: Change default values of TTL for NS from 30 seconds to 2 days (172800 seconds) • Original PR: Change default values of TTL for SOA from 60 seconds to 15 minutes (900 seconds) • Preserve previously-hardcoded TTLs as defaults (@osterman @korenyoni) — see below • Added the tenant context variable to the zone name template - maybe someone will need it :)
why
Original PR: These values were hardcoded and with too small default values. DNS is supposed to be a cache, and having a low TTL of records like NS or SOA contradicts that idea.
The previously-hardcoded low TTLs are values preferred by Cloud Posse as low TTLs lead to better availability in the event of a negative DNS response, such that the negative DNS answer will not be cached for an extended period of time (see @osterman’s comments in this thread).
AWS defaults for TTL:
NS - 172800
SOA - 900
references
Closes #46
Previous PR has some weird permission issues which I couldn’t solve, so I forked it again into a new repo and applied the same changes.
2022-06-17
2022-06-18
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
2022-06-20
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Solved a few open source github issues and bridgecrew issues. Please review.
https://github.com/cloudposse/terraform-aws-codebuild/pull/111
2022-06-22
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
Can I get another review here https://github.com/cloudposse/terraform-aws-backup/pull/39
what
• Add support for multiple backup schedules per backup plan • Simplify configuration in case multiple schedules per backup plan are necessary • Maintain backwards compatibility
why
• One has to have multiple instances of this module to have more than one backup schedule, each with a different value for name • In my case, I need to have three schedules in a backup plan: daily, weekly, monthly
references
• Closes #23
2022-06-23
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
finally added a test to this old PR
https://github.com/cloudposse/terraform-aws-route53-cluster-zone/pull/51
what
• because var.parent_zone_name
is not required by the module as of #33, let the module work if it’s not provided.
why
• to use the module with a parent zone that is managed elsewhere,
parent_zone_record_enabled = false
zone_name = "$${stage}.example.com
INSTEAD OF…
parent_zone_name = "example.com"
parent_zone_record_enabled = false
zone_name = "$${stage}.$${parent_zone_name}"
alternatives considered
"$${parent_zone_name}", coalesce(join("", data.aws_route53_zone.parent_zone.*.name), var.parent_zone_name, "no_parent_zone_name")),
• I consider removing one of var.parent_zone_id
or var.parent_zone_name
because offering both can lead to conflict or confusion. If var.parent_zone_id
is removed, then we can always rely on var.parent_zone_name
instead of coalescing with the output of the data.aws_route53_zone.parent_zone
2022-06-29
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
![Yonatan Koren avatar](https://avatars.slack-edge.com/2023-01-08/4612627141524_cae57b3715b3fb292bd1_72.jpg)
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
cc: @Jeremy G (Cloud Posse) could you review this please ?
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@RB The PR was merged 2 days ago
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
oh… lol.. i thought it was still blocked, sorry about that
2022-06-30
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
boolean flag to enable intra-security group communication.