#pr-reviews (2024-04)

Pull Request Reviews for Cloud Posse Projects

2024-04-02

Chris Dobbyn avatar
Chris Dobbyn

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) avatar
Erik Osterman (Cloud Posse)

@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) avatar
Jeremy G (Cloud Posse)

@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) avatar
Erik Osterman (Cloud Posse)

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

RB avatar

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

1
1

2024-04-11

Simon Heather avatar
Simon Heather

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

1
Simon Heather avatar
Simon Heather

@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

1
jose.amengual avatar
jose.amengual

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

Simon Heather avatar
Simon Heather

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

jose.amengual avatar
jose.amengual

the test must pass for us to be able to merge

Simon Heather avatar
Simon Heather

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

jose.amengual avatar
jose.amengual

I do not know how long they are retained

2024-04-12

Simon Heather avatar
Simon Heather
#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

1
Simon Heather avatar
Simon Heather

And run the tests?

#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 avatar
Simon Heather

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

1
    keyboard_arrow_up