SweetOps #pr-reviews for May, 2023

Pull Request Reviews for Cloud Posse Projects

2023-05-12

Sherif Ayad

Hello there .. Can I please get a review on https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/57? .. It’s kinda simple and urgent at the same time for us at the moment .. Maybe also I could need your help understanding why the workflow is failing

#57 fix: propagated `preferred_maintenance_window` to the docdb cluster instances resources

propagated preferred_maintenance_window to the docdb cluster instances resources

what

• The maintenance window given as a variable is given to the Terraform Resource aws_docdb_cluster_instance.default such that the cluster and its instances have the same value given by the user • Closes #55

why

• Without that the cluster instances were having “random” windows that don’t match the expectations of “potential” down time • Users want to be in control of when their databases can go under maintenance not just at any “random” time

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/docdb_cluster_instance#preferred_maintenance_window

Linda Pham (Cloud Posse)

08:41:17 PM

@Sherif Ayad PR updated with comments!

#57 fix: propagated `preferred_maintenance_window` to the docdb cluster instances resources

propagated preferred_maintenance_window to the docdb cluster instances resources

what

why

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/docdb_cluster_instance#preferred_maintenance_window

Sherif Ayad

09:55:05 AM

I adjusted the PR as per the comments of the reviewers

2023-05-15

2023-05-16

jose.amengual

09:59:47 PM

https://github.com/cloudposse/terraform-aws-alb/pull/125 can we merge this to fix the access log bucket?

#125 chore(deps): update terraform cloudposse/lb-s3-bucket/aws to v0.16.2

Mend Renovate

This PR contains the following updates:

Configuration

Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

Ignore: Close this PR and you won’t be reminded about this update again.

☐ If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

Andriy Knysh (Cloud Posse)

10:32:44 PM

i closed the PR since the latest version of the module is https://github.com/cloudposse/terraform-aws-lb-s3-bucket/releases/tag/0.17.0

#125 chore(deps): update terraform cloudposse/lb-s3-bucket/aws to v0.16.2

Mend Renovate

This PR contains the following updates:

Configuration

Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

Ignore: Close this PR and you won’t be reminded about this update again.

☐ If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

jose.amengual

10:33:09 PM

ok, is renovate going to pick the new one?

Andriy Knysh (Cloud Posse)

10:45:59 PM

it should

2023-05-17

Rin Shoys

08:59:52 PM

https://github.com/cloudposse/docs/pull/584 Just updating the documentation, making it look more professional, merge at your convenience

#584 fixed typos in docs

what

• Fixed typos in documentation, put dots where they need to be.

why

• Improves readability and removes distraction.

references

• Also I am getting familiar with the eco-system and the process of contributing itself.

Erik Osterman (Cloud Posse)

10:51:11 PM

Thanks a lot!

#584 fixed typos in docs

what

• Fixed typos in documentation, put dots where they need to be.

why

• Improves readability and removes distraction.

references

• Also I am getting familiar with the eco-system and the process of contributing itself.

2023-05-21

kevcube

06:56:12 AM

https://github.com/cloudposse/terraform-aws-documentdb-cluster/pull/61

#61 Enable intra-security group traffic on DB port

what

• adds variable allow_ingress_from_self which configures the security group to allow traffic within itself on DB port

why

• This is useful in architectures where the db security group will be used to control db access - i.e. it will also be applied to applications.

references

cloudposse/terraform-aws-rds-cluster#145

Linda Pham (Cloud Posse)

11:50:39 PM

@kevcube updated with comments

#61 Enable intra-security group traffic on DB port

what

• adds variable allow_ingress_from_self which configures the security group to allow traffic within itself on DB port

why

• This is useful in architectures where the db security group will be used to control db access - i.e. it will also be applied to applications.

references

cloudposse/terraform-aws-rds-cluster#145

kevcube

03:21:09 PM

thanks @Linda Pham (Cloud Posse); addressed

Linda Pham (Cloud Posse)

06:09:34 PM

merged!

2023-05-23

2023-05-24

Veronika Gnilitska

03:49:11 PM

Hey! Will appreciate a review on a PR adding some improvements to lambda component: https://github.com/cloudposse/terraform-aws-components/pull/692

#692 feat: allows to use YAML instead of JSON for IAM policy

what

• Add default value for the variable function_name. • Replace policy JSON with YAML support to make custom policies more readable.

why

• function_name was required to set, but it wasn’t actually passed to module "lambda" inputs. • YAML makes the policy declaration in stack more readable and compact compared to JSON.

references

• N/A

Linda Pham (Cloud Posse)

04:20:03 PM

merged

#692 feat: allows to use YAML instead of JSON for IAM policy

what

• Add default value for the variable function_name. • Replace policy JSON with YAML support to make custom policies more readable.

why

• function_name was required to set, but it wasn’t actually passed to module "lambda" inputs. • YAML makes the policy declaration in stack more readable and compact compared to JSON.

references

• N/A

Veronika Gnilitska

05:40:12 PM

Hey @Linda Pham (Cloud Posse), there is a change requested in this PR, and I’ll address that shortly.

Erik Osterman (Cloud Posse)

04:33:59 PM

Added my 2c this morning. I’m okay with this change.

Erik Osterman (Cloud Posse)

04:34:03 PM

@Dan Miller (Cloud Posse) any concerns?

Dan Miller (Cloud Posse)

04:37:26 PM

I think as long as we support var.policy_json option as well, this should be fine. But @Jeremy G (Cloud Posse) had requested changes

Linda Pham (Cloud Posse)

09:24:46 PM

@Veronika Gnilitska, Jeremy requested changes

Jeremy G (Cloud Posse)

09:46:28 PM

@Erik Osterman (Cloud Posse) I am opposed to creating a third parallel syntax (YAML) for IAM policies, but if we are going to do it, I would prefer that we standardize on using terraform-aws-iam-policy and make it full-featured, so that everywhere we allow it, it has the same interface, and we do not need to invest energy reinventing it.

Erik Osterman (Cloud Posse)

10:03:59 PM

@Veronika Gnilitska @Matt Gowie thoughts

Veronika Gnilitska

04:58:22 PM

Thank you for review, using terraform-aws-iam-policy is reasonable indeed, I’ll update the PR.

Matt Gowie

06:27:18 PM

Yeah, we discussed. Veronika has something in the works that I think will be a good add, so let’s review when she’s got that wrapped Thanks for the conversation and direction @Jeremy G (Cloud Posse) @Erik Osterman (Cloud Posse) @Dan Miller (Cloud Posse)

Veronika Gnilitska

10:05:01 AM

@Jeremy G (Cloud Posse) @Erik Osterman (Cloud Posse) @Dan Miller (Cloud Posse) the PR was updated to utilize terraform-aws-iam-policy. Please review when possible, thanks in advance!

Erik Osterman (Cloud Posse)

12:01:25 PM

@Linda Pham (Cloud Posse) @Gabriela Campana (Cloud Posse)

Jeremy G (Cloud Posse)

09:33:32 PM

@Erik Osterman (Cloud Posse) I would like to have terraform-aws-iam-policy and then this component updated to take a Terraform object with optional attributes rather than type = any. I can do it, or we can ask Veronika to do it, or we can just proceed as-is. What would you like us to do?

Veronika Gnilitska

01:08:43 PM

I can totally do that if that’s desired to have at this point

Jeremy G (Cloud Posse)

08:30:55 PM

Yes, please. Thank you @Veronika Gnilitska

Veronika Gnilitska

02:54:40 PM

@Jeremy G (Cloud Posse) the PR is up https://github.com/cloudposse/terraform-aws-iam-policy/pull/26 UPD: the tests are failing, I’ll work on that.

#26 feat: define iam_policy_statements object syntax

what

• This updates iam_policy_statements variable definition to take Terraform object with optional attributes rather than type = any. Provider version is bumped sine optional, as a non-experimental feature, was introduced in 1.3.

why

• Provide more control over the expected structure of the input data for IAM policy statements.

references

• Slack thread + PR #692.

Jeremy G (Cloud Posse)

09:42:48 PM

@Veronika Gnilitska https://github.com/cloudposse/terraform-aws-iam-policy/pull/26/files#r1223574353

Veronika Gnilitska

01:15:30 PM

@Jeremy G (Cloud Posse) I’ve updated the PR #26 and left some comments

Jeremy G (Cloud Posse)

08:59:10 PM

@Veronika Gnilitska Thanks for pointing out that iam_policy_statements is a map and we want to preserve backward compatibility.

I updated your PR to introduce a new iam_policy input that more closely parallels aws_iam_policy_document and ask that you use that in the component that started this whole thread. I also made a bunch of other cleanups. Please let me know if you have any suggestions, objections, concerns, etc. about the changes I made. CC @Matt Gowie

Veronika Gnilitska

10:56:10 AM

@Jeremy G (Cloud Posse) looks great, no additional comments from my side. Thank you! I’ll proceed with the PR to the components once a new module version is released

Jeremy G (Cloud Posse)

12:06:01 AM

@Veronika Gnilitska iam-policy v1.0.0 released . Please use iam_policy input, not iam_policy_statements. Thank you again for your help.

Veronika Gnilitska

08:46:58 AM

Awesome, will do. Thanks!

Matt Gowie

03:42:07 PM

Thanks for working through that folks – That 1.0 is a great cut and I know we’ll be happy to start using that module more now

Veronika Gnilitska

11:03:11 AM

@Jeremy G (Cloud Posse) hey! I don’t see the latest version 1.0.0 in the registry https://registry.terraform.io/modules/cloudposse/iam-policy/aws/latest. Is there anything else that should be done to make it available?

Erik Osterman (Cloud Posse)

04:37:09 PM

I manually resync’d it

Erik Osterman (Cloud Posse)

04:37:19 PM

we have this problem with some of our modules. some bug in the TF registry.

Veronika Gnilitska

05:02:38 PM

Good to know. Thanks @Erik Osterman (Cloud Posse)!

Veronika Gnilitska

10:54:19 AM

@Jeremy G (Cloud Posse) it seems like the ability to use var.policy_json only was lost with the recently added validation blocks. I put up a PR to fix this https://github.com/cloudposse/terraform-aws-iam-policy/pull/28, please take a look when possible.

#28 fix: support JSON inputs for policy document when IAM policy/statements are not set as Terraform objects

what

• Consider JSON source policy documents in the recently added precondition block.

why

• It’s expected to support JSON inputs while var.iam_policy and var.iam_policy_statments may remain unset. The precondition fails in this case:

│ Error: Resource precondition failed
│ 
│   on .terraform/modules/iam_policy/main.tf line 90, in data "aws_iam_policy_document" "this":
│   90:       condition     = var.iam_policy_statements != null || var.iam_policy != null
│     ├────────────────
│     │ var.iam_policy is null
│     │ var.iam_policy_statements is null
│ 
│ Exactly 1 of var.iam_policy and var.iam_policy_statments may be used, preferably var.iam_policy.

references

• N/A

Jeremy G (Cloud Posse)

06:54:42 PM

@Veronika Gnilitska Good catch, and thank you for the PR. Let’s just remove the failing precondition entirely, because there are several inputs that could possibly be sufficient, and AWS will complain if nothing is provided, so we can rely on that.

Veronika Gnilitska

09:34:26 AM

@Jeremy G (Cloud Posse) that makes sense - PR was updated.

Jeremy G (Cloud Posse)

09:18:49 PM

Thank you, @Veronika Gnilitska. iam-policy v1.0.1 is now available.

Veronika Gnilitska

08:13:06 AM

PR #692 was updated to utilize the latest version of terraform-aws-iam-policy. Not sure what’s the issue with bats check though

#692 feat: allows to use YAML instead of JSON for IAM policy

Erik Osterman (Cloud Posse)

08:28:26 PM