#pr-reviews (2020-05)

Pull Request Reviews for Cloud Posse Projects

2020-05-28

Aziz avatar
Aziz
03:44:27 PM

@Aziz has joined the channel

2020-05-27

Joe Niland avatar
Joe Niland

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

Add bitbucket support by joe-niland · Pull Request #34 · cloudposse/terraform-aws-ecs-codepipeline

I needed to add Bitbucket support for CodePipeline so this is my attempt. I'm not sure if you want to add it since you may only want to support GitHub. For now I also duplicated the CodePipelin…

2020-05-26

Mikhail Naletov avatar
Mikhail Naletov

Hi! It would be great if someone took a look https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/34

UPGRADE: Support for Terraform 0.12 by mattclegg · Pull Request #34 · cloudposse/terraform-aws-cloudfront-cdn

what This is an upgrade to support Terraform 0.12. why Terraform v0.12 is a major release but most of the changes are syntax related. I have imported the changes suggested by @rverma-nikiai &amp…

2020-05-22

Andrew Mackett avatar
Andrew Mackett

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!

1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)
cloudposse/terraform-aws-ecs-container-definition

Terraform module to generate well-formed JSON documents (container definitions) that are passed to the aws_ecs_task_definition Terraform resource - cloudposse/terraform-aws-ecs-container-definition

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

thanks @Andrew Mackett

Andrew Mackett avatar
Andrew Mackett

Thank you!

2020-05-21

Sumeet Shukla avatar
Sumeet Shukla
07:47:39 PM

@ has joined the channel

Sumeet Shukla avatar
Sumeet Shukla
fix(#63): instance_count to be independent of autoscaling_min_capacity by sumeetshk · Pull Request #67 · cloudposse/terraform-aws-rds-cluster

Fix for #63 The cluster_size or the instance_count should be independent of autoscaling_min_capacity as autoscaling_min_capacity is automatically taken care of by AWS through the autoscaling policy…

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

@ the PR has been approved, thanks

fix(#63): instance_count to be independent of autoscaling_min_capacity by sumeetshk · Pull Request #67 · cloudposse/terraform-aws-rds-cluster

Fix for #63 The cluster_size or the instance_count should be independent of autoscaling_min_capacity as autoscaling_min_capacity is automatically taken care of by AWS through the autoscaling policy…

Sumeet Shukla avatar
Sumeet Shukla

@Andriy Knysh (Cloud Posse) Thank You

Sumeet Shukla avatar
Sumeet Shukla

Can someone please have a look at this PR?

jose.amengual avatar
jose.amengual

@Erik Osterman (Cloud Posse) @Andriy Knysh (Cloud Posse) if you guys have some time : https://github.com/cloudposse/terraform-aws-ecs-codepipeline/pull/33

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

@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

jose.amengual avatar
jose.amengual

that is the same problem I have

jose.amengual avatar
jose.amengual

basically the provider gets initialize no matter if you set the module enabled or not

jose.amengual avatar
jose.amengual

so by passing the anonymous it will not complain about not having credentials

jose.amengual avatar
jose.amengual

is like a chicken and the egg problem

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

I don’t have time right now to look into that. @Maxim Mironenko (Cloud Posse) might look if he has time

Maxim Mironenko (Cloud Posse) avatar
Maxim Mironenko (Cloud Posse)

Might have some time near middle of next week

jose.amengual avatar
jose.amengual

I will remind you guys next week

1
jose.amengual avatar
jose.amengual

I’m very persistent :)

jose.amengual avatar
jose.amengual

if you haven’t noticed

1
1
jose.amengual avatar
jose.amengual
cloudposse/terraform-aws-ecs-codepipeline

Terraform Module for CI/CD with AWS Code Pipeline and Code Build for ECS https://cloudposse.com/ - cloudposse/terraform-aws-ecs-codepipeline

jose.amengual avatar
jose.amengual

passing that test user and token?

jose.amengual avatar
jose.amengual

maybe is not valid anymore

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

do you know what was the change to the github provider so it started throwing those errors?

jose.amengual avatar
jose.amengual

provider as the terraform provider or you mean this : https://github.com/cloudposse/terraform-github-repository-webhooks/pull/15/files

1
jose.amengual avatar
jose.amengual

Do you guys think you will have time this week ? @Andriy Knysh (Cloud Posse) @Maxim Mironenko (Cloud Posse)

jose.amengual avatar
jose.amengual

silence…..

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Andriy Knysh (Cloud Posse) have a chance tomorrow morning maybe?

jose.amengual avatar
jose.amengual

I told you I was going to remind you

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

2020-05-19

Andrew Mackett avatar
Andrew Mackett
02:23:41 PM

@Andrew Mackett has joined the channel

2020-05-18

Adam Crews avatar
Adam Crews

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!

Adds environment label by tthayer · Pull Request #21 · cloudposse/terraform-aws-ec2-autoscale-group

what Adds environment label why Required for tests to pass for cloudposse/terraform-aws-eks-workers#33 references cloudposse/terraform-aws-eks-workers#33

1
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Marcin Brański @Andriy Knysh (Cloud Posse)

Adds environment label by tthayer · Pull Request #21 · cloudposse/terraform-aws-ec2-autoscale-group

what Adds environment label why Required for tests to pass for cloudposse/terraform-aws-eks-workers#33 references cloudposse/terraform-aws-eks-workers#33

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)
cloudposse/terraform-aws-ec2-autoscale-group

Terraform module to provision Auto Scaling Group and Launch Template on AWS - cloudposse/terraform-aws-ec2-autoscale-group

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

@Adam Crews ^

Adam Crews avatar
Adam Crews

Thank you!

Adam Crews avatar
Adam Crews

Also this one whenever convenient, https://github.com/cloudposse/terraform-aws-eks-workers/pull/37 Thank you!

2020-05-14

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@jose.amengual we’ll tackle more PR reviews next week with help from @Marcin Brański

1
1
Marcin Brański avatar
Marcin Brański
12:44:36 AM

@Marcin Brański has joined the channel

2020-05-13

jose.amengual avatar
jose.amengual

Any takers

2020-05-12

2020-05-11

jose.amengual avatar
jose.amengual
1
jose.amengual avatar
jose.amengual

I fixed the conflicts

jose.amengual avatar
jose.amengual

it should be good for review, although I see a few PRs waiting

jose.amengual avatar
jose.amengual

please @Maxim Mironenko (Cloud Posse) or @Andriy Knysh (Cloud Posse)

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

@jose.amengual please see comments on your PR

jose.amengual avatar
jose.amengual

changed, it should be good now

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

still not perfect (please see comments)

jose.amengual avatar
jose.amengual

I see what you mean now, no reason to check for null, just the bool

jose.amengual avatar
jose.amengual

I commited your changes

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

@jose.amengual I commented again, please review

jose.amengual avatar
jose.amengual

ohhhhh right yes you need to have two….

jose.amengual avatar
jose.amengual

I was trying to simplify so you can have the option to disable both

jose.amengual avatar
jose.amengual

ok, I will do it now

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

You have separate flags so you can disable both if needed

jose.amengual avatar
jose.amengual

exactly

jose.amengual avatar
jose.amengual

I pushed the changes

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)
cloudposse/terraform-aws-s3-bucket

Terraform module that creates an S3 bucket with an optional IAM user for external CI/CD systems - cloudposse/terraform-aws-s3-bucket

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

thanks @jose.amengual

jose.amengual avatar
jose.amengual

thank you

2020-05-08

jose.amengual avatar
jose.amengual

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……

jose.amengual avatar
jose.amengual

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

Matt Gowie avatar
Matt Gowie

Haha @jose.amengual I was wondering what you were referring to with that. No worries though — I’ve done that many times too .

Matt Gowie avatar
Matt Gowie

That module does do read and write. Just different vars handle what you’re trying to do.

2020-05-07

Matt Gowie avatar
Matt Gowie

No rush on these but want to surface them just to clean up my GH PR list :slightly_smiling_face:

  1. ssm-parameter-store: Upgrades to work with Terraform v0.12.20
  2. test-harness: Fix terraform validate test
  3. build-harness: Fixes #207, supplying AWS_DEFAULT_REGION fixes v0.12 tf validate
  4. build-harness: Fixes #149, tf/lint on mac xargs does not support –no-run-if-empty
  5. Docs: Fixes up some small mistakes / formatting Thanks folks!
Upgrades to work with Terraform v0.12.20 by Gowiem · Pull Request #12 · cloudposse/terraform-aws-ssm-parameter-store

Info Following up on #10, this PR uses that fork and then applies the most recent requirement: Updates the type for parameter_write variable so terraform can run without errors.

Fix terraform validate test by Gowiem · Pull Request #11 · cloudposse/test-harness

what Adds a "Prerequisites" header to README to mention Bash v5+ and bats-core requirements. These threw me for a while, so I figured adding them here would help others when running loca…

Fixes #207, supplying AWS_DEFAULT_REGION fixes v0.12 `tf validate` by Gowiem · Pull Request #208 · cloudposse/build-harness

what Updates the terraform/validate target to actually run terraform validate instead of logging "unsupported" message. why This is to provide actual validation on v0.12 Seems the work…

Fixes #149, tf/lint on mac xargs does not support --no-run-if-empty by Gowiem · Pull Request #206 · cloudposse/build-harness

what Fixes make terraform/lint on Mac OSX which was failing. why The terrform/lint target uses the –no-run-if-empty flag, which is not available on the default Mac/BSD xargs, but is available o…

Fixes up some small mistakes / formatting by Gowiem · Pull Request #481 · cloudposse/docs

Info Was reading through this Doc cause I don’t much about Make and figured I’d fix up these small things. Thanks for the intro on Make folks!

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

ya - really sorry about the delays @Matt Gowie

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

I know it’s on @Maxim Mironenko (Cloud Posse) and @Andriy Knysh (Cloud Posse)’s radar

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

the SSM module fixes are wanted by a few folks

Matt Gowie avatar
Matt Gowie

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

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

I’m working on terraform-aws-ssm-parameter-store

2
1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

looks good, just could not trigger the Codefresh test pipeline from the fork for the first time (prob Codefresh issue)

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

need to copy your PR and put into our repo

Matt Gowie avatar
Matt Gowie

jose.amengual avatar
jose.amengual

I’m waiting for that PR to get merged to push mine to add the S3 logging, KMS encryptions stuff

2020-05-05

2020-05-04

jose.amengual avatar
jose.amengual
Bugfix timeouts by jamengual · Pull Request #65 · cloudposse/terraform-aws-rds-cluster

Fixing bug in timeout config, the reference for the value was wrong

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

please resolve the conflicts “This branch has conflicts that must be resolved”

Bugfix timeouts by jamengual · Pull Request #65 · cloudposse/terraform-aws-rds-cluster

Fixing bug in timeout config, the reference for the value was wrong

jose.amengual avatar
jose.amengual

done

jose.amengual avatar
jose.amengual

any takers?

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Andriy Knysh (Cloud Posse)

jose.amengual avatar
jose.amengual

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

looking @jose.amengual

jose.amengual avatar
jose.amengual

thanks

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)
cloudposse/terraform-aws-rds-cluster

Terraform module to provision an RDS Aurora cluster for MySQL or Postgres - cloudposse/terraform-aws-rds-cluster

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

@jose.amengual

jose.amengual avatar
jose.amengual

AWESOME!!!!

jose.amengual avatar
jose.amengual

thanks

jose.amengual avatar
jose.amengual

I introduced the bug

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

you break it, you fix it , thanks

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

for some reason, Tf does not complain about wrong dynamic

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

and we have tests which never showed any errors

jose.amengual avatar
jose.amengual

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

    keyboard_arrow_up