#pr-reviews (2024-07)
Pull Request Reviews for Cloud Posse Projects
2024-07-01
2024-07-02
![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)
Could I please get a review on https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/140 ?
what
• Bump version of cloudposse/repository-webhooks/github to latest • Require Terraform 1.3 and AWS provider v5
why
• Min version requirements should match Codebuild module, which this one depends on
references
• Will resolve one of the issues in #115
2024-07-03
2024-07-17
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Hey all! I have a few PRs open that I’d like a review on
• https://github.com/cloudposse/terraform-aws-rds-cluster/pull/221
• https://github.com/cloudposse/terraform-aws-tfstate-backend/pull/179
![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)
@Gabriela Campana (Cloud Posse)
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Andriy Knysh (Cloud Posse) reviewed the first two:
• https://github.com/cloudposse/terraform-aws-rds-cluster/pull/221
• https://github.com/cloudposse/terraform-aws-tfstate-backend/pull/179 I asked our team earlier today to review https://github.com/cloudposse/.github/pull/115
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Yep, I’m looking into the comments (and seeing if there’s a way I can run the pipeline tests locally to catch everything before I submit )
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
what
Fix the description truncation mechanism to handle sentences such as “Terraform module that provision an S3 bucket to store the terraform.tfstate
file”; or “Made in the U.S.A.”
why
Some CI jobs were failing that had something.other
in the description due to a shell script error caused by a non-closing backtick.
See this CI job as an example.
references
• https://github.com/cloudposse/terraform-aws-tfstate-backend/actions/runs/9797916695/job/27055456433
notes
The formatting was also modified by my IDE YAML formatter. Let me know if you’d prefer that I roll-back the formatting update.
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Igor Rodionov
![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 @Lennart Goedhart, we ended up opening up a separate PR
![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)
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
No worries. Thanks!
2024-07-19
2024-07-20
2024-07-22
![kevcube avatar](https://avatars.slack-edge.com/2022-02-16/3117652675890_e629b00375cf1d67c74d_72.jpg)
• https://github.com/cloudposse/terraform-aws-amplify-app/pull/32
• https://github.com/cloudposse/terraform-aws-amplify-app/pull/33 could someone please check these two PRs!
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Igor Rodionov @Andriy Knysh (Cloud Posse)
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
2024-07-23
![galais.jerome avatar](https://secure.gravatar.com/avatar/f59a7f1b66c854f862cf874823933a5b.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Hello,
I created a PR since 5 December 2023 for Cloudfront module (terraform-aws-cloudfront-cdn) to unlock OAC feature. Since this date I have forked this module to use OAC on my projects.
Please can you validate my PR to add that on the official module ?
PR: https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/112
Thank you
@Gabriela Campana (Cloud Posse)
what
• add origin_access_control_id optional variable to define origin_access_control on Cloudfront • add origin_access_control_id argument on cloudfront_distribution resource • Update the Readme with new vars
why
I done that because before you can’t use Origin access control with this module.
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Jeremy G (Cloud Posse)
what
• add origin_access_control_id optional variable to define origin_access_control on Cloudfront • add origin_access_control_id argument on cloudfront_distribution resource • Update the Readme with new vars
why
I done that because before you can’t use Origin access control with this module.
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
Hi @galais.jerome Thanks for bringing this up. I’ve just asked our engineers to review it. Will keep you posted
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
@galais.jerome I just ran tests on your PR and they falied with the error
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: ╷
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │ Error: Reference to undeclared resource
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: │ A managed resource "origin" "value" has not been declared in module.cdn.
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: ╵
TestExamplesComplete 2024-07-24T13:39:23Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Reference to undeclared resource
│
│ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
│ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
│
│ A managed resource "origin" "value" has not been declared in module.cdn.
╵}
apply.go:15:
Error Trace: apply.go:15
examples_complete_test.go:46
Error: Received unexpected error:
FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Reference to undeclared resource
│
│ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
│ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
│
│ A managed resource "origin" "value" has not been declared in module.cdn.
╵}
Test: TestExamplesComplete
TestExamplesComplete 2024-07-24T13:39:23Z retry.go:91: terraform [destroy -auto-approve -input=false -var attributes=["erpluf"] -var-file fixtures.us-east-2.tfvars -lock=false]
TestExamplesComplete 2024-07-24T13:39:23Z logger.go:66: Running command terraform with args [destroy -auto-approve -input=false -var attributes=["erpluf"] -var-file fixtures.us-east-2.tfvars -lock=false]
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: ╷
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │ Error: Reference to undeclared resource
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: │ A managed resource "origin" "value" has not been declared in module.cdn.
TestExamplesComplete 2024-07-24T13:39:25Z logger.go:66: ╵
TestExamplesComplete 2024-07-24T13:39:25Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Reference to undeclared resource
│
│ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
│ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
│
│ A managed resource "origin" "value" has not been declared in module.cdn.
╵}
destroy.go:11:
Error Trace: destroy.go:11
examples_complete_test.go:15
panic.go:523
testing.go:999
apply.go:15
examples_complete_test.go:46
Error: Received unexpected error:
FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Reference to undeclared resource
│
│ on ../../main.tf line 81, in resource "aws_cloudfront_distribution" "default":
│ 81: origin_access_control_id = lookup(origin.value, "origin_access_control_id", null)
│
│ A managed resource "origin" "value" has not been declared in module.cdn.
╵}
Test: TestExamplesComplete
--- FAIL: TestExamplesComplete (16.26s)
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
I think there are other problems with this module, regarding the distinction between origins, custom origins, and S3 origins. I’m not familiar enough with CloudFront to be sure, but we have both var.custom_origins
and top-level variables like var.origin_domain_name
, so I think any addition, like origin_access_control_id
has to be added in 2 places.
![Gabriela Campana (Cloud Posse) avatar](https://avatars.slack-edge.com/2023-05-17/5281506983315_fbbf3b358313efef4647_72.jpg)
@Yonatan Koren is the work you did recently for a Cloud Posse customer related to this?
![Yonatan Koren avatar](https://avatars.slack-edge.com/2023-01-08/4612627141524_cae57b3715b3fb292bd1_72.jpg)
It was very much related to this, but they stuck with OAI and not OAC (the latter not being available in our module, which this PR solves).
![galais.jerome avatar](https://secure.gravatar.com/avatar/f59a7f1b66c854f862cf874823933a5b.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Hello,
I make a commit to fix my PR. I putted variable on first location but not the second. I think it’s ok now.
Can you test @Igor Rodionov ?
Fixed commit: https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/112/commits/3216561425f11f21caf0009eff41fb07863755ff
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
![galais.jerome avatar](https://secure.gravatar.com/avatar/f59a7f1b66c854f862cf874823933a5b.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Thank you
2024-07-24
![Eligio Mariño avatar](https://secure.gravatar.com/avatar/fa27f4acd153397ff72a61a9d5fa47f7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0005-72.png)
Hello. Could someone review this PR please? https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/103
what
• Upgrade terraform-aws-provider to 5.29.0 • Add variable for storage_type in the test.
why
• storage_type was released in 5.29.0 but the current minimum version is 5.21.0. This change corrects the minimum required version to 5.29.0
references
closes #86
• https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.29.0
2024-07-25
![Carlos Lopes avatar](https://avatars.slack-edge.com/2024-07-25/7482934038052_37b18e41bbec9a634d4a_72.jpg)
trivial PR with a bug fix for the terraform-aws-backup
module
what
Attempt to get attribute from null value
error when a copy_action
is specified in a backup rule without any lifecycle options
steps to reproduce
Based on the complete example:
module "backup" {
source = "../.."
backup_resources = [module.efs.arn]
not_resources = var.not_resources
rules = [
{
name = "${module.this.name}-daily"
schedule = var.schedule
start_window = var.start_window
completion_window = var.completion_window
lifecycle = {
cold_storage_after = var.cold_storage_after
delete_after = var.delete_after
}
copy_action = {
destination_vault_arn = "arn:aws:backup:us-east-2:123456789000:backup-vault:my-vault"
}
}
]
backup_vault_lock_configuration = {
max_retention_days = 365
min_retention_days = 30
}
context = module.this.context
}
why
The following error should not happen:
╷
│ Error: Attempt to get attribute from null value
│
│ on ../../main.tf line 79, in resource "aws_backup_plan" "default":
│ 79: cold_storage_after = rule.value.copy_action.lifecycle.cold_storage_after
│ ├────────────────
│ │ rule.value.copy_action.lifecycle is null
│
│ This value is null, so it does not have any attributes.
╵
╷
│ Error: Attempt to get attribute from null value
│
│ on ../../main.tf line 80, in resource "aws_backup_plan" "default":
│ 80: delete_after = rule.value.copy_action.lifecycle.delete_after
│ ├────────────────
│ │ rule.value.copy_action.lifecycle is null
│
│ This value is null, so it does not have any attributes.
╵
2024-07-30
![Lennart Goedhart avatar](https://secure.gravatar.com/avatar/620a0a01846f308882ee2452414ca7d7.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Would we be able to get a new release of terraform-aws-tfstate-backend that includes some of the recently merged PRs? It’s not a blocker, since I’m using git SHAs to pull at the moment, but it would be nice to have.
Terraform module that provision an S3 bucket to store the terraform.tfstate
file and a DynamoDB table to lock the state file to prevent concurrent modifications and state corruption.
![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)
@Igor Rodionov are these not happening automatically?
Terraform module that provision an S3 bucket to store the terraform.tfstate
file and a DynamoDB table to lock the state file to prevent concurrent modifications and state corruption.
![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)
Looks like a bug
![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)
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
![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)
@Igor Rodionov did you fix the underlying issue?
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
nope. I had not time to research it https://github.com/cloudposse/terraform-aws-tfstate-backend/actions/runs/9797916695/job/27055456433
![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)
Oh, that should have been fixed by @Jeremy G (Cloud Posse) in https://github.com/cloudposse-github-actions/screenshot/pull/8
what
• Update Puppeteer to v22.13.1 • Properly quote string inputs and variables • Update README
why
• Fix 3 high severity vulnerabilities
• Avoid problems when string inputs contain characters like $
or ` that are meaningful to the shell
• Keep README current
references
• Security Advisory GHSA-3h5v-q93c-6h6q
• Example broken run due to use of backtick
• Puppeteer v22 remove waitForTimeout(milliseconds)
• Supersedes and closes #2
• Supersedes and closes #7
![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)
Maybe rerunning it would just work
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
there is no need to rerun it
![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)
how to know if it’s fixed?
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
![Igor Rodionov avatar](https://secure.gravatar.com/avatar/bc70834d32ed4517568a1feb0b9be7e2.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
and cut new release 1.5.0