#pr-reviews (2022-05)

Pull Request Reviews for Cloud Posse Projects

2022-05-03

kevcube avatar
kevcube

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 #137Terraform serverlessv2_scaling_configuration

Matt Gowie avatar
Matt Gowie

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 avatar
Matt Gowie

@RB (Ronak) (Cloud Posse) 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.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

@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

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

Matt Gowie avatar
Matt Gowie

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

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

but maybe that would be more of a headache

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

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

@RB (Ronak) (Cloud Posse) 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.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

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

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.

1

2022-05-04

2022-05-12

azec avatar

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 (Cloud Posse) avatar
Andy Miguel (Cloud Posse)

Cc @Andriy Knysh (Cloud Posse)

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

the README is outdated

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

we are working on a doc website for atmos

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

Got it, thanks!

azec avatar

Just realized there is atmos for future questions like this.

2022-05-25

Chris Dobbyn avatar
Chris Dobbyn

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.

2022-05-31

mihai avatar

an older PR of mine to review https://github.com/cloudposse/terraform-aws-rds/pull/136 then we can have v1.0.0 of this module

Closes #137

1
mihai avatar

a gentle ping for your kind support @RB (Ronak) (Cloud Posse)

Closes #137

mihai avatar

pushed

mihai avatar

and another ping on this. many thanks!

mihai avatar

any idea why it failed to find Terraform?

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I pinged our internal contributor channel hoping to get some help

1
mihai avatar

have rebased now

    keyboard_arrow_up