#pr-reviews (2021-02)
Pull Request Reviews for Cloud Posse Projects
2021-02-04
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Could someone please take a look at this PR? https://github.com/cloudposse/terraform-aws-ecs-container-definition/pull/118
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Jeremy G (Cloud Posse) I can’t figure out why it doesn’t like it
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
we’ve recently merged other PRs that’ didn’t complain about code owners
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
That repo has old workflows. Somehow the mass update missed updating the workflows on that repo.
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@Frank Please run make init; make github/init; make pr/auto-format
and check in and push the changes.
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
@Jeremy G (Cloud Posse) Done!
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@Erik Osterman (Cloud Posse) this module had never been converted to [context.tf](http://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 [context.tf](http://context.tf)
. What do you think?
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
2021-02-05
![Patrick Jahns avatar](https://avatars.slack-edge.com/2021-01-07/1617422085682_1417104395b2c9f52fbe_72.png)
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
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
hey @Patrick Jahns thanks for following up
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Jeremy G (Cloud Posse) please see @Patrick Jahns comment: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/54#issuecomment-764994265
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
(we can discusson on monday)
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@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-06
2021-02-07
![Daniel Pilch avatar](https://avatars.slack-edge.com/2020-07-16/1257849126273_580541d293609aca0741_72.jpg)
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…
2021-02-08
2021-02-09
![Frank avatar](https://secure.gravatar.com/avatar/1b4b7744d083431522fb3e1b49206492.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
Can someone take a look at this one? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/113 Thanks!
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…
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
Good stuff Released as 0.54.0 (Should’ve been a patch, but I missed the label )
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](https://secure.gravatar.com/avatar/4e2af4492d3596b382d555013a4c882d.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0020-72.png)
@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
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
Terraform module to provision webhooks on a set of GitHub repositories - cloudposse/terraform-github-repository-webhooks
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Leia Renée reported this to me yesterday too
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
Terraform module to provision webhooks on a set of GitHub repositories - cloudposse/terraform-github-repository-webhooks
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Jeremy G (Cloud Posse) can you take a look point I think this is related to something you recently were looking at
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
@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.
![Jeremy G (Cloud Posse) avatar](https://avatars.slack-edge.com/2020-07-04/1229022582372_22757dbc9ef96d371614_72.jpg)
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](https://avatars.slack-edge.com/2021-01-28/1711168509744_c80591428b5c0784f5cd_72.png)
@Leia Renée has joined the channel
2021-02-10
![David Lozano avatar](https://avatars.slack-edge.com/2020-10-28/1453157962374_67b9b13d23898f6d2fda_72.png)
Hi everyone,
Can someone take a look at this one please? https://github.com/cloudposse/terraform-aws-vpc/pull/81
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-12
![mihai.plesa avatar](https://secure.gravatar.com/avatar/663277330091d41f09410afd2b1606e1.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0003-72.png)
2021-02-15
![Leo Zavala avatar](https://secure.gravatar.com/avatar/4e2af4492d3596b382d555013a4c882d.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0020-72.png)
Hi all,
Would someone take a look please? https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/122
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](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Maxim Mironenko (Cloud Posse)
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-16
![Joe Hosteny avatar](https://secure.gravatar.com/avatar/851f2d21e357fbb172c3abfc9860d9c5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
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
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](https://secure.gravatar.com/avatar/851f2d21e357fbb172c3abfc9860d9c5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
Basically, it wants to keep swapping the position of the S3 resource and the S3 data source link in the resources
section.
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](https://secure.gravatar.com/avatar/851f2d21e357fbb172c3abfc9860d9c5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
Haven’t dug into the code yet, but wondering if anyone has seen this?
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
There’s a new version of terraform docs
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
I haven’t looked at this particular issue but chances are it hasn’t been updated somewhere
![Joe Hosteny avatar](https://secure.gravatar.com/avatar/851f2d21e357fbb172c3abfc9860d9c5.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
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](https://avatars.slack-edge.com/2019-09-18/753707271651_6f58c1cbab3c77754f58_72.jpg)
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
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-18
![David Lozano avatar](https://avatars.slack-edge.com/2020-10-28/1453157962374_67b9b13d23898f6d2fda_72.png)
Hi all, Can someone take a look at this one please? https://github.com/cloudposse/terraform-aws-vpc/pull/81
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-22
![Bart Coddens avatar](https://secure.gravatar.com/avatar/2172a7ffce39295e04ea825a5bc9b0b6.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Hi all, I enabled support for the s3 deep archive storage class:
![Bart Coddens avatar](https://secure.gravatar.com/avatar/2172a7ffce39295e04ea825a5bc9b0b6.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Support transition to the deep archive storage class We need this for our business.
![Bart Coddens avatar](https://secure.gravatar.com/avatar/2172a7ffce39295e04ea825a5bc9b0b6.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
can you have a look ?
![Bart Coddens avatar](https://secure.gravatar.com/avatar/2172a7ffce39295e04ea825a5bc9b0b6.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0025-72.png)
Thx guys !
![Leo Zavala avatar](https://secure.gravatar.com/avatar/4e2af4492d3596b382d555013a4c882d.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0020-72.png)
Hi all, added GitHub Webhooks to the CI/CD module, would someone have a look? https://github.com/cloudposse/terraform-aws-cicd/pull/89
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-24
![RB avatar](https://avatars.slack-edge.com/2020-02-26/958727689603_86844033e59114029b3c_72.png)
if anyone has a chance, i opened up 5 PRs here
https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pulls
Terraform module which implements an ECS service which exposes a web service via ALB. - cloudposse/terraform-aws-ecs-alb-service-task
2021-02-26
![nnsense avatar](https://avatars.slack-edge.com/2021-11-13/2724381855556_9b369968ac638fae65f2_72.png)
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
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 >…
![Matt Gowie avatar](https://avatars.slack-edge.com/2023-02-06/4762019351860_44dadfaff89f62cba646_72.jpg)
Hey @nnsense 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
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 >…
![nnsense avatar](https://avatars.slack-edge.com/2021-11-13/2724381855556_9b369968ac638fae65f2_72.png)
Oops, sorry I’ve missed your answer on GH, will do, thanks
![nnsense avatar](https://avatars.slack-edge.com/2021-11-13/2724381855556_9b369968ac638fae65f2_72.png)
Done