SweetOps #pr-reviews for April, 2024

Pull Request Reviews for Cloud Posse Projects

2024-04-02

Chris Dobbyn

06:27:13 PM

Simple PR to do some type enforcement on null label: https://github.com/cloudposse/terraform-null-label/pull/158

Ran into too many people using enabled = "false" instead of enabled = false and this will fix that.

#158 fix: enabled var should be enforced

what

• Today var.enabled accepts strings and numbers, as well as bool and null.

why

• When you write enabled = "false" the boolean conversion translates this to true. • This enforces strict typing of bool or null.

references

• n/a

Erik Osterman (Cloud Posse)

06:29:05 PM

@Jeremy G (Cloud Posse) to chime in here

#158 fix: enabled var should be enforced

what

• Today var.enabled accepts strings and numbers, as well as bool and null.

why

• When you write enabled = "false" the boolean conversion translates this to true. • This enforces strict typing of bool or null.

references

• n/a

Jeremy G (Cloud Posse)

07:07:08 PM

@Chris Dobbyn I don’t mean to be harsh, but I don’t know how to say this more politely: AFAIK, the problem you are trying to solve does not exist, and if it did, your PR would not address it. I put more details in my comment closing your PR.

In examples/complete/label5.tf we set enabled = false. You can try different values and see for yourself that enabled = "false" sets enabled to the boolean value false, not true.

  enabled     = false

Erik Osterman (Cloud Posse)

08:48:20 PM

So the key takeaway here is that:

even if enabled = "false" resulted in var.enabled == true neither this validation, nor any validation, would be able to detect and correct that situation, because the validation runs after the conversion takes place.

2024-04-10

07:53:25 PM

Please review https://github.com/cloudposse/terraform-aws-dynamodb/pull/126 to override the dynamodb table name without having to override the context inputs

2024-04-11

Simon Heather

02:32:48 PM

Can someone approve the GitHub workflow on https://github.com/cloudposse/terraform-aws-cloudtrail/pull/70

#70 Add CloudTrail Advanced Event Selector

what

Add support for the CloudTrail advanced event selector

Also bumps the example cloudtrail_s3_bucket module version to fix test errors.

references

• closes #69

Simon Heather

10:50:13 PM

@jose.amengual, can you re-run the tests on this?

#70 Add CloudTrail Advanced Event Selector

what

Add support for the CloudTrail advanced event selector

Also bumps the example cloudtrail_s3_bucket module version to fix test errors.

references

• closes #69

jose.amengual

11:02:23 PM

the test failed again, my guess is that there is more input needed for the updated module for the examples to pass

Simon Heather

11:09:34 PM

I would put money on that these tests would fail on the main branch.

jose.amengual

11:25:10 PM

the test must pass for us to be able to merge

Simon Heather

11:35:10 PM

I can’t see that the tests have ever been run on a merged PR in this repo.

jose.amengual

11:36:37 PM

I do not know how long they are retained

Simon Heather

07:28:57 PM

@jose.amengual, I’ve bumped the cloudtrail-s3-bucket module version on this PR. Can you rerun the tests, which should pass now.

2024-04-12

Simon Heather

10:39:17 AM

Can someone approve the GitHub workflow on https://github.com/cloudposse/terraform-aws-cloudtrail-s3-bucket/pull/91

#91 Fix Race Condition between the Creation of the S3 Bucket Policy and the CloudTrail Trail

what

Fix the race condition between the creation of the S3 Bucket policy and the CloudTrail trail by adding a depends_on argument to the bucket_id output which is used as input to the CloudTrail module. This ensures that all the resources in the CloudTrail S3 Bucket module, including the S3 Bucket Policy have been created before the CloudTrail trail is created.

The example used for the tests has also been updated to include the creation of the CloudTrail Trail to verify that this is working.

why

• Fixes #90

Simon Heather

01:50:18 PM

And run the tests?

#91 Fix Race Condition between the Creation of the S3 Bucket Policy and the CloudTrail Trail

what

The example used for the tests has also been updated to include the creation of the CloudTrail Trail to verify that this is working.

why

• Fixes #90

Simon Heather

04:06:57 PM

@jose.amengual, can you run the tests again? Thanks.

2024-04-15

2024-04-30

kevcube

06:47:49 PM

Hello, just put up some PRs in aws-amplify-app (#32, #33) and realized that all Terratests in this repo are still broken due to Github Token issues. See discussion here about this breakage. Can I request that someone with Cloudposse github management permissions takes a look at this issue? Outside contributor @sestrella has been very patient and helpful during discovery of this github token issue

#15 Fix basic authentication configuration at the branch level

what

Module consumers cannot pass a custom basic_auth_credentials per branch using the existing variables. At the branch level, there is also a typo on enable_basic_auth.

why

The changes made in this PR allow consumers to configure different basic authentication credentials for each branch.

Erik Osterman (Cloud Posse)

03:11:05 AM

We’ll take a look

#15 Fix basic authentication configuration at the branch level

what

Module consumers cannot pass a custom basic_auth_credentials per branch using the existing variables. At the branch level, there is also a typo on enable_basic_auth.

why

The changes made in this PR allow consumers to configure different basic authentication credentials for each branch.