Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create ECS cluster #2

Merged
merged 14 commits into from
May 20, 2018
Merged

Create ECS cluster #2

merged 14 commits into from
May 20, 2018

Conversation

arminc
Copy link
Contributor

@arminc arminc commented Mar 18, 2018

Initial ECS setup (no Fargate)

  • Create ECS
  • Create infrastracture needed by ECS (VPC, etc...)
  • Add EC2 instances module (AutoScale group, EC2, etc...)
  • How for do we go until first version?
  • Publish the module

This should be enough for version 0.0.1, from here on we can extend the module and the example as shown in the TODO in the example readme.

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor things and we can merge it as 0.0.1

@@ -0,0 +1,58 @@
provider "aws" {
region = "eu-west-1"
version = "v1.18.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to specify version here, because we need to be able to run examples using latest automatically.

outputs.tf Outdated
@@ -0,0 +1,3 @@
output "ecs_cluster_id" {
value = "${aws_ecs_cluster.ecs.0.id}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a replacement to concat when create_ecs = false?

@@ -0,0 +1,7 @@
# ECS instance policy

For an EC2 instance to connect it self to ECS it needs rights to do so.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

itself

main.tf Outdated
version = ">= 1.0.0"
}

resource "aws_ecs_cluster" "ecs" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ecs" => "this"

EOF
}

resource "aws_ecs_service" "hello-world" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use snake case for resource's names everywhere

}

#----- ECS Resources--------
module "ec2" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be simplified by using autoscaling module directly here instead of wrapper module (ec2)


module "vpc" {
source = "terraform-aws-modules/vpc/aws"
version = "v1.30.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to pin version here. We need to stay on the edge with examples to be able to see that this still works (yes, we lack automated tests for modules).

provider "terraform" {}

locals {
name = "my-ecs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually pass values similar to the name of the example to be able to have several examples at once - complete-ecs in this case.

# ... omitted
}
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a section Examples to describe each kind of example.

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor naming comments before merge

@@ -0,0 +1,3 @@
output "instance_profile_id" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name it this_iam_instance_profile_id

outputs.tf Outdated
@@ -0,0 +1,3 @@
output "ecs_cluster_id" {
value = "${element(concat(aws_ecs_cluster.this.*.id, list("")), 0)}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name this this_ecs_cluster_id

@antonbabenko antonbabenko merged commit 660da77 into master May 20, 2018
@antonbabenko antonbabenko deleted the initial-setup branch May 20, 2018 18:58
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants