#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?

PePe avatar

@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)

@PePe 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

PePe avatar

that is the same problem I have

PePe avatar

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

PePe avatar

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

PePe avatar

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

PePe avatar

I will remind you guys next week

:--1:1
PePe avatar

I’m very persistent :)

PePe avatar

if you haven’t noticed

1
1
PePe avatar
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

PePe avatar

passing that test user and token?

PePe avatar

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?

PePe avatar

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

1
PePe avatar

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

PePe avatar

silence…..

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

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

PePe avatar

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)

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

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

@Marcin Brański has joined the channel

2020-05-13

PePe avatar

Any takers

2020-05-12

2020-05-11

PePe avatar
1
PePe avatar

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

PePe avatar

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

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

@PePe please see comments on your PR

PePe avatar

changed, it should be good now

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

still not perfect (please see comments)

PePe avatar

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

PePe avatar

I commited your changes

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

@PePe I commented again, please review

PePe avatar

ohhhhh right yes you need to have two….

PePe avatar

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

PePe avatar

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

PePe avatar

exactly

PePe avatar

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 @PePe

PePe avatar

thank you

2020-05-08

PePe avatar

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

PePe avatar

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 @PePe 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: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

PePe avatar

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

PePe avatar
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

PePe avatar

done

PePe avatar

any takers?

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

@Andriy Knysh (Cloud Posse)

PePe avatar

:–1:

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

looking @PePe

PePe avatar

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)

@PePe

PePe avatar

AWESOME!!!!

PePe avatar

thanks

PePe avatar

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

PePe avatar

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