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.

Sign up for GitHub

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

Jump to bottom

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

Merged
merged 6 commits into from
Oct 31, 2018
Merged

aws_ecs_service support deployment_minimum_healthy_percent for DAEMON strategy #6150

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
213d964
enabling MinimumHealthyPercent for DAEMON
mschurenko Oct 14, 2018
3985d60
setting deployment_minimum_healthy_percent in test
mschurenko Oct 14, 2018
f7983bf
using nil value for input.DeploymentConfiguration.MaximumPercent
mschurenko Oct 30, 2018
7306998
using AWS API defaults for deployment_maximum_percent and deployment_…
mschurenko Oct 31, 2018
dd69f6a
adding TestAccAWSEcsService_withDaemonSchedulingStrategySetDeployment…
mschurenko Oct 31, 2018
7fda67f
updating ecs service html markdown
mschurenko Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions aws/resource_aws_ecs_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,6 @@ func resourceAwsEcsService() *schema.Resource {
Type: schema.TypeInt,
Optional: true,
Default: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of leaving a Default: 100 here that only works for one of the scheduling strategies, should we instead remove Default: 100 and implement a DiffSuppressFunc that covers suppressing the difference depending on the strategy? A similar update should likely occur for maximum as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove the Default wouldn't that be backwards incompatible for those who are using a replica strategy and didn't set deployment_minimum_healthy_percent?

Similar for deployment_maximum_percent, if we remove Default won't that be a problem for backwards compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

The port attribute of the aws_elasticache_cluster resource is an example of a place where we allow two defaults chosen by the API, but suppress the difference if it is left unconfigured in the Terraform configuration: https://github.com/terraform-providers/terraform-provider-aws/blob/4ef574a20eeecd860ef40bf4ca94849cee9af0ef/aws/resource_aws_elasticache_cluster.go#L146-L157

Its not pretty, but it works. 😄 We could also set Computed: true.

Copy link
Contributor Author

@mschurenko mschurenko Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. So when you say we allow two defaults chosen by the API, do you mean by the AWS API?

DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if d.Get("scheduling_strategy").(string) == ecs.SchedulingStrategyDaemon {
return true
}
return false
},
},

"load_balancer": {
Expand Down Expand Up @@ -354,7 +348,7 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
input.SchedulingStrategy = aws.String(schedulingStrategy)
if schedulingStrategy == ecs.SchedulingStrategyDaemon {
// unset these if DAEMON
input.DeploymentConfiguration = nil
input.DeploymentConfiguration.MaximumPercent = aws.Int64(100)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to keep this logic here at all? Since the comment eludes to "unsetting" values due to DAEMON we should either remove this line or add a comment why its being hardcoded to 100

input.DesiredCount = nil
}

Expand Down Expand Up @@ -767,13 +761,20 @@ func resourceAwsEcsServiceUpdate(d *schema.ResourceData, meta interface{}) error
input.TaskDefinition = aws.String(n.(string))
}

if schedulingStrategy != ecs.SchedulingStrategyDaemon && (d.HasChange("deployment_maximum_percent") || d.HasChange("deployment_minimum_healthy_percent")) {
if schedulingStrategy == ecs.SchedulingStrategyReplica && (d.HasChange("deployment_maximum_percent") || d.HasChange("deployment_minimum_healthy_percent")) {
input.DeploymentConfiguration = &ecs.DeploymentConfiguration{
MaximumPercent: aws.Int64(int64(d.Get("deployment_maximum_percent").(int))),
MinimumHealthyPercent: aws.Int64(int64(d.Get("deployment_minimum_healthy_percent").(int))),
}
}

if schedulingStrategy == ecs.SchedulingStrategyDaemon && d.HasChange("deployment_minimum_healthy_percent") {
input.DeploymentConfiguration = &ecs.DeploymentConfiguration{
MaximumPercent: aws.Int64(100),
MinimumHealthyPercent: aws.Int64(int64(d.Get("deployment_minimum_healthy_percent").(int))),
}
}

if d.HasChange("network_configuration") {
input.NetworkConfiguration = expandEcsNetworkConfiguration(d.Get("network_configuration").([]interface{}))
}
Expand Down
1 change: 1 addition & 0 deletions aws/resource_aws_ecs_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2092,6 +2092,7 @@ resource "aws_ecs_service" "ghost" {
cluster = "${aws_ecs_cluster.default.id}"
task_definition = "${aws_ecs_task_definition.ghost.family}:${aws_ecs_task_definition.ghost.revision}"
scheduling_strategy = "DAEMON"
deployment_minimum_healthy_percent = "50"
bflad marked this conversation as resolved.
Show resolved Hide resolved
}
`, clusterName, tdName, svcName)
}
Expand Down