provider/aws: Deregister ECS task definition correctly

[radeksimko]

Deregistration has been finally implemented and it expects revision as an argument, which means that if user modifies the task definition, new revision is generated = revision + arn bumped.

1. The old revision is not referenced after that change anywhere anymore, which raises a question whether we should just deregister the old revision after the change has been successfully applied? 2. We now treat `ecs_task_definition` as a resource holding all revisions to make updates in definitions easy. Should we iterate over all revisions and deregister all of them on `destroy`? How do we know if these revisions have been created by Terraform?

Test plan

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=Ecs' 2>/dev/null

go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=Ecs -timeout 90m
=== RUN TestAccAWSEcsCluster_basic
--- PASS: TestAccAWSEcsCluster_basic (1.48s)
=== RUN TestAccAWSEcsServiceWithARN
--- PASS: TestAccAWSEcsServiceWithARN (20.41s)
=== RUN TestAccAWSEcsServiceWithFamilyAndRevision
--- PASS: TestAccAWSEcsServiceWithFamilyAndRevision (29.80s)
=== RUN TestAccAWSEcsTaskDefinition_basic
--- PASS: TestAccAWSEcsTaskDefinition_basic (2.01s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    53.714s

[mitchellh]

Uhhh, I think it should just deregister everything if we have nothing more fine grained to do. I defer to your best decision for this.

[radeksimko]

I was also chatting w/ AWS this week and they confirmed that there's no way of completely getting rid of unregistered/inactive task definitions and they treat that as a feature. All old/unregistered TDs will stay in the given AWS account basically forever.

This effectively means that once a revision for TD family is generated, it will never be reused in the future. It also means that even if we unregister all TDs for a given family, a brand new resource using the same family name will have revision last_revision+1 anyway.

Considering all of the above and Terraform's goal not to be obstructive and not clash with other tools user may use for managing resources, I will probably implement only unregistration of old versions created by Terraform (i.e. previous revision during update & last revision during destroy).

[radeksimko]

This is updated according to decisions above and ready to be reviewed & merged.

If it is too late for this to be merged before 0.6.0, then I will add a note to docs that unregistration is implemented in 0.6.1+, so people aren't confused.

[catsby]

we often return the results of the read method when we're finished in the update method, is that appropriate here?

[radeksimko]

When you say results, you mean error or nil, right?
I don't mind doing it, it just means extra 3 lines, because the error coming from DeregisterTaskDefinition needs to be handled too - i.e.

if err != nil {
  return err
}

[catsby]

yeah, I just meant this:

return resourceAwsEcsTaskDefinitionRead(d, meta)

so yes we'd need to check the error first. The main question is if that's appropriate, e.g. anything new or computed that needs to be updated after this update? The kind of thing that read would pick up. Just wondering, no blocking here

[radeksimko]

Hmm, I see, it's probably one extra API call, but better safe than sorry I guess...

[radeksimko]

@catsby Fixed according to comments, here's the diff since I amended the last commit to keep history clean:

diff --git a/builtin/providers/aws/resource_aws_ecs_task_definition.go b/builtin/providers/aws/resource_aws_ecs_task_definition.go
index e8e2eb1..fe28252 100644
--- a/builtin/providers/aws/resource_aws_ecs_task_definition.go
+++ b/builtin/providers/aws/resource_aws_ecs_task_definition.go
@@ -141,12 +141,17 @@ func resourceAwsEcsTaskDefinitionUpdate(d *schema.ResourceData, meta interface{}
        }
        log.Printf("[DEBUG] New revision of %q created: %q", d.Id(), d.Get("arn").(string))

-       log.Printf("[DEBUG] Unregistering old revision of task definition %q: %q", d.Id(), oldArn)
+       log.Printf("[DEBUG] Deregistering old revision of task definition %q: %q", d.Id(), oldArn)
        conn := meta.(*AWSClient).ecsconn
        _, err = conn.DeregisterTaskDefinition(&ecs.DeregisterTaskDefinitionInput{
                TaskDefinition: aws.String(oldArn),
        })
-       return err
+       if err != nil {
+               return err
+       }
+       log.Printf("[DEBUG] Old revision of task definition deregistered: %q", oldArn)
+
+       return resourceAwsEcsTaskDefinitionRead(d, meta)
 }

 func resourceAwsEcsTaskDefinitionDelete(d *schema.ResourceData, meta interface{}) error {
@@ -159,7 +164,7 @@ func resourceAwsEcsTaskDefinitionDelete(d *schema.ResourceData, meta interface{}
                return err
        }

-       log.Printf("[DEBUG] Task definition %s deregistered.", d.Id())
+       log.Printf("[DEBUG] Task definition %q deregistered.", d.Get("arn").(string))

        return nil
 }

[catsby]

Looks good to me! 👍

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.