Fix migration to terraform-plugin-testing causes panic in existing test suite #1167
Conversation
…eoutError, StateRefreshFunc, StateChangeConf, RetryFunc, RetryContext, Retry, RetryError, RetryableError and NonRetryableError Equivalent types and functions are now available in the helper/retry package (#1166)
| "context" | ||
| "time" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" |
There was a problem hiding this comment.
Do we need to add the exports from the new id package to this set of aliases as well?
| @@ -0,0 +1,6 @@ | |||
| kind: NOTES | |||
| body: '''helper/resource: Deprecated `PrefixedUniqueId()` and `UniqueId()`. Use the | |||
| `helper/id` package instead.''' | |||
There was a problem hiding this comment.
Awkward as it may be, I think we should include enhancement entries for the new packages and target deprecations on minor versions so folks are less "surprised". I'm not sure if its worth calling out that the existing package code was converted to aliases to the migrated code in case that surfaces weirdness with tooling such as bflad/tfproviderlint or other AST-based tooling.
It may also be good if these deprecation notices call out that these deprecations are to assist in migrating to terraform-plugin-testing.
helper/resource/aliases.go
Outdated
| r := retry.NonRetryableError(err) | ||
|
|
||
| return &RetryError{ | ||
| Err: err, |
There was a problem hiding this comment.
This should use the return value as the called function may change the error.
| Err: err, | |
| Err: r.Err, |
| @@ -76,9 +76,9 @@ If a CRUD function timeout is exceeded, the SDK will automatically return a `con | |||
| The retry helper takes a timeout and a retry function. | |||
|
|
|||
| - The **timeout** value specifies the maximum time Terraform will invoke the retry function. You can retrieve the timeout from the `*schema.ResourceData` struct by passing the timeout key (`schema.TimeoutCreate`) to the `Timeout` method. | |||
| - The **retry function** returns either a `resource.NonRetryableError` for unexpected errors/states or a `resource.RetryableError` for expected errrors/states. If the function returns a `resource.RetryableError`, it will re-run the function. | |||
| - The **retry function** returns either a `retry.NonRetryableError` for unexpected errors/states or a `retry.RetryableError` for expected errrors/states. If the function returns a `retry.RetryableError`, it will re-run the function. | |||
There was a problem hiding this comment.
This documentation wasn't doing it before, but it might be a good time to call out the import for this package since its different than any code example above.
* Expanding aliases to include helper/id * Expanding contents of changie notes and adding enhancement entries * Updating documentation to refer to import of helper/retry
|
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 contributions. |
Closes: #1166