fix: Use default timeouts for Fabric wait methods #631
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
===========================================
+ Coverage 38.07% 65.56% +27.49%
===========================================
Files 120 120
Lines 19393 19427 +34
===========================================
+ Hits 7384 12738 +5354
+ Misses 11801 6054 -5747
- Partials 208 635 +427 ☔ View full report in Codecov by Sentry. |
| fcr, _, err := client.CloudRoutersApi.CreateCloudRouter(ctx, createRequest) | ||
| if err != nil { | ||
| return diag.FromErr(equinix_errors.FormatFabricError(err)) | ||
| } | ||
| d.SetId(fcr.Uuid) | ||
|
|
||
| if _, err = waitUntilCloudRouterIsProvisioned(d.Id(), meta, ctx); err != nil { | ||
| createTimeout := d.Timeout(schema.TimeoutCreate) - 30*time.Second - time.Since(start) |
There was a problem hiding this comment.
Why is 30s removed off the top of each timeout? Is this an off-by-one solution for catching the terminal 30s iteration?
There was a problem hiding this comment.
@displague , I had the same question when I first saw it. It's the pattern that Oscar shared when creating the issue: #627
I believe it's to counter the 30 Second time delay before the wait methods send their first polling request; the timeout then starts after that time delay. That's the understanding that I took from it being present.
| Read: schema.DefaultTimeout(6 * time.Minute), | ||
| Create: schema.DefaultTimeout(15 * time.Minute), | ||
| Update: schema.DefaultTimeout(15 * time.Minute), | ||
| Delete: schema.DefaultTimeout(10 * time.Minute), |
There was a problem hiding this comment.
Please, can you make the delete timer 15 minutes as well? I was just about to create a PR for the same change. We had many issues with this today.
There was a problem hiding this comment.
Sure, no issues. Updating.
NOTE: We have errors running Fabric tests in parallel for now. Not sure of the issue, but this change has been testing manually locally with each test being run in an isolated manner. All tests are successful. Logs are too large to post.