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

Initial implementation #3

Merged
merged 9 commits into from
Sep 14, 2018
Merged

Initial implementation #3

merged 9 commits into from
Sep 14, 2018

Conversation

aknysh
Copy link
Member

@aknysh aknysh commented Sep 12, 2018

what

  • Initial implementation of terraform-aws-ec2-autoscale-group

why

@aknysh aknysh self-assigned this Sep 12, 2018
@aknysh aknysh requested review from osterman and goruha September 12, 2018 21:39
Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

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

Missing cloudwatch autoscale logic. The approach we took with ECS was provide the standard mechanisms, but allow user to disable.

See @Jamie-BitFlight 's implementation.

https://github.com/bitflight-public/terraform-aws-ecs-cluster/blob/master/ondemand-autoscaling/cloudwatch.tf

main.tf Outdated
}
}

resource "aws_launch_configuration" "default" {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we should use launch templates: azavea/terraform-aws-ecs-cluster#30

main.tf Outdated
tags = "${var.tags}"

additional_tag_map = {
"propagate_at_launch" = "true"
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be quoted?

main.tf Outdated
vpc_zone_identifier = ["${var.subnet_ids}"]
max_size = "${var.max_size}"
min_size = "${var.min_size}"
desired_capacity = "${var.desired_capacity}"

Choose a reason for hiding this comment

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

IMO putting a conditional here that defaults the value of desired_capacity to var.min_size if var.desired_capacity is unset would be beneficial. Setting this additional value is bit tedious and in my experience almost always winds up being equal to the setting for min_size.

main.tf Outdated
user_data_base64 = "${var.user_data_base64}"
enable_monitoring = "${var.enable_monitoring}"
ebs_optimized = "${var.ebs_optimized}"
root_block_device = "${var.root_block_device}"

Choose a reason for hiding this comment

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

It might be more beneficial for an end user to provide values for storage type, disk size, and boolean for delete on termination as variables for root_block_device. Looking at the syntax in the example with the way it's currently written I think these three variables would be a better experience as a user of the module.

Choose a reason for hiding this comment

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

This is hard to explain without providing an example, please let me know if you need clarification.

main.tf Outdated
key_name = "${var.key_name}"
security_groups = ["${var.security_groups}"]
associate_public_ip_address = "${var.associate_public_ip_address}"
user_data_base64 = "${var.user_data_base64}"

Choose a reason for hiding this comment

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

Just a comment, I would recommend including this in the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @MoonMoon1919
I'll update it

Copy link

@MoonMoon1919 MoonMoon1919 left a comment

Choose a reason for hiding this comment

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

Overall looks good, a few comments/suggestions to make things more simple on the user side.

CA_CERTIFICATE_FILE_PATH=$CA_CERTIFICATE_DIRECTORY/ca.crt
mkdir -p $CA_CERTIFICATE_DIRECTORY
echo XXXXXXXXXXXX | base64 -d > $CA_CERTIFICATE_FILE_PATH
INTERNAL_IP=$(curl -s http://169.254.169.254/latest/meta-data/local-ipv4)
Copy link
Member

Choose a reason for hiding this comment

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

Use envsubst

README.md Outdated
mkdir -p $CA_CERTIFICATE_DIRECTORY
echo XXXXXXXXXXXX | base64 -d > $CA_CERTIFICATE_FILE_PATH
INTERNAL_IP=$(curl -s http://169.254.169.254/latest/meta-data/local-ipv4)
sed -i s,MASTER_ENDPOINT,master.us-west-2.testing.cloudposse.co,g /var/lib/kubelet/kubeconfig
Copy link
Member

Choose a reason for hiding this comment

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

Use simpler example.

@osterman
Copy link
Member

@aknysh see https://github.com/cloudposse/terraform-aws-ecs-web-app

I think maybe using cpu_.... and memory_... is a better convention as it allows us to group the arguments lexicographically.

@aknysh aknysh requested a review from osterman September 13, 2018 22:08
@aknysh aknysh merged commit 1cdb08f into master Sep 14, 2018
@aknysh aknysh deleted the init branch September 14, 2018 03:30
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.

3 participants