New Data Source: ECS Service

[elbuo8]

Closes #1681

[kwyjibos]

Would be very useful if we could get this reviewed/merged

[n1te1337]

Is there any help needed to get this across the line?

[bflad]

Hi @elbuo8 👋 Thanks for submitting this and sorry for the delay in getting it reviewed. It was in decent shape. I left feedback below which I'm going to go ahead and handle on merge since its been open so long.

[bflad]

This attribute and the two below are not configurable (Optional: true) but rather read only and meant to just be set in the Terraform state (Computed: true). https://www.terraform.io/docs/extend/schemas/schema-behaviors.html

[bflad]

We generally prefer to check and immediately return errors for zero returned items or more than one returned item instead of using for loops in singular data sources. e.g.

if len(desc.Services) == 0 {
  return fmt.Errorf("service with name %q in cluster %q not found", serviceName, clusterArn)
}

if len(desc.Services) > 1 {
  return fmt.Errorf("multiple services with name %q found in cluster %q", serviceName, clusterArn)
}

service := desc.Service[0]
d.SetId(aws.StringValue(service.ServiceArn))
// ...

[bflad]

Nitpick: We can use resource.TestCheckResourceAttrPair() to ensure the resource and data source attributes match as expected.

[bflad]

This documentation page is missing a sidebar link in website/aws.erb

[bflad]

Normally we try to not show an unexpected configuration for newer operators (e.g. the data source directly referencing its resource counterpart) -- in this case its fine to simply just list the example as:

data "aws_ecs_service" "example" {
   cluster_arn  = "${data.aws_ecs_cluster.example.arn}"
   service_name = "example"
 }

[bflad]

🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsServiceDataSource_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsServiceDataSource_basic -timeout 120m
=== RUN   TestAccAWSEcsServiceDataSource_basic
--- PASS: TestAccAWSEcsServiceDataSource_basic (34.14s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	34.179s

[bflad]

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

[n1te1337]

@elbuo8 RE: #1681 I was really excited about this PR hoping it would solve my CI/CD issues but now when I think about it, it creates a different issue.

By using it like so:

data "aws_ecs_service" "main" {
  cluster_name = "mycluster"
  service_name = "voting"
}

resource "aws_ecs_service" "main" {
  name            = "voting"
  task_definition = "${data.aws_ecs_service.main.task_definition}"
  desired_count   = 1
}

You solve the issue where the CI/CD updates the task and terraform is still in sync because the data source will pull the currently active task revision for the service (which is great).

Unfortunately, by doing so you also create a dependency on the service itself because you expect the service to be up and running. This pretty much prohibits the use of the data source for new services, the only way to do it is to create a task and a service manually and only after that change it to use the data source (so you can't really automate it for infrastructure deployments).

Thoughts?

[ghost]

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!