-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
r/aws_ecs_service: fix load_balancer
& service_registries
to update without destroy & recreate
#23786
r/aws_ecs_service: fix load_balancer
& service_registries
to update without destroy & recreate
#23786
Conversation
…ork without destroy & recreate
@justinretzolk thank you for having a look at this PR. You labeled it as enhancement but it also fixes a possible panic, so this is mainly a bugfix for #23600. Should this be labeled as a bugfix then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀.
% make testacc TESTS=TestAccECSService_ PKG=ecs ACCTEST_PARALLELISM=2
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 2 -run='TestAccECSService_' -timeout 180m
=== RUN TestAccECSService_basic
=== PAUSE TestAccECSService_basic
=== RUN TestAccECSService_basicImport
=== PAUSE TestAccECSService_basicImport
=== RUN TestAccECSService_disappears
=== PAUSE TestAccECSService_disappears
=== RUN TestAccECSService_PlacementStrategy_unnormalized
=== PAUSE TestAccECSService_PlacementStrategy_unnormalized
=== RUN TestAccECSService_CapacityProviderStrategy_basic
=== PAUSE TestAccECSService_CapacityProviderStrategy_basic
=== RUN TestAccECSService_CapacityProviderStrategy_forceNewDeployment
=== PAUSE TestAccECSService_CapacityProviderStrategy_forceNewDeployment
=== RUN TestAccECSService_CapacityProviderStrategy_update
=== PAUSE TestAccECSService_CapacityProviderStrategy_update
=== RUN TestAccECSService_CapacityProviderStrategy_multiple
=== PAUSE TestAccECSService_CapacityProviderStrategy_multiple
=== RUN TestAccECSService_familyAndRevision
=== PAUSE TestAccECSService_familyAndRevision
=== RUN TestAccECSService_renamedCluster
=== PAUSE TestAccECSService_renamedCluster
=== RUN TestAccECSService_healthCheckGracePeriodSeconds
=== PAUSE TestAccECSService_healthCheckGracePeriodSeconds
=== RUN TestAccECSService_iamRole
=== PAUSE TestAccECSService_iamRole
=== RUN TestAccECSService_DeploymentControllerType_codeDeploy
=== PAUSE TestAccECSService_DeploymentControllerType_codeDeploy
=== RUN TestAccECSService_DeploymentControllerType_codeDeployUpdateDesiredCountAndHealthCheckGracePeriod
=== PAUSE TestAccECSService_DeploymentControllerType_codeDeployUpdateDesiredCountAndHealthCheckGracePeriod
=== RUN TestAccECSService_DeploymentControllerType_external
=== PAUSE TestAccECSService_DeploymentControllerType_external
=== RUN TestAccECSService_DeploymentValues_basic
=== PAUSE TestAccECSService_DeploymentValues_basic
=== RUN TestAccECSService_DeploymentValues_minZeroMaxOneHundred
=== PAUSE TestAccECSService_DeploymentValues_minZeroMaxOneHundred
=== RUN TestAccECSService_deploymentCircuitBreaker
=== PAUSE TestAccECSService_deploymentCircuitBreaker
=== RUN TestAccECSService_loadBalancerChanges
=== PAUSE TestAccECSService_loadBalancerChanges
=== RUN TestAccECSService_clusterName
=== PAUSE TestAccECSService_clusterName
=== RUN TestAccECSService_alb
=== PAUSE TestAccECSService_alb
=== RUN TestAccECSService_multipleTargetGroups
=== PAUSE TestAccECSService_multipleTargetGroups
=== RUN TestAccECSService_forceNewDeployment
=== PAUSE TestAccECSService_forceNewDeployment
=== RUN TestAccECSService_PlacementStrategy_basic
=== PAUSE TestAccECSService_PlacementStrategy_basic
=== RUN TestAccECSService_PlacementStrategy_missing
=== PAUSE TestAccECSService_PlacementStrategy_missing
=== RUN TestAccECSService_PlacementConstraints_basic
=== PAUSE TestAccECSService_PlacementConstraints_basic
=== RUN TestAccECSService_PlacementConstraints_emptyExpression
=== PAUSE TestAccECSService_PlacementConstraints_emptyExpression
=== RUN TestAccECSService_LaunchTypeFargate_basic
=== PAUSE TestAccECSService_LaunchTypeFargate_basic
=== RUN TestAccECSService_LaunchTypeFargate_platformVersion
=== PAUSE TestAccECSService_LaunchTypeFargate_platformVersion
=== RUN TestAccECSService_LaunchTypeFargate_waitForSteadyState
=== PAUSE TestAccECSService_LaunchTypeFargate_waitForSteadyState
=== RUN TestAccECSService_LaunchTypeFargate_updateWaitForSteadyState
=== PAUSE TestAccECSService_LaunchTypeFargate_updateWaitForSteadyState
=== RUN TestAccECSService_LaunchTypeEC2_network
=== PAUSE TestAccECSService_LaunchTypeEC2_network
=== RUN TestAccECSService_DaemonSchedulingStrategy_basic
=== PAUSE TestAccECSService_DaemonSchedulingStrategy_basic
=== RUN TestAccECSService_DaemonSchedulingStrategy_setDeploymentMinimum
=== PAUSE TestAccECSService_DaemonSchedulingStrategy_setDeploymentMinimum
=== RUN TestAccECSService_replicaSchedulingStrategy
=== PAUSE TestAccECSService_replicaSchedulingStrategy
=== RUN TestAccECSService_ServiceRegistries_basic
=== PAUSE TestAccECSService_ServiceRegistries_basic
=== RUN TestAccECSService_ServiceRegistries_container
=== PAUSE TestAccECSService_ServiceRegistries_container
=== RUN TestAccECSService_ServiceRegistries_changes
=== PAUSE TestAccECSService_ServiceRegistries_changes
=== RUN TestAccECSService_Tags_basic
=== PAUSE TestAccECSService_Tags_basic
=== RUN TestAccECSService_Tags_managed
=== PAUSE TestAccECSService_Tags_managed
=== RUN TestAccECSService_Tags_propagate
=== PAUSE TestAccECSService_Tags_propagate
=== RUN TestAccECSService_executeCommand
=== PAUSE TestAccECSService_executeCommand
=== CONT TestAccECSService_basic
=== CONT TestAccECSService_multipleTargetGroups
--- PASS: TestAccECSService_basic (89.50s)
=== CONT TestAccECSService_DaemonSchedulingStrategy_basic
--- PASS: TestAccECSService_DaemonSchedulingStrategy_basic (43.07s)
=== CONT TestAccECSService_executeCommand
--- PASS: TestAccECSService_executeCommand (67.07s)
=== CONT TestAccECSService_Tags_propagate
--- PASS: TestAccECSService_Tags_propagate (100.98s)
=== CONT TestAccECSService_Tags_managed
--- PASS: TestAccECSService_multipleTargetGroups (319.75s)
=== CONT TestAccECSService_Tags_basic
--- PASS: TestAccECSService_Tags_managed (59.99s)
=== CONT TestAccECSService_ServiceRegistries_changes
--- PASS: TestAccECSService_Tags_basic (104.04s)
=== CONT TestAccECSService_replicaSchedulingStrategy
--- PASS: TestAccECSService_replicaSchedulingStrategy (90.51s)
=== CONT TestAccECSService_DaemonSchedulingStrategy_setDeploymentMinimum
--- PASS: TestAccECSService_DaemonSchedulingStrategy_setDeploymentMinimum (40.39s)
=== CONT TestAccECSService_iamRole
--- PASS: TestAccECSService_iamRole (71.33s)
=== CONT TestAccECSService_alb
--- PASS: TestAccECSService_ServiceRegistries_changes (266.71s)
=== CONT TestAccECSService_clusterName
--- PASS: TestAccECSService_clusterName (87.34s)
=== CONT TestAccECSService_loadBalancerChanges
--- PASS: TestAccECSService_alb (317.41s)
=== CONT TestAccECSService_deploymentCircuitBreaker
--- PASS: TestAccECSService_loadBalancerChanges (301.35s)
=== CONT TestAccECSService_DeploymentValues_minZeroMaxOneHundred
--- PASS: TestAccECSService_deploymentCircuitBreaker (87.56s)
=== CONT TestAccECSService_DeploymentValues_basic
--- PASS: TestAccECSService_DeploymentValues_minZeroMaxOneHundred (87.41s)
=== CONT TestAccECSService_DeploymentControllerType_external
--- PASS: TestAccECSService_DeploymentValues_basic (87.63s)
=== CONT TestAccECSService_DeploymentControllerType_codeDeployUpdateDesiredCountAndHealthCheckGracePeriod
--- PASS: TestAccECSService_DeploymentControllerType_external (42.55s)
=== CONT TestAccECSService_ServiceRegistries_basic
--- PASS: TestAccECSService_ServiceRegistries_basic (155.03s)
=== CONT TestAccECSService_ServiceRegistries_container
--- PASS: TestAccECSService_ServiceRegistries_container (170.69s)
=== CONT TestAccECSService_DeploymentControllerType_codeDeploy
--- PASS: TestAccECSService_DeploymentControllerType_codeDeployUpdateDesiredCountAndHealthCheckGracePeriod (384.12s)
=== CONT TestAccECSService_CapacityProviderStrategy_update
--- PASS: TestAccECSService_DeploymentControllerType_codeDeploy (330.96s)
=== CONT TestAccECSService_healthCheckGracePeriodSeconds
--- PASS: TestAccECSService_CapacityProviderStrategy_update (335.18s)
=== CONT TestAccECSService_renamedCluster
--- PASS: TestAccECSService_renamedCluster (87.67s)
=== CONT TestAccECSService_LaunchTypeFargate_basic
--- PASS: TestAccECSService_LaunchTypeFargate_basic (133.12s)
=== CONT TestAccECSService_familyAndRevision
--- PASS: TestAccECSService_healthCheckGracePeriodSeconds (321.01s)
=== CONT TestAccECSService_LaunchTypeEC2_network
--- PASS: TestAccECSService_familyAndRevision (80.70s)
=== CONT TestAccECSService_LaunchTypeFargate_updateWaitForSteadyState
--- PASS: TestAccECSService_LaunchTypeEC2_network (88.74s)
=== CONT TestAccECSService_LaunchTypeFargate_waitForSteadyState
--- PASS: TestAccECSService_LaunchTypeFargate_updateWaitForSteadyState (166.19s)
=== CONT TestAccECSService_LaunchTypeFargate_platformVersion
--- PASS: TestAccECSService_LaunchTypeFargate_waitForSteadyState (185.04s)
=== CONT TestAccECSService_CapacityProviderStrategy_multiple
--- PASS: TestAccECSService_LaunchTypeFargate_platformVersion (117.01s)
=== CONT TestAccECSService_PlacementStrategy_basic
--- PASS: TestAccECSService_CapacityProviderStrategy_multiple (114.26s)
=== CONT TestAccECSService_disappears
--- PASS: TestAccECSService_PlacementStrategy_basic (123.05s)
=== CONT TestAccECSService_PlacementStrategy_missing
--- PASS: TestAccECSService_PlacementStrategy_missing (0.76s)
=== CONT TestAccECSService_basicImport
--- PASS: TestAccECSService_disappears (71.12s)
=== CONT TestAccECSService_PlacementConstraints_emptyExpression
--- PASS: TestAccECSService_basicImport (91.10s)
=== CONT TestAccECSService_PlacementConstraints_basic
--- PASS: TestAccECSService_PlacementConstraints_emptyExpression (86.35s)
=== CONT TestAccECSService_PlacementStrategy_unnormalized
--- PASS: TestAccECSService_PlacementConstraints_basic (98.10s)
=== CONT TestAccECSService_CapacityProviderStrategy_forceNewDeployment
--- PASS: TestAccECSService_PlacementStrategy_unnormalized (86.65s)
=== CONT TestAccECSService_CapacityProviderStrategy_basic
--- PASS: TestAccECSService_CapacityProviderStrategy_forceNewDeployment (147.74s)
=== CONT TestAccECSService_forceNewDeployment
--- PASS: TestAccECSService_forceNewDeployment (73.27s)
--- PASS: TestAccECSService_CapacityProviderStrategy_basic (218.72s)
PASS
ok github.com/hashicorp/terraform-provider-aws/internal/service/ecs 2981.217s
@kevinscholz Thanks for the contribution 🎉 👏. |
@ewbankkit Thank you for the quick response and you efforts. Actually I did not change the tests to the target groups for the following reason: In case somebody is now (after releasing this PR) using the classic load balancer and change it for an ECS service, TF detects it as updatable, tries it and it fails. So we basically break updating ECS services when using a classic load balancer. That's why I posted the question in the description. Feel free to correct me if I'm wrong. |
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. |
Community Note
Relates to #23596
Relates to #23581
Relates to #23600
The above mentioned PR seems to not eliminate the recreation of the ECS service when updating the
load_balancer
andservice_registries
. It looks like there were some left overForceNew
s.Also, there is a cast error when using the
load_balancer
field which can lead to a panic.Unfortunately it seems like the AWS ECS API has a bug in the UpdateService call since we cannot update with a classic load balancer. That's also the reason why the last test fails. Any idea how to proceed here?
Output from acceptance testing: