#pr-reviews (2021-02)

Pull Request Reviews for Cloud Posse Projects

2021-02-26

Matteo avatar
Matteo

Hi there, I don’t know how it works with pending PR, will post here for you https://github.com/cloudposse/terraform-aws-rds/pull/106

Terraform 0.13 requires snapshot_identifier explicitly set to null by nnsense · Pull Request #106 · cloudposse/terraform-aws-rds

what With terraform 0.13 If snapshot_identifier is set to "" then the resource creation will fail ("snapshot_identifier": conflicts with username). Apparently it works with &gt…

1
1
Matt Gowie avatar
Matt Gowie

Hey @ we’ve got some recently introduced hurdles to jump over in regards to GH actions that sometimes cause issues like in this PR where they don’t report status back. The only way I know to solve them to push another commit. See the comment here, follow those steps, and that should kick off that validate codeowners action which will enable us to get that PR merge — https://github.com/cloudposse/terraform-aws-rds/pull/106#issuecomment-784455208

Terraform 0.13 requires snapshot_identifier explicitly set to null by nnsense · Pull Request #106 · cloudposse/terraform-aws-rds

what With terraform 0.13 If snapshot_identifier is set to "" then the resource creation will fail ("snapshot_identifier": conflicts with username). Apparently it works with &gt…

Matteo avatar
Matteo

Oops, sorry I’ve missed your answer on GH, will do, thanks

Matteo avatar
Matteo

Done

2021-02-24

RB avatar

if anyone has a chance, i opened up 5 PRs here

https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pulls

cloudposse/terraform-aws-ecs-alb-service-task

Terraform module which implements an ECS service which exposes a web service via ALB. - cloudposse/terraform-aws-ecs-alb-service-task

1
1
1

2021-02-22

Bart Coddens avatar
Bart Coddens

Hi all, I enabled support for the s3 deep archive storage class:

Bart Coddens avatar
Bart Coddens

can you have a look ?

Bart Coddens avatar
Bart Coddens

Thx guys !

Leo Zavala avatar
Leo Zavala

Hi all, added GitHub Webhooks to the CI/CD module, would someone have a look? https://github.com/cloudposse/terraform-aws-cicd/pull/89

Adds feature: GitHub Webhooks by lezavala · Pull Request #89 · cloudposse/terraform-aws-cicd

what Adds GitHub Webhooks as in cloudposse/ecs-codepipeline/aws why Webhooks seems to be less noisy than polling and likely the preferred method. references CloudPosse/repository-webhooks modu…

2021-02-18

David Lozano avatar
David Lozano

Hi all, Can someone take a look at this one please? https://github.com/cloudposse/terraform-aws-vpc/pull/81

add ipv6_cidr_block variable to toggle IPv6 VPC association by 1david5 · Pull Request #81 · cloudposse/terraform-aws-vpc

what Add ipv6_cidr_block variable to toggle IPv6 VPC association. why This change allows the user to decide if IPv6 CIDR block should be associated to the VPC or not, since there are cases where…

1
1

2021-02-16

Joe Hosteny avatar
Joe Hosteny

Hi folks, seeing a bit of an odd issue on this PR. The build harness readme/build target seems to be non-deterministic from run to run, causing CI to fail. 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 …

Joe Hosteny avatar
Joe Hosteny

Basically, it wants to keep swapping the position of the S3 resource and the S3 data source link in the resources section.

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 …

Joe Hosteny avatar
Joe Hosteny

Haven’t dug into the code yet, but wondering if anyone has seen this?

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

There’s a new version of terraform docs

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

I haven’t looked at this particular issue but chances are it hasn’t been updated somewhere

1
Joe Hosteny avatar
Joe Hosteny

For posterity, we got around this temporarily by just reissuing the readme build via ChatOps, though I think that will also be non-deterministic with respect to the readme CI task

roth.andy avatar
roth.andy

Just wrote up https://github.com/cloudposse/charts/issues/266

No PR yet, but I can work on one later if nobody gets to it first

[incubator/monochart] Dockercfg name mismatch · Issue #266 · cloudposse/charts

Environment Chart: incubator/monochart v0.25.0 What I expect If I use the following in values.yaml dockercfg: enabled: true image: pullSecret: registry: foo username: bar password: baz … A secret…

2021-02-15

Leo Zavala avatar
Leo Zavala

Hi all,

Would someone take a look please? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/122

Expose codepipeline build cache bucket suffix variable. by lezavala · Pull Request #122 · cloudposse/terraform-aws-ecs-web-app

Exposes codepipelines 'cache_bucket_suffix_enabled' as 'codepipeline_build_cache_bucket_suffix_enabled' what Updates the codepipeline module to allow passing in the cache_bucket_su…

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

@Maxim Mironenko (Cloud Posse)

Expose codepipeline build cache bucket suffix variable. by lezavala · Pull Request #122 · cloudposse/terraform-aws-ecs-web-app

Exposes codepipelines 'cache_bucket_suffix_enabled' as 'codepipeline_build_cache_bucket_suffix_enabled' what Updates the codepipeline module to allow passing in the cache_bucket_su…

2021-02-10

David Lozano avatar
David Lozano

Hi everyone,

Can someone take a look at this one please? https://github.com/cloudposse/terraform-aws-vpc/pull/81

add ipv6_cidr_block variable to toggle IPv6 VPC association by 1david5 · Pull Request #81 · cloudposse/terraform-aws-vpc

what Add ipv6_cidr_block variable to toggle IPv6 VPC association. why This change allows the user to decide if IPv6 CIDR block should be associated to the VPC or not, since there are cases where…

2021-02-09

Frank avatar
Frank

Can someone take a look at this one? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/113 Thanks!

fix: make required outputs sensitive by syphernl · Pull Request #113 · cloudposse/terraform-aws-ecs-web-app

what Marks the outputs sensitive to be compatible with TF 0.14 why Otherwise TF 0.14 would give an Error: Output refers to sensitive values when using this module references https://www.terraf

1
1
Matt Gowie avatar
Matt Gowie

Good stuff [Released as 0.54.0> (Should’ve been a patch, but I missed the label </i](https://github.com/cloudposse/terraform-aws-ecs-web-app/releases/tag/0.54.0))

Release v0.54.0 · cloudposse/terraform-aws-ecs-web-app

fix: make required outputs sensitive @syphernl (#113) what Marks the outputs sensitive to be compatible with TF 0.14 why Otherwise TF 0.14 would give an Error: Output refers to sensitive values …

Leo Zavala avatar
Leo Zavala

@Erik Osterman (Cloud Posse) Hi all,

I am getting the following from the ECS web app module using webhooks. I am guessing its coming from the webhooks module. It seems GitHub there are breaking changes with the GitHub provider.

Warning: Additional provider information from registry

The remote registry returned warnings for
registry.terraform.io/hashicorp/github:
- For users on Terraform 0.13 or greater, this provider has moved to
integrations/github. Please update your source in required_providers.



Error: Failed to query available provider packages

Could not retrieve the list of available versions for provider
hashicorp/github: no available releases match the given constraints ~> 2.8.0,
3.0.0
cloudposse/terraform-aws-ecs-web-app

Terraform module that implements a web app on ECS and supports autoscaling, CI/CD, monitoring, ALB integration, and much more. - cloudposse/terraform-aws-ecs-web-app

cloudposse/terraform-github-repository-webhooks

Terraform module to provision webhooks on a set of GitHub repositories - cloudposse/terraform-github-repository-webhooks

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

@ reported this to me yesterday too

cloudposse/terraform-aws-ecs-web-app

Terraform module that implements a web app on ECS and supports autoscaling, CI/CD, monitoring, ALB integration, and much more. - cloudposse/terraform-aws-ecs-web-app

cloudposse/terraform-github-repository-webhooks

Terraform module to provision webhooks on a set of GitHub repositories - cloudposse/terraform-github-repository-webhooks

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

@Jeremy (Cloud Posse) can you take a look point I think this is related to something you recently were looking at

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@Erik Osterman (Cloud Posse) We need to use Github provider 3.0.0 because of breaking changes. We cannot use integrations/github because it does not have v3.0.0 (I think it starts at v4.2). However, the problem above seems due to the ~> 2.8.0 version pin of the GitHub provider. Not sure where that is coming from, but should be alleviated by updating all modules to the latest version.

1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Root modules should update their version pin to

    github = {
      source = "hashicorp/github"
      # breaking changes both immediately before and after 3.0.0, pin exactly
      # until ready to upgrade to integrations/github 4.2 or later
      version = "3.0.0"
    }
Leia Renée avatar
Leia Renée
08:25:39 PM

@ has joined the channel

2021-02-08

2021-02-07

Daniel Pilch avatar
Daniel Pilch
Feature/bool enabled iam key creation by danpilch · Pull Request #47 · cloudposse/terraform-aws-iam-system-user

what adds create_iam_access_key boolean variable to conditionally create aws_iam_access_key. defaults to true which aligns with current expected behavior why We would prefer to not create access…

1

2021-02-06

2021-02-05

Patrick Jahns avatar
Patrick Jahns

Regarding - https://github.com/cloudposse/terraform-aws-eks-node-group/pull/54 - there are some open questions that I am not able to answer ( in terns of aws-provider requirements and cloudposse customers ) - anyone that could chime in and provide a path forward? Would be amazing to have more than a single instance type in a spot instane node group

Fix allow several instance types by patrickjahns · Pull Request #54 · cloudposse/terraform-aws-eks-node-group

what Allow to specify more than a single instance_types for eks node groups Possible to pass more than a single item in the list since the the introduction of capacity_type see upstream pr why F…

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

hey @ thanks for following up

Fix allow several instance types by patrickjahns · Pull Request #54 · cloudposse/terraform-aws-eks-node-group

what Allow to specify more than a single instance_types for eks node groups Possible to pass more than a single item in the list since the the introduction of capacity_type see upstream pr why F…

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

we’ve had some other PRs get in the way of this - mostly related to null label and addressing an issue that prevented backwards compatibility

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)
Fix allow several instance types by patrickjahns · Pull Request #54 · cloudposse/terraform-aws-eks-node-group

what Allow to specify more than a single instance_types for eks node groups Possible to pass more than a single item in the list since the the introduction of capacity_type see upstream pr why F…

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

(we can discusson on monday)

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@Erik Osterman (Cloud Posse) The question for you to resolve is how confusing are we going to allow this module to be, particularly since Terraform does not allow us to output custom error messages. You can only specify multiple instance types if you do not use some non-obvious set of other features which require a launch template. We have gone to great lengths to automatically create and configure a launch template if and only if needed to support a feature (for example, tagging created resources), and we do not have a great way of telling people that the reason their plan/apply is failing is that you cannot use both tagging and spot instances at the same time.

2021-02-04

Frank avatar
Frank
fix: mark outputs as sensitive by syphernl · Pull Request #118 · cloudposse/terraform-aws-ecs-container-definition

what Marks the outputs as sensitive why Otherwise TF 0.14 would give an Error: Output refers to sensitive values when using these outputs to feed into other modules (e.g. terraform-aws-ecs-alb-s…

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

@Jeremy (Cloud Posse) I can’t figure out why it doesn’t like it

fix: mark outputs as sensitive by syphernl · Pull Request #118 · cloudposse/terraform-aws-ecs-container-definition

what Marks the outputs as sensitive why Otherwise TF 0.14 would give an Error: Output refers to sensitive values when using these outputs to feed into other modules (e.g. terraform-aws-ecs-alb-s…

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

we’ve recently merged other PRs that’ didn’t complain about code owners

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

That repo has old workflows. Somehow the mass update missed updating the workflows on that repo.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@ Please run make init; make github/init; make pr/auto-format and check in and push the changes.

1
Frank avatar
Frank

@Jeremy (Cloud Posse) Done!

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@Erik Osterman (Cloud Posse) this module had never been converted to [context.tf> and does not really need to be, since it does not call any other modules and takes an explicit ID (container_name). Furthermore, it already has an environment variable declared. I started to convert it, but then I thought maybe we should just leave it as not having <http://context.tf|context.tf](http://context.tf) . What do you think?

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

Yea I do not think this module needs context tf

1
1
    keyboard_arrow_up