provider/aws: Migrate aws_dms_* resources away from AWS waiters #13291
Conversation
stack72
changed the title
stack72
force-pushed
the
b-aws-dms-away-from-waiter
branch
from
2bd7dcb
to
6be45dd
Compare
stack72
force-pushed
the
b-aws-dms-away-from-waiter
branch
2 times, most recently
from
6c8bafd
to
84a2dcd
Compare
stack72
changed the title
| if waitErr != nil { | ||
| return waitErr | ||
| } | ||
|
|
There was a problem hiding this comment.
Is it OK to no longer wait for deletion here? Waiting for Delete is usually a lesser concern than say waiting for creation, but it's still caused dependency issues in tear-down before
There was a problem hiding this comment.
This isn't an issue luckily :) This wasn't necessary luckily
| //err = waitForInstanceCreated(conn, d.Get("replication_instance_id").(string), 30, 20) | ||
| //if err != nil { | ||
| // return err | ||
| //} |
| @@ -287,6 +304,23 @@ func resourceAwsDmsReplicationInstanceUpdate(d *schema.ResourceData, meta interf | |||
| return err | |||
| } | |||
|
|
|||
| d.SetId(d.Get("replication_instance_id").(string)) | |||
There was a problem hiding this comment.
Do we need re-set ID here, or is this just copy/pasta?
| Client: client, | ||
| Input: input, | ||
| Config: config, | ||
| return v, *v.ReplicationInstances[0].ReplicationInstanceStatus, nil |
There was a problem hiding this comment.
This can panic if v.ReplicationInstances is nil, yeah? Not sure how likely that is, but seems worth guarding
There was a problem hiding this comment.
I swear that was != above... did you rebase? ;)
| Pending: []string{"creating"}, | ||
| Target: []string{"available"}, | ||
| Refresh: resourceAwsDmsReplicationInstanceStateRefreshFunc(d, meta), | ||
| Timeout: d.Timeout(schema.TimeoutCreate), |
There was a problem hiding this comment.
schema.TimeoutCreate isn't defined, are you just leveraging the system level 20 minute default?
| @@ -235,12 +255,26 @@ func resourceAwsDmsReplicationTaskDelete(d *schema.ResourceData, meta interface{ | |||
|
|
|||
| _, err := conn.DeleteReplicationTask(request) | |||
| if err != nil { | |||
| if dmserr, ok := err.(awserr.Error); ok && dmserr.Code() == "ResourceNotFoundFault" { | |||
| d.SetId("") | |||
There was a problem hiding this comment.
We should log with [DEBUG] that the resource d.Id() wasn't found and is being removed from state, so users can at least see in the debug log what/why this is happening
There was a problem hiding this comment.
I don't see it (logging that it will be removed)
stack72
force-pushed
the
b-aws-dms-away-from-waiter
branch
from
84a2dcd
to
d1affbd
Compare
|
@catsby changes made as requested :) |
| @@ -235,12 +255,26 @@ func resourceAwsDmsReplicationTaskDelete(d *schema.ResourceData, meta interface{ | |||
|
|
|||
| _, err := conn.DeleteReplicationTask(request) | |||
| if err != nil { | |||
| if dmserr, ok := err.(awserr.Error); ok && dmserr.Code() == "ResourceNotFoundFault" { | |||
| d.SetId("") | |||
There was a problem hiding this comment.
I don't see it (logging that it will be removed)
The AWS waiter package has changed location in the 1.8.0 version of the SDK. DMS will need to mitigate a breaking change because of this Between @radeksimko and myself, we think that we should migrate the DMS resources to using the Terraform state refresh func pattern that is used across the entire of the AWS provider. DMS is the *only* resource that currently uses the AWS waiters, so the LOE to migrate is pretty low
stack72
force-pushed
the
b-aws-dms-away-from-waiter
branch
from
d1affbd
to
7448e6d
Compare
|
Last log line added, @catsby - will merge on green :) |
|
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. |
ghost
locked and limited conversation to collaborators
The AWS waiter package has changed location in the 1.8.0 version of the
SDK. DMS will need to mitigate a breaking change because of this
Between @radeksimko and myself, we think that we should migrate the DMS
resources to using the Terraform state refresh func pattern that is used
across the entire of the AWS provider. DMS is the only resource that
currently uses the AWS waiters, so the LOE to migrate is pretty low