#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 (Cloud Posse) what do you think?

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

    keyboard_arrow_up