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

Allow consuming TF code to alter wait_for_capacity_timeout. #23

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

joestump
Copy link
Contributor

Includes a default value of 10m, which is TF's default as well. We have some ASGs that keep timing out when we adjust capacity.

Copy link
Member

@tfhartmann tfhartmann left a comment

Choose a reason for hiding this comment

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

All in all, this looks great! Let me know what you think about the comments inline!

value = "${var.name} ${var.tagName}"
propagate_at_launch = true
}]
name_prefix = "asg-${aws_launch_configuration.ecs.name}-"
Copy link
Member

Choose a reason for hiding this comment

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

I think this change will mean we'll need to cut a new major version release. Am I reading that right? Meaning, if someone upgrades to this version of the module, it'll tear down and recreate the autoscaling groups on them? (I haven't had a chance to pull this code and test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that it'll tear down any existing ASGs and bring them backup with prefixed names.

propagate_at_launch = true
},
{
key = "Terraform"
Copy link
Member

Choose a reason for hiding this comment

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

Cool!

min_size = "${var.min_servers}"
max_size = "${var.max_servers}"
desired_capacity = "${var.servers}"
wait_for_capacity_timeout = "${var.wait_for_capacity_timeout}"
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 add a similar function to heartbeat_timeout ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. It's how long TF will wait for a capacity change before timing out. I added it because we had an ASG I wanted to take from 10 to 1 and it timed out.

@@ -100,7 +113,8 @@ resource "aws_security_group" "ecs" {
}

tags {
Name = "ecs-sg-${var.name}"
Name = "ecs-sg-${var.name}"
Terraform = "yes"
Copy link
Member

Choose a reason for hiding this comment

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

I like these tags, one though, what would you think about the tags reading "true" rather then "yes" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have zero preference. We use this pattern internally, but happy to follow whatever patterns the community/maintainers prefer.

Copy link
Member

Choose a reason for hiding this comment

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

cool, lets switch this to true and I'll merge PR in.

@tfhartmann
Copy link
Member

It looks like the last PR added a conflict, so this branch may need a rebase.

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.

2 participants