Pull Request Reviews for Cloud Posse Projects
requesting review on this one (not my PR) https://github.com/cloudposse/terraform-aws-rds-cluster/pull/138
it’d be helpful to have
• Add option to change the SSM base path under which credentials are stored • Store ses_smtp_password_v4 in SSM
• 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:
• 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)
@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.
@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
in any case, you could merge from the default branch, resolve the conflict, and it should be good to go
Ah pulling in master from my local didn’t cause a conflict — Good call.
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.
but maybe that would be more of a headache
or perhaps pre commit should only run it the pr branch is in line with the HEAD default branch
cc: @Jeremy (Cloud Posse) what do you think?
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.
@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.
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.
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.