Add Terraform templates #477

baniol · 2018-07-04T22:49:30Z

What did you implement:

Closes #316

How did you implement it:

How can we verify it:

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

codecov · 2018-07-04T22:55:54Z

Codecov Report

Merging #477 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master    #477   +/-   ##
======================================
  Coverage    71.2%   71.2%           
======================================
  Files          37      37           
  Lines        2195    2195           
======================================
  Hits         1563    1563           
  Misses        571     571           
  Partials       61      61

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c84ddec...b349a32. Read the comment docs.

mthenw

Hey @baniol,

that looks awesome. Thanks for the great PR. I have few comments though.

mthenw

Hey @baniol,

that looks awesome. Thanks for the great PR. I have few comments though.

mthenw · 2018-07-05T13:38:11Z

terraform/README.md

@@ -0,0 +1,31 @@
+# Terraform templates for Event Gateway


Can you move all those file to /contrib/terraform folder.

Ideally would be to expose it as a module that someone else can use. So I think this readme should be mainly about how to use this module.

Not a problem. I'll refactor it in that direction

mthenw · 2018-07-05T13:39:12Z

terraform/network.tf

@@ -0,0 +1,60 @@
+data "aws_availability_zones" "available" {}


What do you think about using existing module for creating VPC and all network stuff https://registry.terraform.io/modules/terraform-aws-modules/vpc/aws/1.37.0?

You're right. Using some community modules makes sense.

mthenw · 2018-07-05T13:42:11Z

terraform/alb.tf

+}
+
+# Config load balancer
+resource "aws_lb" "config" {


This module should probably simplify the setup here https://registry.terraform.io/modules/terraform-aws-modules/alb/aws/3.4.0?tab=resources

The problem with this module is that it doesn't seem to allow to control target group health checks. We need to overwrite the default "/" path with "/v1/status". What's more, the events API server doesn't expose any health check endpoint returning 200 code (the root returns 202).

Good point but looks like it should be possible.

yes, I've missed the default section. Works perfectly with the alb module.

mthenw · 2018-07-05T13:52:52Z

terraform/etcd.tf

@@ -0,0 +1,10 @@
+module "etcd" {


What do you think about using existing module from CoreOS https://github.com/coreos/tectonic-installer/tree/track-1/modules/aws/etcd?

At first glance, it seems like an internal tectonic installer module, but might be useful. I'll check it.

This one will be harder to use as it is. Etcd is integrated with the k8s installer and has some internal dependencies, like dns, certs or s3 bucket with init scripts.
I need to take a deeper look.

mthenw · 2018-07-09T09:47:05Z

contrib/example.tf

@@ -0,0 +1,17 @@
+module "event-gateway" {
+  source = "terraform"


Could you change it to full github URL so people can just copy/paste?

sure, I'll do a little refactor and add a readme file when I'm done with the etcd module

awesome thanks!

mthenw · 2018-07-10T07:56:23Z

.gitignore

@@ -8,3 +8,4 @@ default.etcd*
 coverage.txt
 profile.out
 tests/testing.etcd*
+.terraform


we don't need this here if it's only a module

baniol · 2018-07-15T21:35:21Z

@mthenw A few notes regarding the latest commit:

the coreos etcd module is a part of Kubernetes setup in Tectonic installer and has some dependencies, like certificates, coreos ignition, dns. I've extracted those dependencies into the etcd wrapper (modules/etcd)
Tectonic Installer repo is under Apache v2 licence, so I've included the NOTICE file
it's possible to use https for the etcd (tls_enabled param), however eg libkv store does not initialize with tls config. I don't think this it a big issue for now, as the etcd cluster runs in private subnets.

mthenw · 2018-07-24T10:31:02Z

@baniol overall looks good. We need few days to review it. Again, thanks for contribution.

sebito91

I think this looks good. Just like your other PR if you can please rebase against current commits we'll merge.

baniol changed the title ~~Add Terraform templates (#316)~~ Add Terraform templates Jul 4, 2018

mthenw suggested changes Jul 5, 2018

View reviewed changes

mthenw reviewed Jul 9, 2018

View reviewed changes

mthenw reviewed Jul 10, 2018

View reviewed changes

mthenw requested a review from sebito91 July 23, 2018 08:23

sebito91 self-assigned this Jul 26, 2018

sebito91 added the WIP label Jul 26, 2018

sebito91 suggested changes Jul 31, 2018

View reviewed changes

sebito91 added the stage/pending-contributor label Jul 31, 2018

mthenw approved these changes Jul 31, 2018

View reviewed changes

Add Terraform templates (serverless#316)

b349a32

baniol force-pushed the terraform branch from 68b1859 to b349a32 Compare July 31, 2018 08:03

sebito91 approved these changes Jul 31, 2018

View reviewed changes

sebito91 merged commit de6e4e0 into serverless:master Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Terraform templates #477

Add Terraform templates #477

baniol commented Jul 4, 2018 •

edited by sebito91

Loading

codecov bot commented Jul 4, 2018 •

edited

Loading

mthenw left a comment

mthenw left a comment

mthenw Jul 5, 2018

baniol Jul 5, 2018

mthenw Jul 5, 2018

baniol Jul 5, 2018

mthenw Jul 5, 2018

baniol Jul 5, 2018

mthenw Jul 6, 2018

baniol Jul 6, 2018

mthenw Jul 5, 2018

baniol Jul 5, 2018

baniol Jul 6, 2018

mthenw Jul 9, 2018

baniol Jul 9, 2018

mthenw Jul 10, 2018

mthenw Jul 10, 2018

baniol commented Jul 15, 2018

mthenw commented Jul 24, 2018

sebito91 left a comment

		@@ -0,0 +1,60 @@
		data "aws_availability_zones" "available" {}

		@@ -0,0 +1,17 @@
		module "event-gateway" {
		source = "terraform"

Add Terraform templates #477

Add Terraform templates #477

Conversation

baniol commented Jul 4, 2018 • edited by sebito91 Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

codecov bot commented Jul 4, 2018 • edited Loading

Codecov Report

mthenw left a comment

Choose a reason for hiding this comment

mthenw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baniol commented Jul 15, 2018

mthenw commented Jul 24, 2018

sebito91 left a comment

Choose a reason for hiding this comment

baniol commented Jul 4, 2018 •

edited by sebito91

Loading

codecov bot commented Jul 4, 2018 •

edited

Loading