resource/ecs_service: Support ServiceRegistries #3906

atsushi-ishibashi · 2018-03-24T08:21:50Z

required: #3905
rebase after #3905 merged.

TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withServiceRegistries -timeout 120m
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (131.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	131.944s

atsushi-ishibashi · 2018-03-26T15:30:09Z

rebased

jeffnappi · 2018-04-03T00:47:21Z

Looking forward to making use of this 👍 Thanks!

stevecrozz · 2018-04-03T02:46:05Z

I gave this a try today @atsushi-ishibashi. It looks like its going to work, but I did run into one minor issue when using it with a aws_service_discovery_service which uses A records. The Port parameter should be optional as it says in your documentation, but when I don't provide a port, it looks like this code provides a blank one and I get an error:

InvalidParameterException: A port was provided but is not required by the specified service registry

bflad · 2018-04-03T19:55:51Z

Issue reference: #4025

kgraney · 2018-04-04T01:47:01Z

aws/resource_aws_ecs_service.go

@@ -204,6 +204,24 @@ func resourceAwsEcsService() *schema.Resource {
 					},
 				},
 			},
+
+			"service_registries": {


I think this can only support MaxItems == 1. When I tried using more than one registry in #4026 I got back an error from AWS.

kgraney · 2018-04-04T01:48:38Z

aws/resource_aws_ecs_service.go

+			sr := &ecs.ServiceRegistry{
+				RegistryArn: aws.String(raw["registry_arn"].(string)),
+			}
+			if port, ok := raw["port"].(int); ok {


I think you want ok && port != 0 to prevent what @stevecrozz was seeing.

Sure👍 I'll fix

atsushi-ishibashi · 2018-04-04T06:34:31Z

TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withServiceRegistries -timeout 120m
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (111.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	111.336s

bflad

Thanks for this contribution! Left some initial comments below. Please let me know if you have any questions or do not have time to implement the feedback.

bflad · 2018-04-04T20:11:26Z

aws/resource_aws_ecs_service.go

@@ -204,6 +204,25 @@ func resourceAwsEcsService() *schema.Resource {
 					},
 				},
 			},
+
+			"service_registries": {
+				Type:     schema.TypeSet,


To simplify the implementation, we should opt for schema.TypeList here.

So far, MaxItems is 1 but there is a possibility that aws increases it.
And the order of service_registries has no meaning, so I think set is better.

bflad · 2018-04-04T20:12:11Z

aws/resource_aws_ecs_service.go

+						"registry_arn": {
+							Type:     schema.TypeString,
+							Required: true,
+						},


We should probably add ValidateFunc: validateArn, for this attribute.

bflad · 2018-04-04T20:12:40Z

aws/resource_aws_ecs_service.go

+						"port": {
+							Type:     schema.TypeInt,
+							Optional: true,
+						},


We should probably add ValidateFunc: validation.IntBetween(0, 65536), for this attribute

bflad · 2018-04-04T20:14:05Z

aws/resource_aws_ecs_service.go

+	serviceRegistries := d.Get("service_registries").(*schema.Set).List()
+	if len(serviceRegistries) > 0 {
+		srs := make([]*ecs.ServiceRegistry, 0, len(serviceRegistries))
+		for _, v := range serviceRegistries {


We do not need the for loop for MaxItems == 1 attributes 😄

There is a possibility of increase.

bflad · 2018-04-04T20:16:54Z

aws/resource_aws_ecs_service.go

+	}
+	results := make([]map[string]interface{}, 0)
+	for _, sr := range srs {
+		c := make(map[string]interface{})


This can be simplified to:

m := map[string]interface{}{ "port": int(aws.Int64Value(sr.Port)), "registry_arn": aws.StringValue(sr.RegistryArn), } results = append(results, m)

When sr.Port is null, is it OK?

bflad · 2018-04-04T20:18:07Z

aws/resource_aws_ecs_service_test.go

+				Config: testAccAWSEcsService_withServiceRegistries(rString, clusterName, tdName, svcName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSEcsServiceExists("aws_ecs_service.test", &service),
+					resource.TestCheckResourceAttr("aws_ecs_service.test", "service_registries.#", "1"),


We should test for service_registries.# of "0" in the basic test.

bflad · 2018-04-04T20:18:42Z

aws/resource_aws_ecs_service_test.go

+  name = "tf-acc-sd-%s"
+  dns_config {
+    namespace_id = "${aws_service_discovery_private_dns_namespace.test.id}"
+		dns_records {


Minor nitpick: indentation

bflad · 2018-04-04T20:18:59Z

aws/resource_aws_ecs_service_test.go

+  desired_count = 1
+  service_registries {
+    port = 34567
+		registry_arn = "${aws_service_discovery_service.test.arn}"


Minor nitpick: indentation

atsushi-ishibashi · 2018-04-05T03:16:08Z

TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withServiceRegistries -timeout 120m
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (73.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	73.323s

TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsService_withARN -timeout 120m
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (66.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.194s

Wesam18 · 2018-04-12T16:30:05Z

@bflad @atsushi-ishibashi
We do have a need for this feature. When do we expect to have it released?

bflad

Thank you for getting this support added! LGTM 🚀

Test failure unrelated:

Tests failed: 1, passed: 16
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (11.22s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (31.93s)
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (32.22s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (41.49s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (42.18s)
=== RUN   TestAccAWSEcsService_basicImport
--- PASS: TestAccAWSEcsService_basicImport (42.45s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (47.64s)
=== RUN   TestAccAWSEcsService_withServiceRegistries
--- PASS: TestAccAWSEcsService_withServiceRegistries (49.93s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (53.54s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (63.49s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (70.93s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (73.19s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (119.23s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (145.02s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (194.58s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (230.54s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- FAIL: TestAccAWSEcsService_withLaunchTypeFargate (260.51s)
    testing.go:518: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: aws_security_group.allow_all_a
          ingress.#:                             "0" => "1"
          ingress.2269895920.cidr_blocks.#:      "0" => "1"
          ingress.2269895920.cidr_blocks.0:      "" => "10.10.0.0/16"
          ingress.2269895920.description:        "" => ""
          ingress.2269895920.from_port:          "" => "80"
          ingress.2269895920.ipv6_cidr_blocks.#: "0" => "0"
          ingress.2269895920.protocol:           "" => "tcp"
          ingress.2269895920.security_groups.#:  "0" => "0"
          ingress.2269895920.self:               "" => "false"
          ingress.2269895920.to_port:            "" => "8000"

bflad · 2018-04-18T18:22:25Z

This has been released in version 1.15.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-06T17:42:52Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Mar 24, 2018

radeksimko added the service/ecs Issues and PRs that pertain to the ecs service. label Mar 26, 2018

resource/ecs_service: Support ServiceRegistries

cd54da5

atsushi-ishibashi force-pushed the ecs_service_registry branch from a7964cf to cd54da5 Compare March 26, 2018 15:29

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. dependencies Used to indicate dependency changes. labels Mar 26, 2018

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Apr 3, 2018

bflad mentioned this pull request Apr 3, 2018

[WIP] Add support for ECS service discovery #4026

Closed

kgraney reviewed Apr 4, 2018

View reviewed changes

Reflect reviews

f0a4347

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 4, 2018

bflad reviewed Apr 4, 2018

View reviewed changes

review

3f41ea6

bflad approved these changes Apr 12, 2018

View reviewed changes

bflad added this to the v1.15.0 milestone Apr 12, 2018

bflad merged commit 4ff21b9 into hashicorp:master Apr 12, 2018

bflad added a commit that referenced this pull request Apr 12, 2018

Update CHANGELOG for #3906

ad91628

arledesma mentioned this pull request Apr 20, 2018

Add support for ECS Service Discovery #4025

Closed

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/ecs_service: Support ServiceRegistries #3906

resource/ecs_service: Support ServiceRegistries #3906

atsushi-ishibashi commented Mar 24, 2018

atsushi-ishibashi commented Mar 26, 2018

jeffnappi commented Apr 3, 2018

stevecrozz commented Apr 3, 2018

bflad commented Apr 3, 2018

kgraney Apr 4, 2018

atsushi-ishibashi Apr 4, 2018

kgraney Apr 4, 2018

atsushi-ishibashi Apr 4, 2018

atsushi-ishibashi commented Apr 4, 2018

bflad left a comment

bflad Apr 4, 2018

atsushi-ishibashi Apr 5, 2018 •

edited

Loading

bflad Apr 4, 2018

atsushi-ishibashi Apr 5, 2018

bflad Apr 4, 2018

bflad Apr 4, 2018

atsushi-ishibashi Apr 5, 2018

bflad Apr 4, 2018

atsushi-ishibashi Apr 5, 2018

atsushi-ishibashi Apr 5, 2018

bflad Apr 4, 2018

atsushi-ishibashi Apr 5, 2018

bflad Apr 4, 2018

bflad Apr 4, 2018

atsushi-ishibashi commented Apr 5, 2018

Wesam18 commented Apr 12, 2018

bflad left a comment

bflad commented Apr 18, 2018

ghost commented Apr 6, 2020

resource/ecs_service: Support ServiceRegistries #3906

resource/ecs_service: Support ServiceRegistries #3906

Conversation

atsushi-ishibashi commented Mar 24, 2018

atsushi-ishibashi commented Mar 26, 2018

jeffnappi commented Apr 3, 2018

stevecrozz commented Apr 3, 2018

bflad commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Apr 4, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Apr 5, 2018

Wesam18 commented Apr 12, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 18, 2018

ghost commented Apr 6, 2020

atsushi-ishibashi Apr 5, 2018 •

edited

Loading