#pr-reviews (2020-05)
Pull Request Reviews for Cloud Posse Projects
2020-05-04
please review : https://github.com/cloudposse/terraform-aws-rds-cluster/pull/65
please resolve the conflicts “This branch has conflicts that must be resolved”
done
any takers?
@Andriy Knysh (Cloud Posse)
looking @jose.amengual
thanks
@jose.amengual
AWESOME!!!!
thanks
I introduced the bug
you break it, you fix it , thanks
for some reason, Tf does not complain about wrong dynamic
and we have tests which never showed any errors
I was kinda surprised when I say it and since it did not complained I thought it was my problem and then I went back and saw other examples and then it clicked
2020-05-05
2020-05-07
No rush on these but want to surface them just to clean up my GH PR list :slightly_smiling_face:
- ssm-parameter-store: Upgrades to work with Terraform v0.12.20
- test-harness: Fix terraform validate test
- build-harness: Fixes #207, supplying AWS_DEFAULT_REGION fixes v0.12
tf validate - build-harness: Fixes #149, tf/lint on mac xargs does not support –no-run-if-empty
- Docs: Fixes up some small mistakes / formatting Thanks folks!
ya - really sorry about the delays @Matt Gowie
I know it’s on @Maxim Mironenko (Cloud Posse) and @Andriy Knysh (Cloud Posse)’s radar
the SSM module fixes are wanted by a few folks
No worries! Just throwing it out there. Learned some good stuff working through a couple of those, so regardless if they ever ship or not I’m all good
looks good, just could not trigger the Codefresh test pipeline from the fork for the first time (prob Codefresh issue)
need to copy your PR and put into our repo
I’m waiting for that PR to get merged to push mine to add the S3 logging, KMS encryptions stuff
2020-05-08
I’m getting old definitely @Matt Gowie, all this time talking about adding s3 and kms to your ssm module PR and I just realized this is the read module……
What I was talking about is that SSM can be setup in different ways to use it in conjunction with session manager, which does not have any implications to Parameter Store but I was getting confused because the KMS key we use for session manager is the same one we use for parameter store……this must be an old concussion, early alzheimer’s or something
Haha @jose.amengual I was wondering what you were referring to with that. No worries though — I’ve done that many times too .
That module does do read and write. Just different vars handle what you’re trying to do.
2020-05-11
https://github.com/cloudposse/terraform-aws-s3-bucket/pull/24 Another PR for this
I fixed the conflicts
it should be good for review, although I see a few PRs waiting
please @Maxim Mironenko (Cloud Posse) or @Andriy Knysh (Cloud Posse)
@jose.amengual please see comments on your PR
changed, it should be good now
still not perfect (please see comments)
I see what you mean now, no reason to check for null, just the bool
I commited your changes
@jose.amengual I commented again, please review
ohhhhh right yes you need to have two….
I was trying to simplify so you can have the option to disable both
ok, I will do it now
You have separate flags so you can disable both if needed
exactly
I pushed the changes
thanks @jose.amengual
thank you
2020-05-12
2020-05-13
Any takers 
2020-05-14
@jose.amengual we’ll tackle more PR reviews next week with help from @Marcin Brański
@Marcin Brański has joined the channel
2020-05-18
Good evening, Could I get some eyes on https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pull/21 whenever someone has a few minutes? Thx!
@Marcin Brański @Andriy Knysh (Cloud Posse)
@Adam Crews ^
Thank you!
Also this one whenever convenient, https://github.com/cloudposse/terraform-aws-eks-workers/pull/37 Thank you!
2020-05-19
@Andrew Mackett has joined the channel
2020-05-21
@Sumeet Shukla has joined the channel
@Sumeet Shukla the PR has been approved, thanks
@Andriy Knysh (Cloud Posse) Thank You
Can someone please have a look at this PR?
@Erik Osterman (Cloud Posse) @Andriy Knysh (Cloud Posse) if you guys have some time : https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/33
@jose.amengual thanks. There are some issues with the github provider (and we saw them in other modules as well). The tests are failing, we need to take a look at this
that is the same problem I have
basically the provider gets initialize no matter if you set the module enabled or not
so by passing the anonymous it will not complain about not having credentials
is like a chicken and the egg problem
I don’t have time right now to look into that. @Maxim Mironenko (Cloud Posse) might look if he has time
Might have some time near middle of next week
I’m very persistent :)
mm since the test use the complete example, could it be related to this : https://github.com/cloudposse/terraform-aws-ecs-codepipeline/blob/master/examples/complete/fixtures.us-east-2.tfvars#L81 ?
passing that test user and token?
maybe is not valid anymore
do you know what was the change to the github provider so it started throwing those errors?
provider as the terraform provider or you mean this : https://github.com/cloudposse/terraform-github-repository-webhooks/pull/15/files
Do you guys think you will have time this week ? @Andriy Knysh (Cloud Posse) @Maxim Mironenko (Cloud Posse)
silence…..
@Andriy Knysh (Cloud Posse) have a chance tomorrow morning maybe?
I told you I was going to remind you
2020-05-22
Hi all. It would be great if anybody has the time to look at this small PR: https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/69
Thanks!
thanks @Andrew Mackett
Thank you!
2020-05-26
Hi! It would be great if someone took a look https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/34
2020-05-27
https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/34
Hey guys, I added bitbucket (codestar) support in this PR, but I understand if you don’t want to accept it and only support GitHub! :D
2020-05-28
@Aziz has joined the channel