SweetOps #pr-reviews for May, 2022

Pull Request Reviews for Cloud Posse Projects

2022-05-03

kevcube

05:42:46 PM

requesting review on this one (not my PR) https://github.com/cloudposse/terraform-aws-rds-cluster/pull/138

it’d be helpful to have

what

• Add aurora serverlessv2 support

why

• AWS releases aurora serverless v2. • Adopt this new feature in this rds cluster module

references

• closes #137 • Terraform serverlessv2_scaling_configuration

Matt Gowie

11:40:56 PM

https://github.com/cloudposse/terraform-aws-iam-system-user/pull/65

what

• Add option to change the SSM base path under which credentials are stored • Store ses_smtp_password_v4 in SSM

why

• Even if the IAM users are global per account often times there is a need to separeate them per environment (dev, staging). By adding the ssm_base_path parameter we can have a better SSM hierarchy. Example: /dev/system_user/tc-dev-s3 and /staging/system_user/tc-staging-ses • Also having the ses_smtp_password_v4 stored in SSM will allow better integration with CI systems when deploying applications (rather than having a script to generate the password or read the terraform state)

references

• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow). • closes #60 • Copy of #61, Closes #61

Matt Gowie

04:52:04 PM

@RB check this comment when you get the chance. https://github.com/cloudposse/terraform-aws-iam-system-user/pull/65#issuecomment-1116769627

@nitrocode have you seen those auto-readme conflicts before? Not sure what is up there… I believe I want to accept all the incoming changes, but wanted to get another set of eyes before I undo some work if I’m wrong.

04:57:25 PM

@Matt Gowie i saw that the other day and forgot to respond. ive never seen it before but my guess is that maybe you branched from an earlier default branch without pulling? or perhaps the auto format workflow was updated while your pr was open which lead to the conflict

04:57:45 PM

in any case, you could merge from the default branch, resolve the conflict, and it should be good to go

Matt Gowie

05:02:21 PM

Ah pulling in master from my local didn’t cause a conflict — Good call.

05:07:14 PM

it would be nice if the auto format would first merge in the default branch before running pre commit. then run pre commit, then commit if needed.

05:07:34 PM

but maybe that would be more of a headache

05:08:06 PM

or perhaps pre commit should only run it the pr branch is in line with the HEAD default branch

05:08:18 PM

cc: @Jeremy G (Cloud Posse) what do you think?

Jeremy G (Cloud Posse)

05:46:24 PM

I don’t know what the problem was, because the PR looks all good to me at this point. If a PR is in conflict with the default branch then the default branch should be merged in and the conflict resolved by the PR author. Conflicts in generated files like README.md can be resolved almost any way and then auto-format will fix them after the commit. Sometimes I just use the web editor and delete the entire contents of the file.

Jeremy G (Cloud Posse)

05:47:57 PM

@RB I do not think it is a good idea for automation to try to automatically update a PR that is in conflict; that should have human supervision.

05:49:24 PM

sorry that wasnt my suggestion. the issue seems to have come up cause the branch was branched off of an old commit, precommit kicked off on the old commit and updated it correctly.

my suggestion is to only allow precommit to run if the branch is in line with the default branch. that way precommit cannot cause a git conflict.

Jeremy G (Cloud Posse)

06:26:35 PM

It’s appropriate for pre-commit to cause a git conflict. That is something that needs human resolution before the PR is ready to approve and merge.

2022-05-04

2022-05-12

azec

01:43:19 AM

I was watching recording of this week’s demo (I missed live) and got interested about atmos. Reading through https://github.com/cloudposse/atmos#centralized-project-configuration , I realized that refs to the config files in the example dir structure are little bit outdated. Do these changes require GH Issue to be opened or PR directly is ok?

Andy Miguel

02:17:38 AM

Cc @Andriy Knysh (Cloud Posse)

Andriy Knysh (Cloud Posse)

02:17:43 AM

the README is outdated

Andriy Knysh (Cloud Posse)

02:17:53 AM

we are working on a doc website for atmos

Andriy Knysh (Cloud Posse)

02:18:29 AM

this is a working example https://github.com/cloudposse/atmos/tree/master/examples/complete

azec

02:20:44 AM

Got it, thanks!

azec

02:21:10 AM

Just realized there is atmos for future questions like this.

2022-05-25

Chris Dobbyn

04:07:21 AM

I don’t have a PR yet, but I’m hoping I could gather some thoughts/feedback before I go too far. I was looking at the module aws-vpc-peering. The problem I would like to try to solve is where you have multiple associated_cidr_blocks to a vpc but you only want to route a subset of them. I have written code that will do this via a for_each, but adding this logic into the count and index magic that’s happening is a little painful.

Would you:

• Fork at this point to preserve backwards compatibility

• Create an option that determines which is used

• Completely replace the count with a for_each

• Bite the bullet, accept the count / index stuff and just make it work with what’s there My personal issue with the count is when things change multiple routes need to update (because indexes might also change). Using a for_each is safer because you can create a key that identifies the elements involved and prevent the mass re-creation of objects.