#pr-reviews (2024-02)

Pull Request Reviews for Cloud Posse Projects

2024-02-05

Juan Martinez avatar
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

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

@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 avatar
Juan Martinez

Done. Thanks for the review.

2024-02-06

islam heggy avatar
islam heggy

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

islam heggy avatar
islam heggy

@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) avatar
Andriy Knysh (Cloud Posse)

@islam heggy thanks for the PR, please see comments

islam heggy avatar
islam heggy

@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

islam heggy avatar
islam heggy

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

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

thank you for your contributions

2024-02-07

2024-02-08

2024-02-11

mike186 avatar
mike186

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

Replaced with PRs 917 and 918

1

2024-02-16

2024-02-19

2024-02-23

    keyboard_arrow_up