SweetOps #pr-reviews for February, 2024

2024-02-05

Juan Martinez

Is there any chance I could get a review for this PR for terraform-aws-waf? Am i supposed to request here after creating a PR? https://github.com/cloudposse/terraform-aws-waf/pull/59

#59 feat: adds `geo_allowlist_statement_rules` action

what

This adds an action parameter to geo_allowlist_statement_rules objects, replacing the currently-default block action.

why

This is useful for metrics collection on the geo allowlist statements

To allow the user to choose either of the following actions

• block • count

references

closes #58

1

Andriy Knysh (Cloud Posse)

02:17:37 AM

@Juan Martinez the PR looks good, thanks. Please run the commands described in the comment

#59 feat: adds `geo_allowlist_statement_rules` action

what

This adds an action parameter to geo_allowlist_statement_rules objects, replacing the currently-default block action.

why

This is useful for metrics collection on the geo allowlist statements

To allow the user to choose either of the following actions

• block • count

references

closes #58

Juan Martinez

02:41:53 AM

Done. Thanks for the review.

Andriy Knysh (Cloud Posse)

03:05:24 AM

https://github.com/cloudposse/terraform-aws-waf/releases/tag/1.4.0, thanks

2024-02-06

islam heggy

06:02:05 PM

Hello Team,

Can I get a review for my PR here that adds the functionality for global aws config aggregator. Appreciate your help!

#85 Add support for organization aggregator

what

• Extended the module functionality to include organization wide aggregator • Add the ability to create/pass new IAM role for the organization aggregator • Handled default IAM role cases vs organization aggregator IAM role. So they don’t depend on each other

why

• The current default way is attaching accounts using account ids and there is no way to use organization wide aggregator and it’s really hard to maintain large number of accounts when using organizations.

references

• I used organization aggregation argument of the aws_config_configuration_aggregator provider to add the functionality. • I checked this stale PR and decided to reinvent the wheel as it has been a while since it was opened

1

islam heggy

12:26:33 AM

@Andriy Knysh (Cloud Posse) Could you please assist here?

#85 Add support for organization aggregator

what

• Extended the module functionality to include organization wide aggregator • Add the ability to create/pass new IAM role for the organization aggregator • Handled default IAM role cases vs organization aggregator IAM role. So they don’t depend on each other

why

• The current default way is attaching accounts using account ids and there is no way to use organization wide aggregator and it’s really hard to maintain large number of accounts when using organizations.

references

• I used organization aggregation argument of the aws_config_configuration_aggregator provider to add the functionality. • I checked this stale PR and decided to reinvent the wheel as it has been a while since it was opened

Andriy Knysh (Cloud Posse)

03:18:02 AM

@islam heggy thanks for the PR, please see comments

islam heggy

04:23:41 AM

@Andriy Knysh (Cloud Posse) fixed them and we should be good to go!

Thanks for the review and Let me know if anything else needs fixing

Andriy Knysh (Cloud Posse)

08:57:10 PM

https://github.com/cloudposse/terraform-aws-config/releases/tag/1.4.0 thanks @islam heggy

1

islam heggy

08:01:21 PM

Thanks for the support and review @Andriy Knysh (Cloud Posse)

Andriy Knysh (Cloud Posse)

08:05:16 PM

thank you for your contributions

2024-02-07

2024-02-08

2024-02-11

mike186

10:38:41 AM

PRs https://github.com/cloudposse/geodesic/pull/915 and https://github.com/cloudposse/geodesic/pull/916 are a 1-2 sequence related set, submitted for review #geodesic related

• First one ◦ addresses a variety of bugs related to https://github.com/cloudposse/geodesic/issues/899 and ◦ fixes some Makefile and/or arm64 related build problems ◦ introduces docker OCI labeling (some fields used only by Makefile, gha drafted but not tested) and

• the second one (includes the first; edits need to propagate; second builds on the first) ◦ moves Geodesic debian from bullseye to bookworm, ◦ with the minor tweaks required, ◦ removing 9 critical vulnerabilities, ◦ removes 208 high rated vulnerabilities and more. Review, haze, cherry pick or as you wish. Cheers!

1

Jeremy G (Cloud Posse)

11:54:39 PM

Replaced with PRs 917 and 918

1

2024-02-16

2024-02-19

2024-02-23

2024-02-26

Hans D

09:22:49 AM

https://github.com/cloudposse/terraform-aws-components/pull/992

#992 remove unused vars in `aws-backup` component to reflect the module upgrade

what

After the AWS Backup module https://github.com/cloudposse/terraform-aws-backup/ 1.0 release, there’s a couple of unused variables and unused logic, and some information that should be updated in the README.

• The variables schedule, start_window, completion_window, cold_storage_after, delet_after, and destination_vault_arn, etc. were removed as of the module’s v0.14 and placed in deprecation (see PR https://github.com/cloudposse/terraform-aws-backup/pull/39/files#diff-0c2612bc0fbe3bc116da52a6aece40330dc93c8cbc640dc96427bf940f08f7c3) then finally removed entirely when the v1.0 release (see this PR by @Benbentwo : cloudposse/terraform-aws-backup#63) • They were moved to the rules object to support multiple plans, so getting rid of them in this component since they’re not used anywhere anymore. • Because the destination_vault and related variables aren’t used on the top level (moved to the rule object like the others), the [remote-state.tf](http://remote-state.tf) file and the locals block that dynamically determines if the copy action is enabled is not used anymore either here.

Also updated the README to reflect these changes along with the new method for cross region copying.

why image

Unused variables taking up space & tflint complaining

Also an existing issue for this: #977

references

See links above for each point.

1

#pr-reviews (2024-02)

2024-02-05

2024-02-06

2024-02-07

2024-02-08

2024-02-11

2024-02-16

2024-02-19

2024-02-23

2024-02-26

2024-02-27