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

Terraform Part 1 #42

Merged
merged 16 commits into from
Aug 31, 2017
Merged

Terraform Part 1 #42

merged 16 commits into from
Aug 31, 2017

Conversation

jamesmcclain
Copy link
Member

@jamesmcclain jamesmcclain commented Aug 30, 2017

screenshot from 2017-08-31 09 02 59

To test:

  • Fetch the terraform-wip branch which is the same as this one except for an extra commit that points to an update docker image (the changes in this PR have not been published to quay.io).
  • Go into the terraform directory
  • Use terraform
    • terraform init
    • terraform apply
    • ...
    • terraform destroy

Still needs:

  • Connect to EMR cluster
  • Parameterize ECS instance role, VPC, subnet

James McClain added 16 commits August 29, 2017 12:11
If `log_uri` is a "s3://..." URL, it seems to get turned into a
"s3n://..." URL internally, which is different from the variable.  That
seems to cause terraform to want to tear down and rebuild the cluster
when `terraform apply` is run with a cluster already running.
Must connect directly to the instance, the load balancer does not pass
the websocket through.
@jamesmcclain jamesmcclain changed the title [WiP] Terraform Terraform Aug 31, 2017
Copy link
Contributor

@echeipesh echeipesh left a comment

Choose a reason for hiding this comment

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

Not looking deeply into terraform stuff the other changes look ok.

@@ -0,0 +1,5 @@
provider "aws" {
access_key = "${var.access_key}"

Choose a reason for hiding this comment

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

This can be simplified to the snippet below if you set AWS_PROFILE when invoking terraform:

provider "aws" {
  region = "${var.region}"
}

}
}

data "aws_instance" "emr-master" {

Choose a reason for hiding this comment

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

Nice.

iam_role = "${aws_iam_role.ecs-service.name}"
task_definition = "${aws_ecs_task_definition.jupyterhub.arn}"

load_balancer {

Choose a reason for hiding this comment

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

You weren't able to omit this? Does it work via the ELB now or is it still broken because of the WebSocket dependency?

Copy link
Member Author

@jamesmcclain jamesmcclain Aug 31, 2017

Choose a reason for hiding this comment

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

I have not attempted to, yet. I would like to merge this as-is in order for the image changes to be published, then I will come back and implement your suggestions.

So far my recollections are: (1) splitting up of the security groups and (2) removing the load_balancer block. Are there any others?

Choose a reason for hiding this comment

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

Yeah, security_group_rule and trying to remove the load_balancer block sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a work in progress PR up do deal with those two issues: #45.

@jamesmcclain jamesmcclain changed the title Terraform Terraform Part 1 Aug 31, 2017
@jamesmcclain jamesmcclain merged commit 7b9fd54 into geodocker:master Aug 31, 2017
@jamesmcclain jamesmcclain deleted the feature/terraform branch August 31, 2017 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants