#pr-reviews (2021-03)

Pull Request Reviews for Cloud Posse Projects

2021-03-01

RB avatar
Fewer required variables by nitrocode · Pull Request #127 · cloudposse/terraform-aws-ecs-web-app

what Added 3 variable defaults. The ecs cluster name is only required if codepipeline is enabled. The region variables don't need to be specified when the current aws region could be provided …

1
RB avatar
Default priority to null by nitrocode · Pull Request #53 · cloudposse/terraform-aws-alb-ingress

what Set the priority variables to null by default why If you do not set the priority, the listener rule will automatically set an unused priority for you which is a very nice default. We can ta…

1
RB avatar

Still says it needs to validate code owners. I saw the other day that someone was able to retrigger that check and it worked. Is there a gitops command for that?

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

Push an empty commit

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

Basically anytime the auto-format formats code, it causes the validate code owners to not run. We can fix it but it will require another mass update. We are trying to queue up some updates for that

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

In the meantime just make sure to auto format locally to avoid any issues

RB avatar
Fewer required variables by nitrocode · Pull Request #127 · cloudposse/terraform-aws-ecs-web-app

what Added 3 variable defaults. The ecs cluster name is only required if codepipeline is enabled. The region variables don't need to be specified when the current aws region could be provided …

1

2021-03-02

RB avatar

review plz https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/126
Run make init
make init
shell: bash -e -o pipefail {0}
env:
MAKE_INCLUDES: Makefile
BUILD_HARNESS_BRANCH: master
curl: (6) Could not resolve host: git.io
make: *** No rule to make target ‘init’. Stop.
Error: Process completed with exit code 2.

secrets valueFrom by nitrocode · Pull Request #126 · cloudposse/terraform-aws-ecs-container-definition

what Fixes an issue in a previous PR #123 where value was used instead of valueFrom for the secrets key why This fixes a bug in which the secrets key will break the module due to the incorrect k…

1
jose.amengual avatar
jose.amengual

the readme

secrets valueFrom by nitrocode · Pull Request #126 · cloudposse/terraform-aws-ecs-container-definition

what Fixes an issue in a previous PR #123 where value was used instead of valueFrom for the secrets key why This fixes a bug in which the secrets key will break the module due to the incorrect k…

jose.amengual avatar
jose.amengual

is not working for some reason

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

RB avatar

nothing is returned from https://git.io/build-harness

RB avatar

DNS_PROBE_FINISHED_NXDOMAIN

RB avatar
$ curl -vvv <https://git.io/build-harness>
* Could not resolve host: git.io
* Closing connection 0
curl: (6) Could not resolve host: git.io
RB avatar

so this is what happens when git.io is down

jose.amengual avatar
jose.amengual

well I guess we will have to wait

Alex Jurkiewicz avatar
Alex Jurkiewicz

I was looking for this functionality yesterday! Nice one

1
RB avatar

looks like it’s back @jose.amengual

RB avatar

i did a /rebuild-readme

RB avatar

but it doesnt look like it’s running. last fail was from an hour ago

RB avatar

the emojis are on my comment…

jose.amengual avatar
jose.amengual

weird

RB avatar

i just ran a /test all and i got a thumbs up emoji but tests aren’t rerunning

jose.amengual avatar
jose.amengual

I’m running it manually to see

RB avatar

the tests look like they kicked off!

RB avatar

Thanks all merged.

1
RB avatar
add sensitive variants of outputs by syphernl · Pull Request #124 · cloudposse/terraform-aws-ecs-container-definition

what Add additional outputs which are marked sensitive which can be used when passing on secrets (secrets) or secret maps (map_secrets). why When using secrets (or map_secrets) and without sensi…

Frank avatar

https://github.com/cloudposse/terraform-aws-rds/pull/109 Not sure why the do not merge label was added without any comments

create_before_destroy for parameter groups, explicit dependencies by syphernl · Pull Request #109 · cloudposse/terraform-aws-rds

what Make parameter groups create_before_destroy Make the name for parameter & option groups unique to ensure we can create new one&#39;s Add explicit dependencies why You cannot delete a pa…

1

2021-03-08

Frank avatar

https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/124 Can someone please take a look at this one? Thanks!

add sensitive variants of outputs by syphernl · Pull Request #124 · cloudposse/terraform-aws-ecs-container-definition

what Add additional outputs which are marked sensitive which can be used when passing on secrets (secrets) or secret maps (map_secrets). why When using secrets (or map_secrets) and without sensi…

1
Matt Gowie avatar
Matt Gowie

@Frank merged

add sensitive variants of outputs by syphernl · Pull Request #124 · cloudposse/terraform-aws-ecs-container-definition

what Add additional outputs which are marked sensitive which can be used when passing on secrets (secrets) or secret maps (map_secrets). why When using secrets (or map_secrets) and without sensi…

Frank avatar

Thanks!

2021-03-09

Frank avatar

https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/115 <– the 0.35.0 version causes quite a few problems in existing setups. This PR should provide a way to work around that.

fix: make the security group description configurable by syphernl · Pull Request #115 · cloudposse/terraform-aws-elasticache-redis

what Made the security group description configurable why Since v0.35.0 a description will be set which replaces the default &quot;Managed by Terraform&quot;. Unfortunately, a change in security…

1
marcuz avatar
Allow empty "egress_cidr_blocks" by marcuz · Pull Request #93 · cloudposse/terraform-aws-elasticache-redis

what Allow empty egress_cidr_blocks (no egress rules in default security group) why Not mandatory to have an egress rule references #90

2021-03-12

Bart Coddens avatar
Bart Coddens

hey all, I am a bit confused what is needed for this pull request:

Bart Coddens avatar
Bart Coddens
Add secondary sources by cfir · Pull Request #73 · cloudposse/terraform-aws-codebuild

what I am adding dynamic object to support secindary sources why You are not supporting it right now and we need it.

jose.amengual avatar
jose.amengual

comment on the PR to get more details

Add secondary sources by cfir · Pull Request #73 · cloudposse/terraform-aws-codebuild

what I am adding dynamic object to support secindary sources why You are not supporting it right now and we need it.

mihai.plesa avatar
mihai.plesa

hello again, here’s one adding more utility to the CDN by passing cache policy ID https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/57

Adding cache_policy_id to default and ordered behaviours by justnom · Pull Request #57 · cloudposse/terraform-aws-cloudfront-cdn

what Adding cache_policy_id to the aws_cloudfront_distribution resource. why We want to provide a cloudfront_cache_policy resource to the module. references aws_cloudfront_distribution Terraf…

1
1

2021-03-15

Bart Coddens avatar
Bart Coddens

hey all, to proceed pull request 73, I rebased:

Bart Coddens avatar
Bart Coddens
Integrate secondary source functionality in codebuild by bcoddens · Pull Request #85 · cloudposse/terraform-aws-codebuild

what Our codebuild environment needs double sources as input to build why We use code that is based on gitlab , gitlab is not supported by codebuild so we need to upload zip files to support our…

Bart Coddens avatar
Bart Coddens

could you have a look ?

jose.amengual avatar
jose.amengual

looks like is still behind master

Bart Coddens avatar
Bart Coddens

ran a fmt through all the terraform code in the branch

Bart Coddens avatar
Bart Coddens

and pushed

Bart Coddens avatar
Bart Coddens

should I allow the cloudpossebot write access to my repo ?

jose.amengual avatar
jose.amengual

yes

jose.amengual avatar
jose.amengual

you can pull from origin/master and fix the conflicts

Bart Coddens avatar
Bart Coddens

the cloudposse master ?

jose.amengual avatar
jose.amengual

no the master branch

jose.amengual avatar
jose.amengual

your code is behind so you need to rebase

Bart Coddens avatar
Bart Coddens

ok, let me check

Bart Coddens avatar
Bart Coddens

thx for the help, I don’t do this often

2021-03-16

Bart Coddens avatar
Bart Coddens

Hi all, found the time to do this properly. This is a rebase of pull request 73:

Bart Coddens avatar
Bart Coddens
Integrated the changes from dome9 by bcoddens · Pull Request #86 · cloudposse/terraform-aws-codebuild

What Our codebuild environment needs double sources as input to build Why We use code that is based on gitlab , gitlab is not supported by codebuild so we need to upload zip files to support our co…

1
jose.amengual avatar
jose.amengual

I left a comment

Integrated the changes from dome9 by bcoddens · Pull Request #86 · cloudposse/terraform-aws-codebuild

What Our codebuild environment needs double sources as input to build Why We use code that is based on gitlab , gitlab is not supported by codebuild so we need to upload zip files to support our co…

Bart Coddens avatar
Bart Coddens

let me check, thanks

Bart Coddens avatar
Bart Coddens

pushed a change, is this what you mean ?

jose.amengual avatar
jose.amengual

left another comment but basically you can do : for_each = var.secondary_sources

Bart Coddens avatar
Bart Coddens

yeah that’s clear, thx

Bart Coddens avatar
Bart Coddens

pushed

jose.amengual avatar
jose.amengual
feat: add support for optional S3 secondary artifact by jhosteny · Pull Request #78 · cloudposse/terraform-aws-codebuild

Fixes #77. what Adds an optional secondary artifact deployment to S3 why Allow the https://github.com/cloudposse/terraform-aws-ecs-web-app module to use the changes for a lightweight deployment …

Bart Coddens avatar
Bart Coddens

yeah I saw this as well

Bart Coddens avatar
Bart Coddens

it could go hand in hand indeed

Bart Coddens avatar
Bart Coddens

let me check

jose.amengual avatar
jose.amengual

secondary_sources is missing a description

jose.amengual avatar
jose.amengual

the tests are failing on that

Bart Coddens avatar
Bart Coddens

let me check

Bart Coddens avatar
Bart Coddens

where should that go ?

jose.amengual avatar
jose.amengual

you need a description in every variable

Bart Coddens avatar
Bart Coddens

ha correct, let me fix it

Bart Coddens avatar
Bart Coddens

pushed, I am not a native English speaker, so feel free to correct

jose.amengual avatar
jose.amengual

me neither

jose.amengual avatar
jose.amengual

what do you mean by (Optional) secondary source for the built next to the primary location defined in variable source_location

jose.amengual avatar
jose.amengual

built or build?

Bart Coddens avatar
Bart Coddens

to build

Bart Coddens avatar
Bart Coddens

I will rephrase, second

jose.amengual avatar
jose.amengual

ok

Bart Coddens avatar
Bart Coddens

rephrased and pushed

jose.amengual avatar
jose.amengual

so it is an additional source for the build

jose.amengual avatar
jose.amengual

?

Bart Coddens avatar
Bart Coddens

yes correct

Bart Coddens avatar
Bart Coddens

so you can use two s3 buckets for example, sepparate from each other

Bart Coddens avatar
Bart Coddens

we do use it as such

jose.amengual avatar
jose.amengual

(Optional) secondary source for the codebuild project in addition to the primary location

Bart Coddens avatar
Bart Coddens

agreed

jose.amengual avatar
jose.amengual

that is more concise

Bart Coddens avatar
Bart Coddens

agreed

jose.amengual avatar
jose.amengual

please change it

Bart Coddens avatar
Bart Coddens

done

jose.amengual avatar
jose.amengual

merged

Bart Coddens avatar
Bart Coddens

excellent !

Bart Coddens avatar
Bart Coddens

thx man !

jose.amengual avatar
jose.amengual

np

Bart Coddens avatar
Bart Coddens

Could you have a look ?

Bart Coddens avatar
Bart Coddens

A very big thank you to @jose.amengual for helping me with merging code ! You are one of kind !

1
1
Bart Coddens avatar
Bart Coddens

here goes the other one:

Bart Coddens avatar
Bart Coddens
S3 secondary artifact integration by bcoddens · Pull Request #87 · cloudposse/terraform-aws-codebuild

Fixes #77. what Adds an optional secondary artifact deployment to S3 why Allow the https://github.com/cloudposse/terraform-aws-ecs-web-app module to use the changes for a lightweight deployment of …

jose.amengual avatar
jose.amengual

make sure to run make init

S3 secondary artifact integration by bcoddens · Pull Request #87 · cloudposse/terraform-aws-codebuild

Fixes #77. what Adds an optional secondary artifact deployment to S3 why Allow the https://github.com/cloudposse/terraform-aws-ecs-web-app module to use the changes for a lightweight deployment of …

jose.amengual avatar
jose.amengual

make readme

jose.amengual avatar
jose.amengual

closed in favour of #77

Joe Hosteny avatar
Joe Hosteny

Hi all, since there have been a few changes on this module lately, can I get a final review of this one too? https://github.com/cloudposse/terraform-aws-codebuild/pull/78

feat: add support for optional S3 secondary artifact by jhosteny · Pull Request #78 · cloudposse/terraform-aws-codebuild

Fixes #77. what Adds an optional secondary artifact deployment to S3 why Allow the https://github.com/cloudposse/terraform-aws-ecs-web-app module to use the changes for a lightweight deployment …

1
Joe Hosteny avatar
Joe Hosteny

Thanks @jose.amengual!

feat: add support for optional S3 secondary artifact by jhosteny · Pull Request #78 · cloudposse/terraform-aws-codebuild

Fixes #77. what Adds an optional secondary artifact deployment to S3 why Allow the https://github.com/cloudposse/terraform-aws-ecs-web-app module to use the changes for a lightweight deployment …

jose.amengual avatar
jose.amengual

np

jose.amengual avatar
jose.amengual

@Joe Hosteny have you seen this :

resource/aws_codebuild_project: The source and secondary_sources configuration block auth attributes have been deprecated to match the CodeBuild API documentation. Use the aws_codebuild_source_credential resource instead. (#17465)
jose.amengual avatar
jose.amengual
Release v3.33.0 · hashicorp/terraform-provider-aws

NOTES: data-source/aws_vpc_endpoint_service: The service_type argument filtering has been switched from client-side to new EC2 API functionality (#17641) provider: New default_tags argument as a p…

Joe Hosteny avatar
Joe Hosteny

I have not. We are only using the secondary artifact config, which I don’t think this applies to?

jose.amengual avatar
jose.amengual

ohh I think it was @Bart Coddens

jose.amengual avatar
jose.amengual

you both have PRs for the repo

Joe Hosteny avatar
Joe Hosteny

I could help look into this one, but I will be on vacation this week

jose.amengual avatar
jose.amengual

np

2021-03-17

2021-03-18

RB avatar
Add checks for var.secrets and var.environment by nitrocode · Pull Request #129 · cloudposse/terraform-aws-ecs-container-definition

what Adds a null check for secrets and environment why For backwards compatibility cloudposse/terraform-aws-ecs-web-app#134 (comment) references N/A

1

2021-03-19

2021-03-21

2021-03-23

RB avatar
var.use_name_prefix by nitrocode · Pull Request #60 · cloudposse/terraform-aws-ec2-autoscale-group

what Toggles between name and name_prefix asg arguments why To backport existing resources to use this module without recreating resources references Closes #28

1

2021-03-24

Frank avatar
feat: add force_new_deployment variable by syphernl · Pull Request #112 · cloudposse/terraform-aws-ecs-alb-service-task

what Added the force_new_deployment flag to force new task deployment of the service Updated github configs why To make it possible to force a new deployment

Frank avatar
feat: add variables key_usage and customer_master_key_spec by syphernl · Pull Request #27 · cloudposse/terraform-aws-kms-key

what Added optional variables key_usage and customer_master_key_spec Updated GitHub configs why To be able to use the created KMS key with DNSSEC in Route53. The defaults remain unchanged. refe…

1
1
Matt Gowie avatar
Matt Gowie

Have to get a core team member to review this one. Will start that process.

feat: add variables key_usage and customer_master_key_spec by syphernl · Pull Request #27 · cloudposse/terraform-aws-kms-key

what Added optional variables key_usage and customer_master_key_spec Updated GitHub configs why To be able to use the created KMS key with DNSSEC in Route53. The defaults remain unchanged. refe…

RB avatar

Could i please get a review for this ^

RB avatar

All 5 of the open PRs for the asg module are also ready for review

https://github.com/cloudposse/terraform-aws-ec2-autoscale-group/pulls

cloudposse/terraform-aws-ec2-autoscale-group

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

2021-03-25

2021-03-26

jose.amengual avatar
jose.amengual

no, you burn out your Quota for the week

1
Matt Gowie avatar
Matt Gowie

I got some quota left — particularly for small changes like that one

Matt Gowie avatar
Matt Gowie

Though you need to post an empty commit to re-trigger some failed CI @RB

RB avatar

i think i messed it up

RB avatar

but now it’s running all the tests again lol

Matt Gowie avatar
Matt Gowie

Haha as long as it runs that one test then you’re good.

Matt Gowie avatar
Matt Gowie

Merged

1
RB avatar
Fix example in README.md by Appelberg-s · Pull Request #131 · cloudposse/terraform-aws-ecs-web-app

what The example provided in the README.md gave an error when trying to run it since it set the attribute environment when it was probably trying set the attribute container_environment.

1
jose.amengual avatar
jose.amengual

this guy AGAIN!!!!!!

RB avatar
Fix example in README.md by Appelberg-s · Pull Request #131 · cloudposse/terraform-aws-ecs-web-app

what The example provided in the README.md gave an error when trying to run it since it set the attribute environment when it was probably trying set the attribute container_environment.

RB avatar

the last update on the readme was in line with the automation, no ?

RB avatar

he updated the yaml

jose.amengual avatar
jose.amengual

mmm no, he needs to only update the yaml

jose.amengual avatar
jose.amengual

the then run make readme

RB avatar

wouldn’t it be the same result ?

RB avatar

if i ran /rebuild-readme wouldnt it be enough ?

jose.amengual avatar
jose.amengual

it will fail I think

jose.amengual avatar
jose.amengual

but the reason behind running the make command is that the readme itself gets updated with the latest too

RB avatar

that makes sense!

2021-03-27

jose.amengual avatar
jose.amengual

not even on the weekends…..

1
RB avatar
BucketRegionError: incorrect region · Issue #101 · cloudposse/terraform-aws-cloudfront-s3-cdn

Describe the Bug I am getting the following error when using this module: Error: Failed getting S3 bucket: BucketRegionError: incorrect region, the bucket is not in &#39;eu-central-1&#39; region at…

Remove static bucket hack by nitrocode · Pull Request #143 · cloudposse/terraform-aws-cloudfront-s3-cdn

This is for comments to ensure this PR doesn&#39;t break backwards compatibility. what This simplifies calculating the correct bucket regional domain name using the s3 bucket&#39;s bucket_regional…

RB avatar

Could i get another review on the removal of the static s3 bucket hack?

BucketRegionError: incorrect region · Issue #101 · cloudposse/terraform-aws-cloudfront-s3-cdn

Describe the Bug I am getting the following error when using this module: Error: Failed getting S3 bucket: BucketRegionError: incorrect region, the bucket is not in &#39;eu-central-1&#39; region at…

Remove static bucket hack by nitrocode · Pull Request #143 · cloudposse/terraform-aws-cloudfront-s3-cdn

This is for comments to ensure this PR doesn&#39;t break backwards compatibility. what This simplifies calculating the correct bucket regional domain name using the s3 bucket&#39;s bucket_regional…

jose.amengual avatar
jose.amengual

OMG that is confusing

jose.amengual avatar
jose.amengual

the workaround of the data sources is interesting

2021-03-28

RB avatar

Fully removes the template provider from versions.tf. i forgot about this file in my last pr

https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/146

Remove unused template provider by nitrocode · Pull Request #146 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Removes the unused template provider why Forgot to remove the provider in this PR #141 references Fully closes #139

1

2021-03-29

2021-03-30

Frank avatar

Can someone please take a look at this one? Thanks! https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/147

Correct enabled behavior by syphernl · Pull Request #147 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Replaced the enabled flag with distribution_enabled Corrected behavior for actual enabled flag to prevent from creating any resources why The enabled input was being used to manage the dist…

1
mihai.plesa avatar
mihai.plesa

another good one from Harry https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/60 thanks in advance!

Adding Origin Request Policy for cache behaviours by justnom · Pull Request #60 · cloudposse/terraform-aws-cloudfront-cdn

what Adds the Origin Request Policy attribute to the CloudFront distribution cache behaviours Nullifies any cache forwarded values if non-legacy cache behaviours are set. Taken from @dmattia PR. …

1
1
mihai.plesa avatar
mihai.plesa

thank you @jose.amengual

Adding Origin Request Policy for cache behaviours by justnom · Pull Request #60 · cloudposse/terraform-aws-cloudfront-cdn

what Adds the Origin Request Policy attribute to the CloudFront distribution cache behaviours Nullifies any cache forwarded values if non-legacy cache behaviours are set. Taken from @dmattia PR. …

1
RB avatar
Remove static bucket hack by nitrocode · Pull Request #143 · cloudposse/terraform-aws-cloudfront-s3-cdn

This is for comments to ensure this PR doesn&#39;t break backwards compatibility. what This simplifies calculating the correct bucket regional domain name using the s3 bucket&#39;s bucket_regional…

1
RB avatar

cc: @Andriy Knysh (Cloud Posse) if time permits since you left a comment on it last

Remove static bucket hack by nitrocode · Pull Request #143 · cloudposse/terraform-aws-cloudfront-s3-cdn

This is for comments to ensure this PR doesn&#39;t break backwards compatibility. what This simplifies calculating the correct bucket regional domain name using the s3 bucket&#39;s bucket_regional…

RB avatar
Set minimum protocol version to TLSv1.2_2019 by nitrocode · Pull Request #144 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Set minimum protocol version to TLSv1.2_2019 why This is the most secure option as it only supports tls 1.2 and 1.3 references https://aws.amazon.com/about-aws/whats-new/2020/07/cloudfron

1
1
RB avatar

Very small pr

Set minimum protocol version to TLSv1.2_2019 by nitrocode · Pull Request #144 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Set minimum protocol version to TLSv1.2_2019 why This is the most secure option as it only supports tls 1.2 and 1.3 references https://aws.amazon.com/about-aws/whats-new/2020/07/cloudfron

RB avatar

Thanks Pepe!

1
RB avatar
Provide better description for var.origin_bucket by nitrocode · Pull Request #142 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Provide better description for var.origin_bucket Provide an example of reusing an s3 bucket in the README why There&#39;s some confusion whether to define this value as the bucket to be cre…

1

2021-03-31

RB avatar

You folks are probably getting tired of me. Almost done with all the prs in cloudfront cdn!

https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/149

Use `website_endpoint` instead of `bucket_regional_domain_name` by nitrocode · Pull Request #149 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Uses the bucket website_endpoint instead of the bucket_regional_domain_name The bucket_regional_domain_name is incorrectly documented in the hashicorp aws_cloudfront_distribution docs why F…

1
jose.amengual avatar
jose.amengual

long time ago but we still like you

2
RB avatar
Use acm module instead of awscli by nitrocode · Pull Request #145 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Use acm module instead of awscli Kept original awscli command for posterity why Why use the CLI when you can use terraform ? references Closes #26

jose.amengual avatar
jose.amengual

test failing

Use acm module instead of awscli by nitrocode · Pull Request #145 · cloudposse/terraform-aws-cloudfront-s3-cdn

what Use acm module instead of awscli Kept original awscli command for posterity why Why use the CLI when you can use terraform ? references Closes #26

    keyboard_arrow_up