#pr-reviews (2020-04)

Pull Request Reviews for Cloud Posse Projects

2020-04-24

Gowiem avatar
Gowiem

Hey folks — I just opened up Issue #207 on build-harness. Is there a reason ya’ll at CloudPosse are printing that message and not running terraform validate? Wanted to check here before I submitted a PR to fix.

`make terraform/validate` does not to run on Terraform v0.12.X · Issue #207 · cloudposse/build-harness

Describe the Bug Running make terraform/validate on Terraform v0.12.X prints the following message: Terraform 0.12 does not support validate without skipping variables Expected Behavior I would exp…

Gowiem avatar
Gowiem
The argument "region" is required, but was not set. · Issue #11058 · terraform-providers/terraform-provider-aws

This issue was originally opened by @Craigfis as hashicorp/terraform#23517. It was migrated here as a result of the provider split. The original body of the issue is below. I'm using Terraform …

"terraform validate" should not fail on implicit provider config when provider arguments are required · Issue #21408 · hashicorp/terraform

I ran the command terraform validate -check-variables=false and I got the below output. It seems like the command is not well format. However, when I run terraform validate then the command seems f…

Gowiem avatar
Gowiem

Aha — there is a workaround though… providing AWS_DEFAULT_REGION before tf validate allows validate to not throw that error. I’m going to put up a PR to fix…

"terraform validate" should not fail on implicit provider config when provider arguments are required · Issue #21408 · hashicorp/terraform

I ran the command terraform validate -check-variables=false and I got the below output. It seems like the command is not well format. However, when I run terraform validate then the command seems f…

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

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

@Gowiem won’t get to this until next week. @Maxim Mironenko (Cloud Posse) working on some other fixes at the moment.

Gowiem avatar
Gowiem

@Erik Osterman (Cloud Posse) No rush at all — I just ran into a handful of CP build-harness/test-harness issues while working on https://github.com/cloudposse/terraform-aws-ssm-parameter-store/pull/12.

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.

2020-04-23

Adam Crews avatar
Adam Crews

I’d love a review of https://github.com/cloudposse/terraform-aws-eks-workers/pull/33 if someone has a few moments. It pretty simple, and just adds the environment value from the label module into the rest of the resources.

2020-04-22

2020-04-20

cabrinha avatar
cabrinha
07:51:00 PM

@cabrinha has joined the channel

cabrinha avatar
cabrinha
07:51:00 PM

Hello there, I’d like to disable the creation of the s3 endpoint when using the EMR module: https://github.com/cloudposse/terraform-aws-emr-cluster/pull/14 – I’ve already got an S3 endpoint managed somewhere else.

1

2020-04-14

2020-04-13

2020-04-10

PePe avatar
Adding timeout for rds cluster resources and defaulting to TF default… by jamengual · Pull Request #64 · cloudposse/terraform-aws-rds-cluster

Adding configurable timeouts for create, update and delete actions. Default is to 120m base onf the Terraform default.

1
PePe avatar

any takers ?

Adding timeout for rds cluster resources and defaulting to TF default… by jamengual · Pull Request #64 · cloudposse/terraform-aws-rds-cluster

Adding configurable timeouts for create, update and delete actions. Default is to 120m base onf the Terraform default.

PePe avatar

anyone @here available ?

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

@PePe I am on it

PePe avatar

awesome sir

2020-04-08

Joe Niland avatar
Joe Niland
01:14:14 AM

@Joe Niland has joined the channel

Joe Niland avatar
Joe Niland

[reposted from #terraform]

Hi @Maxim Mironenko (Cloud Posse) would you mind reviewing this PR? I need a bit of help with the bats tests as well. When you have time, I’ll explain what I tried already. Thanks!! https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/27

Update for Terraform 0.12 by joe-niland · Pull Request #27 · cloudposse/terraform-aws-ec2-bastion-server

I have also added one example and tests, however I will need a bit of help to get the tests fully working.

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

sure, whats up with bats?

Update for Terraform 0.12 by joe-niland · Pull Request #27 · cloudposse/terraform-aws-ec2-bastion-server

I have also added one example and tests, however I will need a bit of help to get the tests fully working.

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

@Joe Niland give me some context of your problems

Joe Niland avatar
Joe Niland

Sure, one moment

Joe Niland avatar
Joe Niland

I copied the tests from another cloudposse module and modified them. They are here: https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/27/files#diff-920672592109ed71b04a314cf153cb22 I know I need to add a couple more tests but just getting started.

I run make to run the tests. All the initial ones are OK until check if terraform code is valid. This just seems to run terraform validate. If I run this manually it is returning success.

Update for Terraform 0.12 by joe-niland · Pull Request #27 · cloudposse/terraform-aws-ec2-bastion-server

I have also added one example and tests, however I will need a bit of help to get the tests fully working.

Joe Niland avatar
Joe Niland

Once I sort that out, I’ll see if the apply test works.

Joe Niland avatar
Joe Niland

Should be something silly I guess!

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

ok, I will pull your branch and test it on my local

Joe Niland avatar
Joe Niland

Thank you

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

will let you know

Joe Niland avatar
Joe Niland

@Maxim Mironenko (Cloud Posse) sorry just pushed a small fix for one of the test conditions

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

ok

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

@Joe Niland, please, check for my commit. I’ve fixed validation for example module

Joe Niland avatar
Joe Niland

@Maxim Mironenko (Cloud Posse) thanks - i’ll check it out

Joe Niland avatar
Joe Niland

ah I see - the validation was failing for the examples/complete module

Joe Niland avatar
Joe Niland

@Maxim Mironenko (Cloud Posse) PR is updated

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

thanks, will check for it

Joe Niland avatar
Joe Niland

Thanks!

Joe Niland avatar
Joe Niland

Hi @Maxim Mironenko (Cloud Posse) I was wondering if you have had time to look at this one. Let me know if I can fix anything.

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

Hi @Joe Niland! Will rise the priority of this one. Looks like the only thing left is to setup pipeline for tests. Should be done shortly. Stay tuned and thanks for reminder!

Joe Niland avatar
Joe Niland

Thanks @Maxim Mironenko (Cloud Posse)!

2020-04-06

PePe avatar
PePe
07:26:35 PM

@PePe has joined the channel

PePe avatar
PePe
07:26:35 PM
:--1:1
Mikhail Naletov avatar
Mikhail Naletov

Thanks for the reviews

2020-04-02

Gowiem avatar
Gowiem

Hey any chance I could get a push in the right direction on this PR? https://github.com/cloudposse/terraform-aws-ssm-parameter-store/pull/12

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.

:--1:1
Mikhail Naletov avatar
Mikhail Naletov
09:40:31 PM

@ has joined the channel

Mikhail Naletov avatar
Mikhail Naletov
Add environment var by okgolove · Pull Request #108 · cloudposse/terraform-aws-elastic-beanstalk-environment

what Environment variable why Most cloudposse modules have environment variable. This one doesn’t. The PR fixes it.

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

Next week we should be faster at reviewing

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

sorry for the hold up

2020-04-01

Adam Crews avatar
Adam Crews
Add blocking of public access to the bucket by adamcrews · Pull Request #25 · cloudposse/terraform-aws-s3-log-storage

This is a change to the current default behavior, where buckets will be left in a state where they could be made public, and instead makes the buckets block public access by default.

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

@Andriy Knysh (Cloud Posse) looks like a quick one (@Maxim Mironenko (Cloud Posse) is on vacation this week)

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

checking

    keyboard_arrow_up